Re: Should svn_fs_fs__noderev_same_rep_key() accept NULL representation_t::uniquifier?

Kouhei Sutou wrote:
> Hi,
>
> I don't know about svn_fs_fs__noderev_same_rep_key() but it
> causes "Segmentation fault" if a->uniquifier or
> b->uniquifier is NULL. Should it accepts NULL or not?

Yes. When reading pre-format-4 versions of the file system, the uniquifier will
 be NULL, so we should account for this possibility.

> If it accepts NULL as valid VALUE:
>
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 34710)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -3435,6 +3435,12 @@
> if (a->revision != b->revision)
> return FALSE;
>
> + if (a->uniquifier == b->uniquifier)

Should we explicitly make sure that these are both equal to NULL?

> + return TRUE;
> +
> + if (a->uniquifier == NULL || b->uniquifier == NULL)
> + return FALSE;
> +
> return strcmp(a->uniquifier, b->uniquifier) == 0;
> }
>
>
> Otherwise:
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 34710)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -3435,6 +3435,9 @@
> if (a->revision != b->revision)
> return FALSE;
>
> + if (a->uniquifier == NULL || b->uniquifier == NULL)
> + return FALSE;
> +
> return strcmp(a->uniquifier, b->uniquifier) == 0;
> }
>
>
> You can reproduce it by running the Ruby bindings test:
>
> % make swig-rb
> % make svnserve
> % cd binding/swig/ruby
> % test/run-test.rb -n test_transaction

Indeed I can. Thanks for tracking this down!

-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=984478