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

[Liam Merwick]

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.
> >
> > Signed-off-by: Liam Merwick <address@hidden>
> > Reviewed-by: Darren Kenny <address@hidden>
> > Reviewed-by: Mark Kanda <address@hidden>
> > ---
> >   io/channel-command.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/io/channel-command.c b/io/channel-command.c
> > index 3e7eb17eff54..38deb687da21 100644
> > --- a/io/channel-command.c
> > +++ b/io/channel-command.c
> > @@ -59,10 +59,10 @@ qio_channel_command_new_spawn(const char *const 
> > argv[],
> >       flags = flags & O_ACCMODE;
> > -    if (flags == O_RDONLY) {
> > +    if ((flags & O_RDONLY) == O_RDONLY) {
>
> NACK.  O_RDONLY and O_WRONLY are subsets of O_ACCMODE, which we already 
> masked out above.
>
> On some systems, we have:
> O_RDONLY == 0
> O_WRONLY == 1
> O_RDWR == 2
>
> On other systems, we have:
> O_RDONLY == 1
> O_WRONLY == 2
> O_RDWR == 3
>
> Either way, if the user passed in O_RDWR, (flags & O_RDONLY) == O_RDONLY 
> returns true, which is wrong.
>
> O_ACCMODE was historically 0x3, although now that POSIX has O_EXEC and 
> O_SEARCH (which can be the same bit pattern), some systems now make 
> O_ACCMODE occupy 3 bits instead of 2.

Thanks for catching that and for the explanation.

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.

--- 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) {
         stdoutnull = true;
     }

Regards,
Liam