[Qemu-block] Re [PATCH] qemu-pr-helper: check the return value of fcntl in do_pr_out

[lizhengui]

The fcntl call fails in the actual scene and it is really hard to happen. But 
according to a good coding style, I think there should be a error handling for 
a system call.

+ if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) {

The flags is a int type. According to strict programming specifications, it 
should be converted to unsigned type before doing bitwise operator. I am doing 
this just to avoid codex warnings.
If you think it is not necessary to do so,  I can remove it.


-----邮件原件-----
发件人: Eric Blake [mailto:address@hidden 
发送时间: 2019年3月22日 10:01
收件人: lizhengui; Paolo Bonzini; address@hidden; address@hidden; address@hidden
抄送: wangjie (P); address@hidden; address@hidden; Fangyi (C)
主题: Re: [Qemu-block] [PATCH] qemu-pr-helper: check the return value of fcntl in 
do_pr_out

[top-posting is harder to read on technical lists; I'm reordering your message 
before replying inline]

> On 20/03/19 15:07, Zhengui li wrote:
>> The function fcntl maybe return -1, which is not a unsigned type.
>> Unsigned type or Negative values should not do bitwise operator with 
>> O_ACCMODE.
> 
> Did you actually find a case in which the fcntl can fail?

On 3/21/19 8:50 PM, lizhengui wrote:

> If the fd is invalid or interrupted by signal.

If the fd is invalid, we have a coding bug on our hand - we should not be 
calling do_pr_out with an invalid fd. Do you have a backtrace where that 
actually happened?

As for being interrupted by a signal, that's not possible. fcntl() can only be 
interrupted by signal forF_SETLKW, F_OFD_SETLKW, F_GETLK, F_SETLK, F_OFD_GETLK, 
or F_OFD_SETLK.

I agree that your fix avoids a bug if it can actually happen - but I also want 
to know if it happened in practice or whether it is just plugging a theoretical 
hole (it may determine whether your patch must go into 4.0, or can slip to 4.1).

>> -    if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) {
>> +    flags = fcntl(fd, F_GETFL);
>> +    if (flags < 0) {
>> +        return -1;
>> +    }

This addition is fine.

>> +
>> +    if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) {

This cast is not. You already guaranteed that flags is non-negative by the code 
added above, and therefore the bitwise-and on the signed type is well-defined, 
without the need to muddy things up with a cast.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org