You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@vmoo.com> on 2010/09/09 18:58:10 UTC
RE: svn commit: r995475 - /subversion/trunk/subversion/libsvn_repos/load.c
> -----Original Message-----
> From: stsp@apache.org [mailto:stsp@apache.org]
> Sent: donderdag 9 september 2010 17:52
> To: commits@subversion.apache.org
> Subject: svn commit: r995475 -
> /subversion/trunk/subversion/libsvn_repos/load.c
>
> Author: stsp
> Date: Thu Sep 9 15:51:34 2010
> New Revision: 995475
>
> URL: http://svn.apache.org/viewvc?rev=995475&view=rev
> Log:
> * subversion/libsvn_repos/load.c
> (parse_property_block): Use svn_cstring_atoi64() instead of atoi()
> (switching
> to 64bit because size_t isn't guaranteed to be 32bit).
>
> Modified:
> subversion/trunk/subversion/libsvn_repos/load.c
>
> Modified: subversion/trunk/subversion/libsvn_repos/load.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/lo
> ad.c?rev=995475&r1=995474&r2=995475&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_repos/load.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/load.c Thu Sep 9 15:51:34
> 2010
> @@ -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.
Bert
Re: svn commit: r995475 -
/subversion/trunk/subversion/libsvn_repos/load.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 10, 2010 at 04:02:52PM +0200, Branko Čibej wrote:
> On 10.09.2010 15:26, Stefan Sperling wrote:
> > 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...
>
> Microsoft's C compiler, to name only one, still does not provide most of
> the C99 features. I don't know about standard library support.
>
> However, I don't see where you gain with using strtol(). First of all,
> it was already in C89 and has effectively the same interface as the APR
> conversions. C99 added strtoll() but it has the same interface. What's
> the benefit?
Hmmm.. looking closer, you're right, the interface is the same.
It's just that the programmer (me) has to use it properly.
The strtol(3) man page has an interesting hint which is missing from
the apr_strtoi64() documentation. It wasn't clear to me how to make
sure that the entire string was a valid number, but this explains it:
If endptr is non-null, strtol() stores the address of the first invalid
character in *endptr. If there were no digits at all, however, strtol()
stores the original value of nptr in *endptr. (Thus, if *nptr is not
`\0' but **endptr is `\0' on return, the entire string was valid.)
I found this snippet in fs_fs.c (which has since been changed, I'm showing
an old version), where the caller is happy with a string like "42\n",
but potentially accepts "42foo":
/* note that apr_atoi64() will stop reading as soon as it encounters
the final newline. */
if (changes_offset)
*changes_offset = rev_offset + apr_atoi64(&buf[i]);
So that caller will need to be adjusted.
But yes, checking whether *endptr == '\0' should do the trick.
So I guess we can keep using apr_atoi64(). Thanks for the cluestick :)
Stefan
Re: svn commit: r995475 - /subversion/trunk/subversion/libsvn_repos/load.c
Posted by Branko Čibej <br...@xbc.nu>.
On 15.09.2010 21:03, Stefan Sperling wrote:
> On Fri, Sep 10, 2010 at 04:02:52PM +0200, Branko Čibej wrote:
>> On 10.09.2010 15:26, Stefan Sperling wrote:
>>> 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...
>> Microsoft's C compiler, to name only one, still does not provide most of
>> the C99 features. I don't know about standard library support.
>>
>> However, I don't see where you gain with using strtol(). First of all,
>> it was already in C89 and has effectively the same interface as the APR
>> conversions. C99 added strtoll() but it has the same interface. What's
>> the benefit?
> Raising this thread again, because there is a benefit to using strtol()'s
> unsigned cousin, strtoul(). APR does not provide an interface for it.
> Can we use it? Can we also use its 64-bit cousin strtoull()? If we can use
> those two, we might as well use strtol() and strtoul() and skip apr_strtoi64()
> completely.
You'd still have to provide an alternate implementation for all the
environments that do not provide this function. I think there may be one
with a different spelling in Microsoft's C runtime, but haven't looked
in a while ...
-- Brane
Re: svn commit: r995475 - /subversion/trunk/subversion/libsvn_repos/load.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Wed, Sep 15, 2010 at 9:03 PM, Stefan Sperling <st...@elego.de> wrote:
> On Fri, Sep 10, 2010 at 04:02:52PM +0200, Branko Čibej wrote:
>> On 10.09.2010 15:26, Stefan Sperling wrote:
>> > 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...
>>
>> Microsoft's C compiler, to name only one, still does not provide most of
>> the C99 features. I don't know about standard library support.
>>
>> However, I don't see where you gain with using strtol(). First of all,
>> it was already in C89 and has effectively the same interface as the APR
>> conversions. C99 added strtoll() but it has the same interface. What's
>> the benefit?
>
> Raising this thread again, because there is a benefit to using strtol()'s
> unsigned cousin, strtoul(). APR does not provide an interface for it.
> Can we use it? Can we also use its 64-bit cousin strtoull()? If we can use
> those two, we might as well use strtol() and strtoul() and skip apr_strtoi64()
> completely.
Adding an implementation to APR isn't a problem, but it would take a
while to trickle down (we'd still need a private implementation).
-Hyrum
Re: svn commit: r995475 -
/subversion/trunk/subversion/libsvn_repos/load.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 10, 2010 at 04:02:52PM +0200, Branko Čibej wrote:
> On 10.09.2010 15:26, Stefan Sperling wrote:
> > 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...
>
> Microsoft's C compiler, to name only one, still does not provide most of
> the C99 features. I don't know about standard library support.
>
> However, I don't see where you gain with using strtol(). First of all,
> it was already in C89 and has effectively the same interface as the APR
> conversions. C99 added strtoll() but it has the same interface. What's
> the benefit?
Raising this thread again, because there is a benefit to using strtol()'s
unsigned cousin, strtoul(). APR does not provide an interface for it.
Can we use it? Can we also use its 64-bit cousin strtoull()? If we can use
those two, we might as well use strtol() and strtoul() and skip apr_strtoi64()
completely.
Stefan
Re: svn commit: r995475 - /subversion/trunk/subversion/libsvn_repos/load.c
Posted by Branko Čibej <br...@xbc.nu>.
On 10.09.2010 15:26, Stefan Sperling wrote:
> 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...
Microsoft's C compiler, to name only one, still does not provide most of
the C99 features. I don't know about standard library support.
However, I don't see where you gain with using strtol(). First of all,
it was already in C89 and has effectively the same interface as the APR
conversions. C99 added strtoll() but it has the same interface. What's
the benefit?
-- Brane
Re: svn commit: r995475 -
/subversion/trunk/subversion/libsvn_repos/load.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 10, 2010 at 04:47:39PM +0200, Bert Huijben wrote:
>
>
> > -----Original Message-----
> > From: Stefan Sperling [mailto:stsp@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
RE: svn commit: r995475 - /subversion/trunk/subversion/libsvn_repos/load.c
Posted by Bert Huijben <be...@vmoo.com>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@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.
I think we need a special svn_string_atosize() (Beter name welcome) to
compensate for this problem.
Bert
Re: svn commit: r995475 -
/subversion/trunk/subversion/libsvn_repos/load.c
Posted by Stefan Sperling <st...@elego.de>.
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