On Fri, Sep 10, 2010 at 04:47:39PM +0200, Bert Huijben wrote: > > > > -----Original Message----- > > From: Stefan Sperling [mailto:s...@elego.de] > > Sent: vrijdag 10 september 2010 15:26 > > To: Bert Huijben > > Cc: dev@subversion.apache.org > > Subject: Re: svn commit: r995475 - > > /subversion/trunk/subversion/libsvn_repos/load.c > > > > 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. > > Yes, after 2^64 apr_stroi64() will set an error. But you truncate it to 2^32 > in the read_key_or_val() line. (You explicitly suppressed the warning for > this problem) > > So instead of erroring at 2^32 , you just created a different problem, with > possible different security implications.
Ah, I see. You're talking about the case where size_t is 32bit. > I think we need a special svn_string_atosize() (Beter name welcome) to > compensate for this problem. We can use svn_cstring_strtoui64() with minval = 0 and maxval == APR_SIZE_MAX. Since APR_SIZE_MAX will expand to the correct max value on either 32bit and 64bit platforms, this should trigger an error if the result won't fit into a 32bit size_t. Stefan
