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.merw...@oracle.com>
Reviewed-by: Darren Kenny <darren.ke...@oracle.com>
Reviewed-by: Mark Kanda <mark.ka...@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
