diff mbox series

[7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

Message ID 1535644031-848-8-git-send-email-Liam.Merwick@oracle.com (mailing list archive)
State New, archived
Headers show
Series off-by-one and NULL pointer accesses detected by static analysis | expand

Commit Message

Liam Merwick Aug. 30, 2018, 3:47 p.m. UTC 
Incorrect checking of flags could result in uninitialized
file descriptor being used.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
---
 io/channel-command.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eric Blake Aug. 30, 2018, 4:18 p.m. UTC | #1 
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 <Liam.Merwick@oracle.com>
> Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
> Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
> ---
>   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.
Liam Merwick Aug. 31, 2018, 3:36 p.m. UTC | #2 
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 <Liam.Merwick@oracle.com>
>> Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
>> Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
>> ---
>>   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
Eric Blake Aug. 31, 2018, 3:50 p.m. UTC | #3 
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?

> 
> --- 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
>
Liam Merwick Aug. 31, 2018, 4:19 p.m. UTC | #4 
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
>>
>
Eric Blake Aug. 31, 2018, 4:45 p.m. UTC | #5 
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.
diff mbox series

Patch

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) {
         stdinnull = true;
     }
-    if (flags == O_WRONLY) {
+    if ((flags & O_WRONLY) == O_WRONLY) {
         stdoutnull = true;
     }