| Message ID | 1553090847-11300-1-git-send-email-lizhengui@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-pr-helper: check the return value of fcntl in do_pr_out | expand |
Hi Zhengui, thanks for this patch! For the next patches, please add a version tag when resending. "git format-patch" and "git send-email" both understand the "-v2" option. On Wed, Mar 20, 2019 at 10:07:27PM +0800, 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. > > Signed-off-by: Zhengui li <lizhengui@huawei.com> > --- > scsi/qemu-pr-helper.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c > index e7af637..fcbe4d9 100644 > --- a/scsi/qemu-pr-helper.c > +++ b/scsi/qemu-pr-helper.c > @@ -551,8 +551,14 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, > const uint8_t *param, int sz) > { > int resp_sz; > + int flags; > > - if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) { > + flags = fcntl(fd, F_GETFL); > + if (flags < 0) { > + return -1; > + } > + > + if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) { > scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE)); > return CHECK_CONDITION; > } > -- > 2.7.2.windows.1 > > >
Ok！ -----邮件原件----- 发件人: Stefano Garzarella [mailto:sgarzare@redhat.com] 发送时间: 2019年3月21日 17:28 收件人: lizhengui 抄送: pbonzini@redhat.com; stefanha@redhat.com; mreitz@redhat.com; kwolf@redhat.com; wangjie (P); qemu-devel@nongnu.org; qemu-block@nongnu.org; Fangyi (C) 主题: Re: [Qemu-devel] [PATCH] qemu-pr-helper: check the return value of fcntl in do_pr_out Hi Zhengui, thanks for this patch! For the next patches, please add a version tag when resending. "git format-patch" and "git send-email" both understand the "-v2" option. On Wed, Mar 20, 2019 at 10:07:27PM +0800, 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. > > Signed-off-by: Zhengui li <lizhengui@huawei.com> > --- > scsi/qemu-pr-helper.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c > index e7af637..fcbe4d9 100644 > --- a/scsi/qemu-pr-helper.c > +++ b/scsi/qemu-pr-helper.c > @@ -551,8 +551,14 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, > const uint8_t *param, int sz) > { > int resp_sz; > + int flags; > > - if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) { > + flags = fcntl(fd, F_GETFL); > + if (flags < 0) { > + return -1; > + } > + > + if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) { > scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE)); > return CHECK_CONDITION; > } > -- > 2.7.2.windows.1 > > >
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? Paolo > Signed-off-by: Zhengui li <lizhengui@huawei.com> > --- > scsi/qemu-pr-helper.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c > index e7af637..fcbe4d9 100644 > --- a/scsi/qemu-pr-helper.c > +++ b/scsi/qemu-pr-helper.c > @@ -551,8 +551,14 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, > const uint8_t *param, int sz) > { > int resp_sz; > + int flags; > > - if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) { > + flags = fcntl(fd, F_GETFL); > + if (flags < 0) { > + return -1; > + } > + > + if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) { > scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE)); > return CHECK_CONDITION; > } >
If the fd is invalid or interrupted by signal. -----邮件原件----- 发件人: Paolo Bonzini [mailto:pbonzini@redhat.com] 发送时间: 2019年3月21日 18:38 收件人: lizhengui; stefanha@redhat.com; mreitz@redhat.com; kwolf@redhat.com 抄送: qemu-block@nongnu.org; qemu-devel@nongnu.org; Fangyi (C); wangjie (P) 主题: Re: [PATCH] qemu-pr-helper: check the return value of fcntl in do_pr_out 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? Paolo > Signed-off-by: Zhengui li <lizhengui@huawei.com> > --- > scsi/qemu-pr-helper.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c index > e7af637..fcbe4d9 100644 > --- a/scsi/qemu-pr-helper.c > +++ b/scsi/qemu-pr-helper.c > @@ -551,8 +551,14 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, > const uint8_t *param, int sz) { > int resp_sz; > + int flags; > > - if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) { > + flags = fcntl(fd, F_GETFL); > + if (flags < 0) { > + return -1; > + } > + > + if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) { > scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE)); > return CHECK_CONDITION; > } >
[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.
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:eblake@redhat.com]
发送时间: 2019年3月22日 10:01
收件人: lizhengui; Paolo Bonzini; stefanha@redhat.com; mreitz@redhat.com; kwolf@redhat.com
抄送: wangjie (P); qemu-devel@nongnu.org; qemu-block@nongnu.org; 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
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c index e7af637..fcbe4d9 100644 --- a/scsi/qemu-pr-helper.c +++ b/scsi/qemu-pr-helper.c @@ -551,8 +551,14 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, const uint8_t *param, int sz) { int resp_sz; + int flags; - if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) { + flags = fcntl(fd, F_GETFL); + if (flags < 0) { + return -1; + } + + if (((unsigned int) flags & O_ACCMODE) == O_RDONLY) { scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE)); return CHECK_CONDITION; }
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. Signed-off-by: Zhengui li <lizhengui@huawei.com> --- scsi/qemu-pr-helper.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)