On 14.05.2013 08:05, Blair Zajac wrote: > On 4/18/13 11:53 AM, Philip Martin wrote: >> bl...@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
