Philip Martin wrote on Thu, Apr 18, 2013 at 19:53:47 +0100:
> 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:
>
> 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.
What would a better fix be?
The following is a first sketch, it should resolve the compiler warning
but won't solve the actual semantic bug (if there is one) here.
Index: subversion/libsvn_fs_fs/tree.c
===================================================================
--- subversion/libsvn_fs_fs/tree.c (revision 1469842)
+++ subversion/libsvn_fs_fs/tree.c (working copy)
@@ -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 = NULL;
+ const char *directory;
assert(svn_fs__is_canonical_abspath(path));
if (flags & open_path_node_only)
{
@@ -925,6 +925,8 @@ open_path(parent_path_t **parent_path_p,
/* did the shortcut work? */
if (here)
{
+ /* Assert that DIRECTORY is initialised. */
+ SVN_ERR_ASSERT(flags & open_path_node_only);
path_so_far = directory;
rest = path + strlen(directory) + 1;
}
Received on 2013-04-19 16:23:36 CEST