On Fri, Jun 5, 2009 at 8:45 PM, Neels J Hofmeyr<neels_at_elego.de> wrote:
> Paul Burba wrote:
>> On Fri, Jun 5, 2009 at 10:40 AM, Neels Janosch Hofmeyr<neels_at_elego.de> wrote:
>>> Author: neels
>>> Date: Fri Jun 5 07:40:07 2009
>>> New Revision: 37939
>>>
>>> Log:
>>> Fix segfault from r37795 and add regression test.
>>> The segfault occurs when svnsync encounters deletion of an
>>> "svn:*" property.
>>>
>>> * subversion/svnsync/main.c
>>> (normalize_string): Check for NULL pointer.
>>>
>>> * subversion/tests/cmdline/svnsync_tests_data/delete-svn-props.dump:
>>> New dump file for svnsync cmdline tests that involves the removal of
>>> an "svn:eol-style" file-property (s.b.).
>>>
>>> * subversion/tests/cmdline/svnsync_tests.py
>>> (delete_svn_props): New test for this segfault, using the dump file.
>>>
>>> The dump file above was created with the lines:
>>> [[[
>>> set -x
>>> dir=repos
>>> url="file://`pwd`/$dir"
>>> propname=svn:eol-style
>>> propval=LF
>>> rm -rf ./$dir
>>> rm -rf ./WC
>>> svnadmin create $dir
>>> svn co $url WC
>>> echo file-content > WC/file
>>> svn add WC/file
>>> svn ps $propname $propval WC/file
>>> svn ci -m "adding file with prop '$propname'" WC
>>> svn pd $propname WC/file
>>> svn ci -m "removing prop '$propname' from file" WC
>>> svnadmin dump $dir > delete-svn-props.dump
>>> ]]]
>>>
>>> Added:
>>> trunk/subversion/tests/cmdline/svnsync_tests_data/delete-svn-props.dump
>>> Modified:
>>> trunk/subversion/svnsync/main.c
>>> trunk/subversion/tests/cmdline/svnsync_tests.py
>>>
>>> Modified: trunk/subversion/svnsync/main.c
>>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svnsync/main.c?pathrev=37939&r1=37938&r2=37939
>>> ==============================================================================
>>> --- trunk/subversion/svnsync/main.c Fri Jun 5 07:19:08 2009 (r37938)
>>> +++ trunk/subversion/svnsync/main.c Fri Jun 5 07:40:07 2009 (r37939)
>>> @@ -529,6 +529,9 @@ normalize_string(const svn_string_t **st
>>> {
>>> *was_normalized = FALSE;
>>>
>>> + if (*str == NULL || (*str)->data == NULL)
>>
>> Hi Neels,
>>
>> Do we really need the second NULL check? (*str)->data should never be
>> NULL per svn_string_t's 1st invariant. If we are passing around an
>> svn_string_t with a NULL data member I don't think we should ignore
>> it.
>>
>> Paul
>
> ...oh, so it *was* the right way 'round in hyrum's mail.
>
> Ok, as you put it, the second check should be stricter. Making it an assert.
> Thanks for pointing it out. r37946, updated STATUS.
>
> ~Neels
Hi Neels,
No objection to the assert you added in r37946, it does no harm
obviously. But to clarify what I was suggesting: It was more that we
don't need this second check. Looking through our code it seems clear
that while we frequently check if svn_string_t *'s are NULL (a good
idea!), but if they are not NULL then we assume the svn_string_t's
data member is also non-NULL.
To expand on the documentation for svn_string_t's, "If code outside of
the svn_string.h functions manually builds these structures, then they
must enforce this invariant (and we may not want to trust that it is
true)". But that is not the case here AFAICT.
Anyhow, I voted and approved r37939 and r37946 for 1.6.3 as I never
intended to hold up the release over this.
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2360357
Received on 2009-06-08 18:41:46 CEST