You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/12/01 03:25:28 UTC
Re: svn commit: r1040831 - in /subversion/trunk/subversion:
include/svn_checksum.h libsvn_subr/checksum.c
stefan2@apache.org wrote on Tue, Nov 30, 2010 at 23:56:41 -0000:
> Author: stefan2
> Date: Tue Nov 30 23:56:40 2010
> New Revision: 1040831
>
> URL: http://svn.apache.org/viewvc?rev=1040831&view=rev
> Log:
> Fix 'svnadmin verify' for format 5 repositories. During the checking process,
> they yield NULL for SHA1 checksums. The most robust way to fix that is to
> allow NULL for checksum in svn_checksum_to_cstring. This seems justified
> as NULL is already a valid return value instead of e.g. an empty string. So,
> callers may receive NULL as result and could therefore assume that NULL is
> a valid input, too
>
Can you explain how to trigger the bug you are fixing? Just running
'svnadmin verify' on my r1040058-created Greek repository doesn't.
> * subversion/include/svn_checksum.h
> (svn_checksum_to_cstring): extend doc string
> * subversion/libsvn_subr/checksum.c
> (svn_checksum_to_cstring): return NULL for NULL checksums as well
>
> Modified:
> subversion/trunk/subversion/include/svn_checksum.h
> subversion/trunk/subversion/libsvn_subr/checksum.c
>
> Modified: subversion/trunk/subversion/include/svn_checksum.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_checksum.h?rev=1040831&r1=1040830&r2=1040831&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_checksum.h (original)
> +++ subversion/trunk/subversion/include/svn_checksum.h Tue Nov 30 23:56:40 2010
> @@ -121,7 +121,7 @@ svn_checksum_to_cstring_display(const sv
>
> /** Return the hex representation of @a checksum, allocating the
> * string in @a pool. If @a checksum->digest is all zeros (that is,
> - * 0, not '0'), then return NULL.
> + * 0, not '0') or NULL, then return NULL.
> *
First, this change doesn't match the code change: the docstring change
says CHECKSUM->DIGEST may be NULL, but the code change allows CHECKSUM
to be NULL.
Second, let's have the docstring say that NULL is only allowed by 1.7+.
(Passing NULL will segfault in 1.6.)
(doxygen has a @note directive.)
> * @since New in 1.6.
> */
>
> Modified: subversion/trunk/subversion/libsvn_subr/checksum.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/checksum.c?rev=1040831&r1=1040830&r2=1040831&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/checksum.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/checksum.c Tue Nov 30 23:56:40 2010
> @@ -135,6 +135,9 @@ const char *
> svn_checksum_to_cstring(const svn_checksum_t *checksum,
> apr_pool_t *pool)
> {
> + if (checksum == NULL)
> + return NULL;
> +
> switch (checksum->kind)
> {
> case svn_checksum_md5:
>
>
Re: svn commit: r1040831 - in /subversion/trunk/subversion:
include/svn_checksum.h libsvn_subr/checksum.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Dec 01, 2010 at 11:44:55AM +0100, Stefan Sperling wrote:
> On Wed, Dec 01, 2010 at 05:25:28AM +0200, Daniel Shahaf wrote:
> > stefan2@apache.org wrote on Tue, Nov 30, 2010 at 23:56:41 -0000:
> > > Author: stefan2
> > > Date: Tue Nov 30 23:56:40 2010
> > > New Revision: 1040831
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1040831&view=rev
> > > Log:
> > > Fix 'svnadmin verify' for format 5 repositories. During the checking process,
> > > they yield NULL for SHA1 checksums. The most robust way to fix that is to
> > > allow NULL for checksum in svn_checksum_to_cstring. This seems justified
> > > as NULL is already a valid return value instead of e.g. an empty string. So,
> > > callers may receive NULL as result and could therefore assume that NULL is
> > > a valid input, too
> > >
> >
> > Can you explain how to trigger the bug you are fixing? Just running
> > 'svnadmin verify' on my r1040058-created Greek repository doesn't.
> >
> > > * subversion/include/svn_checksum.h
> > > (svn_checksum_to_cstring): extend doc string
> > > * subversion/libsvn_subr/checksum.c
> > > (svn_checksum_to_cstring): return NULL for NULL checksums as well
> > >
> > > Modified:
> > > subversion/trunk/subversion/include/svn_checksum.h
> > > subversion/trunk/subversion/libsvn_subr/checksum.c
> > >
> > > Modified: subversion/trunk/subversion/include/svn_checksum.h
> > > URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_checksum.h?rev=1040831&r1=1040830&r2=1040831&view=diff
> > > ==============================================================================
> > > --- subversion/trunk/subversion/include/svn_checksum.h (original)
> > > +++ subversion/trunk/subversion/include/svn_checksum.h Tue Nov 30 23:56:40 2010
> > > @@ -121,7 +121,7 @@ svn_checksum_to_cstring_display(const sv
> > >
> > > /** Return the hex representation of @a checksum, allocating the
> > > * string in @a pool. If @a checksum->digest is all zeros (that is,
> > > - * 0, not '0'), then return NULL.
> > > + * 0, not '0') or NULL, then return NULL.
> > > *
> >
> > First, this change doesn't match the code change: the docstring change
> > says CHECKSUM->DIGEST may be NULL, but the code change allows CHECKSUM
> > to be NULL.
> >
> > Second, let's have the docstring say that NULL is only allowed by 1.7+.
> > (Passing NULL will segfault in 1.6.)
> >
> > (doxygen has a @note directive.)
>
> Yes, this certainly violates API backwards compatibility rules.
Well, it doesn't really since NULL was an invalid parameter in 1.6.
Ignore my statement above.
> A regression test that shows the problem would be nice.
Re: svn commit: r1040831 - in /subversion/trunk/subversion:
include/svn_checksum.h libsvn_subr/checksum.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Dec 01, 2010 at 05:25:28AM +0200, Daniel Shahaf wrote:
> stefan2@apache.org wrote on Tue, Nov 30, 2010 at 23:56:41 -0000:
> > Author: stefan2
> > Date: Tue Nov 30 23:56:40 2010
> > New Revision: 1040831
> >
> > URL: http://svn.apache.org/viewvc?rev=1040831&view=rev
> > Log:
> > Fix 'svnadmin verify' for format 5 repositories. During the checking process,
> > they yield NULL for SHA1 checksums. The most robust way to fix that is to
> > allow NULL for checksum in svn_checksum_to_cstring. This seems justified
> > as NULL is already a valid return value instead of e.g. an empty string. So,
> > callers may receive NULL as result and could therefore assume that NULL is
> > a valid input, too
> >
>
> Can you explain how to trigger the bug you are fixing? Just running
> 'svnadmin verify' on my r1040058-created Greek repository doesn't.
>
> > * subversion/include/svn_checksum.h
> > (svn_checksum_to_cstring): extend doc string
> > * subversion/libsvn_subr/checksum.c
> > (svn_checksum_to_cstring): return NULL for NULL checksums as well
> >
> > Modified:
> > subversion/trunk/subversion/include/svn_checksum.h
> > subversion/trunk/subversion/libsvn_subr/checksum.c
> >
> > Modified: subversion/trunk/subversion/include/svn_checksum.h
> > URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_checksum.h?rev=1040831&r1=1040830&r2=1040831&view=diff
> > ==============================================================================
> > --- subversion/trunk/subversion/include/svn_checksum.h (original)
> > +++ subversion/trunk/subversion/include/svn_checksum.h Tue Nov 30 23:56:40 2010
> > @@ -121,7 +121,7 @@ svn_checksum_to_cstring_display(const sv
> >
> > /** Return the hex representation of @a checksum, allocating the
> > * string in @a pool. If @a checksum->digest is all zeros (that is,
> > - * 0, not '0'), then return NULL.
> > + * 0, not '0') or NULL, then return NULL.
> > *
>
> First, this change doesn't match the code change: the docstring change
> says CHECKSUM->DIGEST may be NULL, but the code change allows CHECKSUM
> to be NULL.
>
> Second, let's have the docstring say that NULL is only allowed by 1.7+.
> (Passing NULL will segfault in 1.6.)
>
> (doxygen has a @note directive.)
Yes, this certainly violates API backwards compatibility rules.
A regression test that shows the problem would be nice.
Thanks,
Stefan
>
> > * @since New in 1.6.
> > */
> >
> > Modified: subversion/trunk/subversion/libsvn_subr/checksum.c
> > URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/checksum.c?rev=1040831&r1=1040830&r2=1040831&view=diff
> > ==============================================================================
> > --- subversion/trunk/subversion/libsvn_subr/checksum.c (original)
> > +++ subversion/trunk/subversion/libsvn_subr/checksum.c Tue Nov 30 23:56:40 2010
> > @@ -135,6 +135,9 @@ const char *
> > svn_checksum_to_cstring(const svn_checksum_t *checksum,
> > apr_pool_t *pool)
> > {
> > + if (checksum == NULL)
> > + return NULL;
> > +
> > switch (checksum->kind)
> > {
> > case svn_checksum_md5:
> >
> >
Re: svn commit: r1040831 - in /subversion/trunk/subversion:
include/svn_checksum.h libsvn_subr/checksum.c
Posted by Julian Foad <ju...@wandisco.com>.
On Sun, 2010-12-05, Stefan Fuhrmann wrote:
> On 02.12.2010 12:18, Julian Foad wrote:
> > A good test for whether it's worth making an API accept NULL as an input
> > is: what proportion of the callers would find that useful? I see there
> > are about 40 callers in the code base. Would you mind scanning through
> > them and letting us know?
> >
> There were two places where the callers had to check
> for NULL before calling svn_checksum_to_cstring().
> I removed those checks in r1042460.
Great. And three more in load-fs-vtable.c, which I removed in r1042673.
> Two more places might try to call the function with a NULL
> checksum under adverse conditions:
>
> * 6x in dump_node() (subversion/libsvn_repos/dump.c)
> because the svn_fs_file_checksum() is not instructed
> to calculate missing checksums
> * While constructing an error message in window_handler()
> (subversion\libsvn_wc\update_editor.c)
>
> There are about ten more places where it is not entirely
> crystal clear that the desired checksum is actually available.
> However, I'm reasonably sure that they will in the respective
> contexts (e.g. wc operations).
Thanks for checking, Stefan. That gives me reason to like this change.
- Julian
Re: svn commit: r1040831 - in /subversion/trunk/subversion: include/svn_checksum.h
libsvn_subr/checksum.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 02.12.2010 12:18, Julian Foad wrote:
> A good test for whether it's worth making an API accept NULL as an input
> is: what proportion of the callers would find that useful? I see there
> are about 40 callers in the code base. Would you mind scanning through
> them and letting us know?
>
There were two places where the callers had to check
for NULL before calling svn_checksum_to_cstring().
I removed those checks in r1042460.
Two more places might try to call the function with a NULL
checksum under adverse conditions:
* 6x in dump_node() (subversion/libsvn_repos/dump.c)
because the svn_fs_file_checksum() is not instructed
to calculate missing checksums
* While constructing an error message in window_handler()
(subversion\libsvn_wc\update_editor.c)
There are about ten more places where it is not entirely
crystal clear that the desired checksum is actually available.
However, I'm reasonably sure that they will in the respective
contexts (e.g. wc operations).
-- Stefan^2.
Re: svn commit: r1040831 - in /subversion/trunk/subversion:
include/svn_checksum.h libsvn_subr/checksum.c
Posted by Julian Foad <ju...@wandisco.com>.
Hi Stefan2.
A good test for whether it's worth making an API accept NULL as an input
is: what proportion of the callers would find that useful? I see there
are about 40 callers in the code base. Would you mind scanning through
them and letting us know?
- Julian
On Thu, 2010-12-02 at 07:51 +0200, Daniel Shahaf wrote:
> Stefan Fuhrmann wrote on Thu, Dec 02, 2010 at 01:39:34 +0100:
> > On 01.12.2010 04:25, Daniel Shahaf wrote:
> >> stefan2@apache.org wrote on Tue, Nov 30, 2010 at 23:56:41 -0000:
> >>> Author: stefan2
> >>> Date: Tue Nov 30 23:56:40 2010
> >>> New Revision: 1040831
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1040831&view=rev
> >>> Log:
> >>> Fix 'svnadmin verify' for format 5 repositories. During the checking process,
> >>> they yield NULL for SHA1 checksums. The most robust way to fix that is to
> >>> allow NULL for checksum in svn_checksum_to_cstring. This seems justified
> >>> as NULL is already a valid return value instead of e.g. an empty string. So,
> >>> callers may receive NULL as result and could therefore assume that NULL is
> >>> a valid input, too
> >>>
> >> Can you explain how to trigger the bug you are fixing? Just running
> >> 'svnadmin verify' on my r1040058-created Greek repository doesn't.
> > Sure:
> >
> > $svnadmin-1.5.4 create /mnt/svnroot/test
> > $<add pre-revprop-change hook>
> > $svnsync-1.5.4 init file:///mnt/svnroot/test http://svn.apache.org/repos/asf
> > $svnsync-1.5.4 sync file:///mnt/svnroot/test
> > $<cancel after a few revs; rev 1 will already trigger the error>
> > $svnadmin-trunk verify /mnt/svnroot/test
>
> And then the dump logic calls svn_fs_file_checksum(force=FALSE), which
> causes a NULL checksum to be returned.
>
> Thanks for the recipe; knowing it it's easier to review the fix.
>
> Daniel
Re: svn commit: r1040831 - in /subversion/trunk/subversion:
include/svn_checksum.h libsvn_subr/checksum.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Thu, Dec 02, 2010 at 01:39:34 +0100:
> On 01.12.2010 04:25, Daniel Shahaf wrote:
>> stefan2@apache.org wrote on Tue, Nov 30, 2010 at 23:56:41 -0000:
>>> Author: stefan2
>>> Date: Tue Nov 30 23:56:40 2010
>>> New Revision: 1040831
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1040831&view=rev
>>> Log:
>>> Fix 'svnadmin verify' for format 5 repositories. During the checking process,
>>> they yield NULL for SHA1 checksums. The most robust way to fix that is to
>>> allow NULL for checksum in svn_checksum_to_cstring. This seems justified
>>> as NULL is already a valid return value instead of e.g. an empty string. So,
>>> callers may receive NULL as result and could therefore assume that NULL is
>>> a valid input, too
>>>
>> Can you explain how to trigger the bug you are fixing? Just running
>> 'svnadmin verify' on my r1040058-created Greek repository doesn't.
> Sure:
>
> $svnadmin-1.5.4 create /mnt/svnroot/test
> $<add pre-revprop-change hook>
> $svnsync-1.5.4 init file:///mnt/svnroot/test http://svn.apache.org/repos/asf
> $svnsync-1.5.4 sync file:///mnt/svnroot/test
> $<cancel after a few revs; rev 1 will already trigger the error>
> $svnadmin-trunk verify /mnt/svnroot/test
And then the dump logic calls svn_fs_file_checksum(force=FALSE), which
causes a NULL checksum to be returned.
Thanks for the recipe; knowing it it's easier to review the fix.
Daniel
Re: svn commit: r1040831 - in /subversion/trunk/subversion: include/svn_checksum.h
libsvn_subr/checksum.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 01.12.2010 04:25, Daniel Shahaf wrote:
> stefan2@apache.org wrote on Tue, Nov 30, 2010 at 23:56:41 -0000:
>> Author: stefan2
>> Date: Tue Nov 30 23:56:40 2010
>> New Revision: 1040831
>>
>> URL: http://svn.apache.org/viewvc?rev=1040831&view=rev
>> Log:
>> Fix 'svnadmin verify' for format 5 repositories. During the checking process,
>> they yield NULL for SHA1 checksums. The most robust way to fix that is to
>> allow NULL for checksum in svn_checksum_to_cstring. This seems justified
>> as NULL is already a valid return value instead of e.g. an empty string. So,
>> callers may receive NULL as result and could therefore assume that NULL is
>> a valid input, too
>>
> Can you explain how to trigger the bug you are fixing? Just running
> 'svnadmin verify' on my r1040058-created Greek repository doesn't.
Sure:
$svnadmin-1.5.4 create /mnt/svnroot/test
$<add pre-revprop-change hook>
$svnsync-1.5.4 init file:///mnt/svnroot/test http://svn.apache.org/repos/asf
$svnsync-1.5.4 sync file:///mnt/svnroot/test
$<cancel after a few revs; rev 1 will already trigger the error>
$svnadmin-trunk verify /mnt/svnroot/test
>> * subversion/include/svn_checksum.h
>> (svn_checksum_to_cstring): extend doc string
>> * subversion/libsvn_subr/checksum.c
>> (svn_checksum_to_cstring): return NULL for NULL checksums as well
>>
>> Modified:
>> subversion/trunk/subversion/include/svn_checksum.h
>> subversion/trunk/subversion/libsvn_subr/checksum.c
>>
>> Modified: subversion/trunk/subversion/include/svn_checksum.h
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_checksum.h?rev=1040831&r1=1040830&r2=1040831&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/include/svn_checksum.h (original)
>> +++ subversion/trunk/subversion/include/svn_checksum.h Tue Nov 30 23:56:40 2010
>> @@ -121,7 +121,7 @@ svn_checksum_to_cstring_display(const sv
>>
>> /** Return the hex representation of @a checksum, allocating the
>> * string in @a pool. If @a checksum->digest is all zeros (that is,
>> - * 0, not '0'), then return NULL.
>> + * 0, not '0') or NULL, then return NULL.
>> *
> First, this change doesn't match the code change: the docstring change
> says CHECKSUM->DIGEST may be NULL, but the code change allows CHECKSUM
> to be NULL.
>
> Second, let's have the docstring say that NULL is only allowed by 1.7+.
> (Passing NULL will segfault in 1.6.)
>
> (doxygen has a @note directive.)
>
Done.
-- Stefan^2.
>> * @since New in 1.6.
>> */
>>
>> Modified: subversion/trunk/subversion/libsvn_subr/checksum.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/checksum.c?rev=1040831&r1=1040830&r2=1040831&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_subr/checksum.c (original)
>> +++ subversion/trunk/subversion/libsvn_subr/checksum.c Tue Nov 30 23:56:40 2010
>> @@ -135,6 +135,9 @@ const char *
>> svn_checksum_to_cstring(const svn_checksum_t *checksum,
>> apr_pool_t *pool)
>> {
>> + if (checksum == NULL)
>> + return NULL;
>> +
>> switch (checksum->kind)
>> {
>> case svn_checksum_md5:
>>
>>