[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
signature.asc
Description: OpenPGP digital signature
