You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Blair Zajac <bl...@orcaware.com> on 2013/05/14 08:05:46 UTC
Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
On 4/18/13 11:53 AM, Philip Martin wrote:
> blair@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.
Blair
Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
>> These warnings are "maybe" which means there will be false positives and
>> different compilers will have different results. I see a number of
>> false positives with gcc 4.7.2 and I don't think we should be adding
>> spurious initialisation to make them go away.
>>
>> Why do people want to fix this particular instance?
>
> In both cases, I was not able to determine that the warnings actually
> were false positives. That depended on external circumstances outside
> the scope of the functions where the warnings were generated.
I thought it was obvious that the original code was a false positive:
dag_node_t *here = NULL; /* The directory we're currently looking at. */
const char *directory;
if (flags & open_path_node_only)
{
directory = svn_dirent_dirname(path, pool);
if (directory[1] != 0) /* root nodes are covered anyway */
SVN_ERR(dag_node_cache_get(&here, root, directory, TRUE, pool));
}
/* did the shortcut work? */
if (here)
{
path_so_far = directory;
rest = path + strlen(directory) + 1;
}
The warning is on the "path_so_far = directory" line. We only get to
that line if "here" is non-NULL, that only happens if dag_node_cache_get
is called and that only happens if "directory" is assigned.
I think it is obvious the new code is also a false positive. "here"
starts as NULL, the only path that sets "here" also sets "rest". If
"here" is still NULL "rest" gets set by a different path. The result is
"rest" is always set.
> It is always better to address the warning by changing the code, than to
> pooh-pooh it away as a "maybe". I agree, however, that a simple
> initialization is often not the correct solution. Hence my attempt at
> restructuring the code so that it does not hide potential problems by
> initializing stack variables unless it's really necessary.
We fix the real positives. We might fix the false positives if there is
a reasonable change. But this warning is documented to produce false
positives and it is producing false positives in our code (I see 28
instances and all the ones I looked at are false positives). I don't
understand why this particular instance has to be fixed or proposed for
backport to 1.8.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Branko Čibej <br...@wandisco.com>.
On 14.05.2013 13:02, Philip Martin wrote:
> Branko Čibej <br...@wandisco.com> writes:
>
>> This discussion is out of date, I committed a different fix yesterday
>> that doesn't require premature initialization, yet avoids the
>> uninitialized-read problem.
> There was no uninitialized read to avoid. My compiler now produces:
>
> ../src/subversion/libsvn_fs_fs/tree.c: In function 'open_path':
> ../src/subversion/libsvn_fs_fs/tree.c:956:13: warning: 'rest' may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> which is essentially the same as the original warning that Blair saw:
>
> ../src/subversion/libsvn_fs_fs/tree.c: In function 'open_path':
> ../src/subversion/libsvn_fs_fs/tree.c:929:27: warning: 'directory' may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> These warnings are "maybe" which means there will be false positives and
> different compilers will have different results. I see a number of
> false positives with gcc 4.7.2 and I don't think we should be adding
> spurious initialisation to make them go away.
>
> Why do people want to fix this particular instance?
In both cases, I was not able to determine that the warnings actually
were false positives. That depended on external circumstances outside
the scope of the functions where the warnings were generated.
It is always better to address the warning by changing the code, than to
pooh-pooh it away as a "maybe". I agree, however, that a simple
initialization is often not the correct solution. Hence my attempt at
restructuring the code so that it does not hide potential problems by
initializing stack variables unless it's really necessary.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
> This discussion is out of date, I committed a different fix yesterday
> that doesn't require premature initialization, yet avoids the
> uninitialized-read problem.
There was no uninitialized read to avoid. My compiler now produces:
../src/subversion/libsvn_fs_fs/tree.c: In function 'open_path':
../src/subversion/libsvn_fs_fs/tree.c:956:13: warning: 'rest' may be used uninitialized in this function [-Wmaybe-uninitialized]
which is essentially the same as the original warning that Blair saw:
../src/subversion/libsvn_fs_fs/tree.c: In function 'open_path':
../src/subversion/libsvn_fs_fs/tree.c:929:27: warning: 'directory' may be used uninitialized in this function [-Wmaybe-uninitialized]
These warnings are "maybe" which means there will be false positives and
different compilers will have different results. I see a number of
false positives with gcc 4.7.2 and I don't think we should be adding
spurious initialisation to make them go away.
Why do people want to fix this particular instance?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Branko Čibej <br...@wandisco.com>.
On 14.05.2013 08:05, Blair Zajac wrote:
> On 4/18/13 11:53 AM, Philip Martin wrote:
>> blair@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