Re: Warning in spawn-pipe.c (create_pipe)

[Tim Rühsen]

On 12/13/2017 11:24 PM, Bruno Haible wrote:
> Hi Tim,
> 
>> clang's warning:
>>
>> spawn-pipe.c:364:34: warning: variable 'child' may be uninitialized when
>> used here [-Wconditional-uninitialized]
>>       register_slave_subprocess (child);
>>                                  ^~~~~
> 
> I agree with your analysis that without massive inlining or other propagation,
> the compiler cannot know whether the previous call to
>   posix_spawnp (&child, ...)
> will have initialized the variable 'child'.
> 
> So this class of warning is of the category "don't turn it on by default
> if you want to have a warning-free build; but do turn it on occasionally
> if you find that it detects real bugs in your code".
> 
>> That's why I vote for initializing 'child' to 0 as suggested below. It
>> seems to be good programming practice.
> 
> No. Not for me.
> 
> 1) It is not a goal to have absolutely no warnings with GCC or
>    with clang. It is perfectly OK, IMO, if a compilation with "gcc -Wall"
>    shows, say, 5 warnings in 10 files. The maintainer will get used to
>    these warnings and see new warnings when they arise.

That is really bad and it makes me sad. You are saying it's a good thing
to get used to a bad situation. I hope you don't mean it.


> 2) For the problem of uninitialized variables that lead to undefined
>    behaviour, I don't see a GCC option that would warn about them [1].
>    Instead, 'valgrind' is the ultimate tool for detecting these kinds
>    of problems.
>    So if someone has a habit of looking only at GCC warnings, they should
>    change their habit and also use valgrind once in a while.

Valgrind is not the ultimate tool. It is just one tool (or one line of
defense) out of several. One is the coder itself, another one is the
compiler with it's code analysis. Before I start testing with valgrind,
most all of the compiler warnings have to be dealt with.

Valgrind is slow, especially if you are testing applications (think of
(then)thousands of valgrind invocations). And the quality of Valgrind
depends on the code path coverage - that is not the same as code
coverage. To get the test data to cover most code paths you need a
fuzzer, at least for the more complex functions / functionality. Writing
good fuzzers takes time and sometimes need a lot of human time for
tuning. If you have done that you are ready to use Valgrind in a pretty
reliable way for regression testing, at least you are beyond
'random|human' code path coverage.

BTW, the recent findings in glob() were random findings when fuzzing
wget2 (which uses glob() at some point). Just to underline the power of
fuzzing in comparison with human generated test data - since there are
tests in glibc/gnulib for glob() that are also used with Valgrind,
aren't there ?

Aiming at fuzzing glob() to get full code path coverage likely reveals
more issues. I wish I had the time to work on fuzzing gnulib... but
currently I don't see any time window for the future.

> 3) Adding the 'child = 0' initialization has the effect that it will
>    silence both the clang warning and valgrind. However, if there is
>    a bug in the code that forgets to assign a value to the variable, the
>    bug will not be detected. In other words, the bug will still be present
>    but will be deterministic. => Such an initialization is a plus in
>    some situations (when debugging with gdb) and a minus in others.

Catched automatically by (description above).


With Best Regards, Tim

signature.asc
Description: OpenPGP digital signature