[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()
Date: Fri, 31 Aug 2018 11:45:10 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

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?


  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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]