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

[Liam Merwick]

On 31/08/18 16:50, Eric Blake wrote:
> On 08/31/2018 10:36 AM, Liam Merwick wrote:
> > On 30/08/2018 17:18, Eric Blake wrote:
> > > On 08/30/2018 10:47 AM, Liam Merwick wrote:
> > > > Incorrect checking of flags could result in uninitialized
> > > > file descriptor being used.
>
> > 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?  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
              and flags != 1 at line 65
              and stdinnull is false at line 69
              and stdoutnull is false at line 69.


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)

Regards,
Liam

> > --- a/io/channel-command.c
> > +++ b/io/channel-command.c
> > @@ -61,8 +61,7 @@ qio_channel_command_new_spawn(const char *const 
> > argv[],
> >
> >       if (flags == O_RDONLY) {
> >           stdinnull = true;
> > -    }
> > -    if (flags == O_WRONLY) {
> > +    } else if (flags == O_WRONLY) {
>
> But this sort of change is acceptable, especially if it does shut up 
> the analyzer, and done with a commit message mentioning that it is not 
> a semantic change but does make it easier to use the static checker to 
> find real problems by getting rid of noise.
>
> >           stdoutnull = true;
> >       }
> >
> > Regards,
> > Liam