Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c

On 14.05.2013 08:05, Blair Zajac wrote:
> On 4/18/13 11:53 AM, Philip Martin wrote:
>> blair_at_apache.org writes:
>>
>>> Author: blair
>>> Date: Thu Apr 18 18:41:55 2013
>>> New Revision: 1469520
>>>
>>> URL: http://svn.apache.org/r1469520
>>> Log:
>>> open_path(): silence compiler warning.
>>>
>>> subversion/libsvn_fs_fs/tree.c: In function 'open_path':
>>> subversion/libsvn_fs_fs/tree.c:916: warning: 'directory' may be used
>>> uninitialized in this function
>>>
>>> * subversion/libsvn_fs_fs/tree.c
>>> (open_path):
>>> Set directory to NULL to silence a compiler warning.
>>>
>>> Modified:
>>> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>>>
>>> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
>>> URL:
>>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1469520&r1=1469519&r2=1469520&view=diff
>>> ==============================================================================
>>>
>>> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
>>> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Thu Apr 18
>>> 18:41:55 2013
>>> @@ -913,7 +913,7 @@ open_path(parent_path_t **parent_path_p,
>>> a sibling of PATH has been presently accessed. Try to start
>>> the lookup
>>> directly at the parent node, if the caller did not requested
>>> the full
>>> parent chain. */
>>> - const char *directory;
>>> + const char *directory = NULL;
>>> assert(svn_fs__is_canonical_abspath(path));
>>> if (flags & open_path_node_only)
>>> {
>>>
>>>
>>
>> In the past we have not been adding these sorts of initialisations. A
>> build without warnings is nice but unnecessary initialisation can reduce
>> the effectiveness of memory checking tools. In this particular case the
>> code is:
>
> I just saw this now when looking through STATUS.
>
> I would say that we have been keeping our build warning free though,
> if it takes a initialization.
>
>>
>> if (here)
>> {
>> path_so_far = directory;
>> rest = path + strlen(directory) + 1;
>> }
>>
>> Passing NULL to strlen is not guaranteed to work but will work on some
>> platforms. Without the initialisation a memory checking tool like
>> valgrind will trigger if strlen were to access directory. The
>> initialisation means there is no longer a guarantee that valgrind will
>> trigger.
>
> I would say though that I would prefer keeping the runtime in better
> shape, say reading from NULL and getting a core dumo, than random
> memory location.
>
> I did assume that the code was good though and just did this to
> silence a warning.

This discussion is out of date, I committed a different fix yesterday
that doesn't require premature initialization, yet avoids the
uninitialized-read problem.

All it took is changing a bit more than one line of code.

-- Brane

-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com