On Fri, Sep 10, 2010 at 04:47:39PM +0200, Bert Huijben wrote:
>
>
> > -----Original Message-----
> > From: Stefan Sperling [mailto:stsp_at_elego.de]
> > Sent: vrijdag 10 september 2010 15:26
> > To: Bert Huijben
> > Cc: dev_at_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
Received on 2010-09-10 17:11:58 CEST