Re: [Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

[Eric Blake]

On 08/31/2018 11:19 AM, Liam Merwick wrote:

> > > Looking at it again, the very minor optimisation of converting the 
> > > 2nd 'if' to an 'else if' has the useful side-effect of appeasing the 
> > > static analysis tool.
> >
> > I never figured out what the tool precisely thought was wrong in the 
> > first place. Can you paste the output of the tool to see exactly what 
> > it analyzed as the potential flaw?

Thanks.

> >   Perhaps the analyzer was trying to 
> > see what would happen if a caller submitting the fourth value (3 on 
> > systems where O_RDONLY is 0, 0 on systems where O_RDONLY is 1) and the 
> > code not behaving in that setup, even though we know that all callers 
> > only submit the three valid values and never the fourth invalid 
> > value?  Or maybe it's a weakness where we have made dependent 
> > assumptions but in independent branches, in such a way that we know it 
> > will never fail but the analyzer doesn't?
>
> The specific error it reported was
>
> Error: File Invalid
>     File Descriptor not Initialized [file-desc-not-init]:
>        The value <unknown> is not initialized as a file descriptor.
>          at line 91 of io/channel-command.c in function 
> 'qio_channel_command_new_spawn'.
>            resource  not initialized when flags != 0 at line 62

flags is not O_RDONLY,

>                and flags != 1 at line 65

flags is not O_WRONLY,

>                and stdinnull is false at line 69
>                and stdoutnull is false at line 69.

so yes, both stdinnull and stdoutnull will be false at this point. 
Based on our earlier masking, that means flags is either O_RDWR (2) or 
bogus (3).  The analyzer is then complaining about:

        dup2(stdinnull ? devnull : stdinfd[0], STDIN_FILENO);

So either it is complaining about devnull being uninitialized (which it 
is when flags is O_RDWR - but in that situation, we know stdinnull is 
false which also implies that the left half of ?: is unreachable and 
therefore shouldn't matter) or it is complaining about stdinfd[0] being 
uninitialized (but we can't reach here without having gone through the 
earlier (!stdinnull && pipe(stdinfd)), and again, given that stdinnull 
is false, that is initialized).  The fact that it lists '<unknown>' 
rather than the actual (sub-)expression that it claims is uninitialized 
doesn't help.  So yeah, I'm seriously confused as to why this false 
positive is being reported, or why the change from 'if/if' to 'if/else 
if' shuts it up.

> I've been staring at the code and can see no reason why it isn't a false 
> positive (and I'll let the tool authors know)

Thanks for the followup.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org