On Thu, Sep 09, 2010 at 08:58:10PM +0200, Bert Huijben wrote:
> > @@ -524,9 +524,11 @@ parse_property_block(svn_stream_t *strea
> > else if ((buf[0] == 'D') && (buf[1] == ' '))
> > {
> > char *keybuf;
> > + apr_int64_t len;
> >
> > + SVN_ERR(svn_cstring_atoi64(&len, buf + 2));
> > SVN_ERR(read_key_or_val(&keybuf, actual_length,
> > - stream, atoi(buf + 2), proppool));
> > + stream, (apr_size_t)len, proppool));
>
> What would this do if you have 4GB + 1 byte?
>
> I expected that we would use the new svn_cstring conversion variants
> to check for that kind of errors (for overflows, etc.), but now we
> just ignore the error at a different level.
How so? apr_strtoi64() will set errno upon overflow so we'll error out.
atoi() did no such thing.
BTW, I've been pondering about remaining problems with apr_strtoi64().
It doesn't seem to be a very good API.
It doesn't tell us reliably whether the string that it parsed was
in fact a valid number. E.g. I think that if it parses the string "0xYZ"
in base 16, the endptr will point to Y, errno isn't set, and the returned
value is zero (or possibly whatever we initialised the output argument to).
Which means that we probably can't tell apart "0x00YZ" from "0xYZ" without
actually looking at the string ourselves. Note that 0x00YZ is supposed to be
valid and parse as 0, because processing stops at the first invalid character,
rather than at '\0' -- which IMO is another misfeature of this API.
I've seen a comment somewhere in our code indicating that we're relying on
apr_strtoi64() to stop processing when it encounters a newline character.
But this could be fixed to pass in a NUL-terminated string instead.
Would it be OK to use the C99 strtol() family functions instead? That
would allow an easy and proper svn_cstring_atoi() etc. implementation.
I know we're using C89, but maybe it's time to move on and upgrade to C99
where the benefits are desirable? When Subversion was started, C89 was about
a decade old, and C99 is just as old now...
Stefan
Received on 2010-09-10 15:26:46 CEST