9p: Convert use of atoi to qemu_strtoi to allow error checking

[Qemu-devel] [PATCH v2] 9p: Convert use of atoi to qemu_strtoi to allow error checking

Posted by Nia Alarie 6 years, 1 month ago

Signed-off-by: Nia Alarie <nia.alarie@gmail.com>
---
 hw/9pfs/9p.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 48fa48e720..254385dfa4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -15,6 +15,7 @@
 #include <glib/gprintf.h>
 #include "hw/virtio/virtio.h"
 #include "qapi/error.h"
+#include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qemu/iov.h"
 #include "qemu/sockets.h"
@@ -2213,8 +2214,14 @@ static void coroutine_fn v9fs_create(void *opaque)
         }
         v9fs_path_copy(&fidp->path, &path);
     } else if (perm & P9_STAT_MODE_LINK) {
-        int32_t ofid = atoi(extension.data);
-        V9fsFidState *ofidp = get_fid(pdu, ofid);
+        int ofid;
+        V9fsFidState *ofidp;
+
+        if (qemu_strtoi(extension.data, NULL, 10, &ofid)) {
+            err = -EINVAL;
+            goto out;
+        }
+        ofidp = get_fid(pdu, (int32_t)ofid);
         if (ofidp == NULL) {
             err = -EINVAL;
             goto out;
-- 
2.16.2

Re: [Qemu-devel] [PATCH v2] 9p: Convert use of atoi to qemu_strtoi to allow error checking

Posted by Eric Blake 6 years, 1 month ago

On 03/12/2018 10:33 AM, Nia Alarie wrote:
> Signed-off-by: Nia Alarie <nia.alarie@gmail.com>
> ---
>   hw/9pfs/9p.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)

Helping out our CI tools:
Based-on: <20180312124939.20562-1-berrange@redhat.com>

> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 48fa48e720..254385dfa4 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -15,6 +15,7 @@
>   #include <glib/gprintf.h>
>   #include "hw/virtio/virtio.h"
>   #include "qapi/error.h"
> +#include "qemu/cutils.h"
>   #include "qemu/error-report.h"
>   #include "qemu/iov.h"
>   #include "qemu/sockets.h"
> @@ -2213,8 +2214,14 @@ static void coroutine_fn v9fs_create(void *opaque)
>           }
>           v9fs_path_copy(&fidp->path, &path);
>       } else if (perm & P9_STAT_MODE_LINK) {
> -        int32_t ofid = atoi(extension.data);
> -        V9fsFidState *ofidp = get_fid(pdu, ofid);
> +        int ofid;

'unsigned int' and...

> +        V9fsFidState *ofidp;
> +
> +        if (qemu_strtoi(extension.data, NULL, 10, &ofid)) {

qemu_strtoui() might be smarter, per Greg's comments on v1.

> +            err = -EINVAL;
> +            goto out;
> +        }
> +        ofidp = get_fid(pdu, (int32_t)ofid);

This cast is spurious.

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

Re: [Qemu-devel] [PATCH v2] 9p: Convert use of atoi to qemu_strtoi to allow error checking

Posted by nee 6 years, 1 month ago

On Mon, Mar 12, 2018 at 3:43 PM, Eric Blake <eblake@redhat.com> wrote:
> On 03/12/2018 10:33 AM, Nia Alarie wrote:
>>
>> Signed-off-by: Nia Alarie <nia.alarie@gmail.com>
>> ---
>>   hw/9pfs/9p.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>
>
> Helping out our CI tools:
> Based-on: <20180312124939.20562-1-berrange@redhat.com>
>
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 48fa48e720..254385dfa4 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -15,6 +15,7 @@
>>   #include <glib/gprintf.h>
>>   #include "hw/virtio/virtio.h"
>>   #include "qapi/error.h"
>> +#include "qemu/cutils.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/iov.h"
>>   #include "qemu/sockets.h"
>> @@ -2213,8 +2214,14 @@ static void coroutine_fn v9fs_create(void *opaque)
>>           }
>>           v9fs_path_copy(&fidp->path, &path);
>>       } else if (perm & P9_STAT_MODE_LINK) {
>> -        int32_t ofid = atoi(extension.data);
>> -        V9fsFidState *ofidp = get_fid(pdu, ofid);
>> +        int ofid;
>
>
> 'unsigned int' and...
>
>> +        V9fsFidState *ofidp;
>> +
>> +        if (qemu_strtoi(extension.data, NULL, 10, &ofid)) {
>
>
> qemu_strtoui() might be smarter, per Greg's comments on v1.
>
>> +            err = -EINVAL;
>> +            goto out;
>> +        }
>> +        ofidp = get_fid(pdu, (int32_t)ofid);
>
>
> This cast is spurious.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

I did this because get_fid() takes an int32_t, not an unsigned int.
The struct V9fsFidState also uses an int32_t for its `fid` member. Do
you want me to change all these types, or just the function being used
here?

Re: [Qemu-devel] [PATCH v2] 9p: Convert use of atoi to qemu_strtoi to allow error checking

Posted by Eric Blake 6 years, 1 month ago

On 03/12/2018 10:50 AM, nee wrote:

>>>        } else if (perm & P9_STAT_MODE_LINK) {
>>> -        int32_t ofid = atoi(extension.data);
>>> -        V9fsFidState *ofidp = get_fid(pdu, ofid);
>>> +        int ofid;
>>
>>
>> 'unsigned int' and...
>>
>>> +        V9fsFidState *ofidp;
>>> +
>>> +        if (qemu_strtoi(extension.data, NULL, 10, &ofid)) {
>>
>>
>> qemu_strtoui() might be smarter, per Greg's comments on v1.
>>
>>> +            err = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +        ofidp = get_fid(pdu, (int32_t)ofid);
>>
>>
>> This cast is spurious.
>>
>> --
>> Eric Blake, Principal Software Engineer
>> Red Hat, Inc.           +1-919-301-3266
>> Virtualization:  qemu.org | libvirt.org
> 
> I did this because get_fid() takes an int32_t, not an unsigned int.
> The struct V9fsFidState also uses an int32_t for its `fid` member. Do
> you want me to change all these types, or just the function being used
> here?

I'll let Greg answer; he's more familiar with the 9p code (I was just 
commenting based on his initial answer to v1).

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

Re: [Qemu-devel] [PATCH v2] 9p: Convert use of atoi to qemu_strtoi to allow error checking

Posted by Greg Kurz 6 years, 1 month ago

On Mon, 12 Mar 2018 11:00:39 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 03/12/2018 10:50 AM, nee wrote:
> 
> >>>        } else if (perm & P9_STAT_MODE_LINK) {
> >>> -        int32_t ofid = atoi(extension.data);
> >>> -        V9fsFidState *ofidp = get_fid(pdu, ofid);
> >>> +        int ofid;  
> >>
> >>
> >> 'unsigned int' and...
> >>  
> >>> +        V9fsFidState *ofidp;
> >>> +
> >>> +        if (qemu_strtoi(extension.data, NULL, 10, &ofid)) {  
> >>
> >>
> >> qemu_strtoui() might be smarter, per Greg's comments on v1.
> >>  
> >>> +            err = -EINVAL;
> >>> +            goto out;
> >>> +        }
> >>> +        ofidp = get_fid(pdu, (int32_t)ofid);  
> >>
> >>
> >> This cast is spurious.
> >>
> >> --
> >> Eric Blake, Principal Software Engineer
> >> Red Hat, Inc.           +1-919-301-3266
> >> Virtualization:  qemu.org | libvirt.org  
> > 
> > I did this because get_fid() takes an int32_t, not an unsigned int.
> > The struct V9fsFidState also uses an int32_t for its `fid` member. Do
> > you want me to change all these types, or just the function being used
> > here?  
> 
> I'll let Greg answer; he's more familiar with the 9p code (I was just 
> commenting based on his initial answer to v1).
> 

Yeah... as I was saying, fids are 32-bit unsigned per spec but the code is
using int32_t indeed. This is incorrect and it should be fixed.