[v1] qemu-pr-helper: check the return value of fcntl in do_pr_out

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

Posted by Zhengui li 6 years, 10 months ago

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(-)

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

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

Posted by Stefano Garzarella 6 years, 10 months ago

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
> 
> 
>

[Qemu-devel] 答复: [PATCH] qemu-pr-helper: check the return value of fcntl in do_pr_out

Posted by lizhengui 6 years, 10 months ago

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
> 
> 
>

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

Posted by Paolo Bonzini 6 years, 10 months ago

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;
>      }
>

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

Posted by lizhengui 6 years, 10 months ago

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;
>      }
>

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

Posted by Eric Blake 6 years, 10 months ago

[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

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

Posted by lizhengui 6 years, 10 months ago

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