[Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion

[Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion

[Prasanna Kumar Kalever]

using atoi() for converting string to int may be error prone in case if
string supplied in the argument is not a fold of numerical number,

This is not a bug because in the existing code,

static QemuOptsList runtime_tcp_opts = {
    .name = "gluster_tcp",
    .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
    .desc = {
        ...
        {
            .name = GLUSTER_OPT_PORT,
            .type = QEMU_OPT_NUMBER,
            .help = "port number ...",
        },
...
};

port type is QEMU_OPT_NUMBER, before we actually reaches atoi() port is already
defended by parse_option_number()

However It is a good practice to use function like parse_uint_full()
over atoi() to keep port self defended

Note: As now the port string to int conversion has its defence code set,
and also we understand that port argument is actually a string type,
in the follow up patch let's move port type from QEMU_OPT_NUMBER to
QEMU_OPT_STRING

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
---
v1: Initial patch
v2: Address comments on v1 given by Markus
---
 block/gluster.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/gluster.c b/block/gluster.c
index 01b479f..edde1ad 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -14,6 +14,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/uri.h"
 #include "qemu/error-report.h"
+#include "qemu/cutils.h"
 
 #define GLUSTER_OPT_FILENAME        "filename"
 #define GLUSTER_OPT_VOLUME          "volume"
@@ -318,6 +319,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
     int ret;
     int old_errno;
     GlusterServerList *server;
+    unsigned long long port;
 
     glfs = glfs_new(gconf->volume);
     if (!glfs) {
@@ -330,10 +332,17 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
                                    GlusterTransport_lookup[server->value->type],
                                    server->value->u.q_unix.path, 0);
         } else {
+            if ((parse_uint_full(server->value->u.tcp.port, &port, 10) < 0) ||
+                (port > 65535)) {
+                error_setg(errp, "'%s' is not a valid port number",
+                           server->value->u.tcp.port);
+                errno = EINVAL;
+                goto out;
+            }
             ret = glfs_set_volfile_server(glfs,
                                    GlusterTransport_lookup[server->value->type],
                                    server->value->u.tcp.host,
-                                   atoi(server->value->u.tcp.port));
+                                   (int)port);
         }
 
         if (ret < 0) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion

[Jeff Cody]

On Tue, Aug 09, 2016 at 02:20:09PM +0530, Prasanna Kumar Kalever wrote:
> using atoi() for converting string to int may be error prone in case if
> string supplied in the argument is not a fold of numerical number,
> 
> This is not a bug because in the existing code,
> 
> static QemuOptsList runtime_tcp_opts = {
>     .name = "gluster_tcp",
>     .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
>     .desc = {
>         ...
>         {
>             .name = GLUSTER_OPT_PORT,
>             .type = QEMU_OPT_NUMBER,
>             .help = "port number ...",
>         },
> ...
> };
> 
> port type is QEMU_OPT_NUMBER, before we actually reaches atoi() port is already
> defended by parse_option_number()
> 
> However It is a good practice to use function like parse_uint_full()
> over atoi() to keep port self defended
> 
> Note: As now the port string to int conversion has its defence code set,
> and also we understand that port argument is actually a string type,
> in the follow up patch let's move port type from QEMU_OPT_NUMBER to
> QEMU_OPT_STRING
> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> ---
> v1: Initial patch
> v2: Address comments on v1 given by Markus
> ---
>  block/gluster.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 01b479f..edde1ad 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -14,6 +14,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/uri.h"
>  #include "qemu/error-report.h"
> +#include "qemu/cutils.h"
>  
>  #define GLUSTER_OPT_FILENAME        "filename"
>  #define GLUSTER_OPT_VOLUME          "volume"
> @@ -318,6 +319,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>      int ret;
>      int old_errno;
>      GlusterServerList *server;
> +    unsigned long long port;
>  
>      glfs = glfs_new(gconf->volume);
>      if (!glfs) {
> @@ -330,10 +332,17 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>                                     GlusterTransport_lookup[server->value->type],
>                                     server->value->u.q_unix.path, 0);
>          } else {
> +            if ((parse_uint_full(server->value->u.tcp.port, &port, 10) < 0) ||
> +                (port > 65535)) {
> +                error_setg(errp, "'%s' is not a valid port number",
> +                           server->value->u.tcp.port);
> +                errno = EINVAL;

As long as we are range checking, we should probably kick back 0 as an
invalid port number as well, right?

> +                goto out;
> +            }
>              ret = glfs_set_volfile_server(glfs,
>                                     GlusterTransport_lookup[server->value->type],
>                                     server->value->u.tcp.host,
> -                                   atoi(server->value->u.tcp.port));
> +                                   (int)port);
>          }
>  
>          if (ret < 0) {
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Tue, Aug 09, 2016 at 02:20:09PM +0530, Prasanna Kumar Kalever wrote:
>> using atoi() for converting string to int may be error prone in case if
>> string supplied in the argument is not a fold of numerical number,
>> 
>> This is not a bug because in the existing code,
>> 
>> static QemuOptsList runtime_tcp_opts = {
>>     .name = "gluster_tcp",
>>     .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
>>     .desc = {
>>         ...
>>         {
>>             .name = GLUSTER_OPT_PORT,
>>             .type = QEMU_OPT_NUMBER,
>>             .help = "port number ...",
>>         },
>> ...
>> };
>> 
>> port type is QEMU_OPT_NUMBER, before we actually reaches atoi() port is already
>> defended by parse_option_number()
>> 
>> However It is a good practice to use function like parse_uint_full()
>> over atoi() to keep port self defended
>> 
>> Note: As now the port string to int conversion has its defence code set,
>> and also we understand that port argument is actually a string type,
>> in the follow up patch let's move port type from QEMU_OPT_NUMBER to
>> QEMU_OPT_STRING
>> 
>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
>> ---
>> v1: Initial patch
>> v2: Address comments on v1 given by Markus
>> ---
>>  block/gluster.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 01b479f..edde1ad 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -14,6 +14,7 @@
>>  #include "qapi/qmp/qerror.h"
>>  #include "qemu/uri.h"
>>  #include "qemu/error-report.h"
>> +#include "qemu/cutils.h"
>>  
>>  #define GLUSTER_OPT_FILENAME        "filename"
>>  #define GLUSTER_OPT_VOLUME          "volume"
>> @@ -318,6 +319,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>      int ret;
>>      int old_errno;
>>      GlusterServerList *server;
>> +    unsigned long long port;
>>  
>>      glfs = glfs_new(gconf->volume);
>>      if (!glfs) {
>> @@ -330,10 +332,17 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>                                     GlusterTransport_lookup[server->value->type],
>>                                     server->value->u.q_unix.path, 0);
>>          } else {
>> +            if ((parse_uint_full(server->value->u.tcp.port, &port, 10) < 0) ||
>> +                (port > 65535)) {

Two pairs of superfluous parenthesis.  Better:

                if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
                    port > 65535) {

or, if you prefer to break before the operator (I do):

                if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0
                    || port > 65535) {

or, if you prefer to keep side effects out of complex conditionals:

                ret = parse_uint_full(server->value->u.tcp.port, &port, 10);
                if (ret < 0 || port > 65535) {

Maintainer's choice.  Perhaps Jeff can touch this up on commit.

>> +                error_setg(errp, "'%s' is not a valid port number",
>> +                           server->value->u.tcp.port);
>> +                errno = EINVAL;
>
> As long as we are range checking, we should probably kick back 0 as an
> invalid port number as well, right?

Port 0 is an oddity.  On the one hand, bind() interprets it as "pick an
ephemeral port for me".  That makes port 0 unusable with the classical
sockets interface.  On the other hand, IP doesn't treat port 0
specially.  You *can* send and receive port 0 packets with a raw socket
interface.

Anyway, I wouldn't bother rejecting port 0 here.  Just let the system do
its "pick an ephemeral port" magic.

>> +                goto out;
>> +            }
>>              ret = glfs_set_volfile_server(glfs,
>>                                     GlusterTransport_lookup[server->value->type],
>>                                     server->value->u.tcp.host,
>> -                                   atoi(server->value->u.tcp.port));
>> +                                   (int)port);
>>          }
>>  
>>          if (ret < 0) {
>> -- 
>> 2.7.4
>> 

Preferrably with the parenthesis cleaned up:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion

[Jeff Cody]

On Wed, Aug 10, 2016 at 09:13:48AM +0200, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> writes:
> 
> > On Tue, Aug 09, 2016 at 02:20:09PM +0530, Prasanna Kumar Kalever wrote:
> >> using atoi() for converting string to int may be error prone in case if
> >> string supplied in the argument is not a fold of numerical number,
> >> 
> >> This is not a bug because in the existing code,
> >> 
> >> static QemuOptsList runtime_tcp_opts = {
> >>     .name = "gluster_tcp",
> >>     .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
> >>     .desc = {
> >>         ...
> >>         {
> >>             .name = GLUSTER_OPT_PORT,
> >>             .type = QEMU_OPT_NUMBER,
> >>             .help = "port number ...",
> >>         },
> >> ...
> >> };
> >> 
> >> port type is QEMU_OPT_NUMBER, before we actually reaches atoi() port is already
> >> defended by parse_option_number()
> >> 
> >> However It is a good practice to use function like parse_uint_full()
> >> over atoi() to keep port self defended
> >> 
> >> Note: As now the port string to int conversion has its defence code set,
> >> and also we understand that port argument is actually a string type,
> >> in the follow up patch let's move port type from QEMU_OPT_NUMBER to
> >> QEMU_OPT_STRING
> >> 
> >> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> >> ---
> >> v1: Initial patch
> >> v2: Address comments on v1 given by Markus
> >> ---
> >>  block/gluster.c | 11 ++++++++++-
> >>  1 file changed, 10 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/block/gluster.c b/block/gluster.c
> >> index 01b479f..edde1ad 100644
> >> --- a/block/gluster.c
> >> +++ b/block/gluster.c
> >> @@ -14,6 +14,7 @@
> >>  #include "qapi/qmp/qerror.h"
> >>  #include "qemu/uri.h"
> >>  #include "qemu/error-report.h"
> >> +#include "qemu/cutils.h"
> >>  
> >>  #define GLUSTER_OPT_FILENAME        "filename"
> >>  #define GLUSTER_OPT_VOLUME          "volume"
> >> @@ -318,6 +319,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
> >>      int ret;
> >>      int old_errno;
> >>      GlusterServerList *server;
> >> +    unsigned long long port;
> >>  
> >>      glfs = glfs_new(gconf->volume);
> >>      if (!glfs) {
> >> @@ -330,10 +332,17 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
> >>                                     GlusterTransport_lookup[server->value->type],
> >>                                     server->value->u.q_unix.path, 0);
> >>          } else {
> >> +            if ((parse_uint_full(server->value->u.tcp.port, &port, 10) < 0) ||
> >> +                (port > 65535)) {
> 
> Two pairs of superfluous parenthesis.  Better:
> 
>                 if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
>                     port > 65535) {
> 
> or, if you prefer to break before the operator (I do):
> 
>                 if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0
>                     || port > 65535) {
> 
> or, if you prefer to keep side effects out of complex conditionals:
> 
>                 ret = parse_uint_full(server->value->u.tcp.port, &port, 10);
>                 if (ret < 0 || port > 65535) {
> 
> Maintainer's choice.  Perhaps Jeff can touch this up on commit.
> 
> >> +                error_setg(errp, "'%s' is not a valid port number",
> >> +                           server->value->u.tcp.port);
> >> +                errno = EINVAL;
> >
> > As long as we are range checking, we should probably kick back 0 as an
> > invalid port number as well, right?
> 
> Port 0 is an oddity.  On the one hand, bind() interprets it as "pick an
> ephemeral port for me".  That makes port 0 unusable with the classical
> sockets interface.  On the other hand, IP doesn't treat port 0
> specially.  You *can* send and receive port 0 packets with a raw socket
> interface.
> 
> Anyway, I wouldn't bother rejecting port 0 here.  Just let the system do
> its "pick an ephemeral port" magic.
> 
> >> +                goto out;
> >> +            }
> >>              ret = glfs_set_volfile_server(glfs,
> >>                                     GlusterTransport_lookup[server->value->type],
> >>                                     server->value->u.tcp.host,
> >> -                                   atoi(server->value->u.tcp.port));
> >> +                                   (int)port);
> >>          }
> >>  
> >>          if (ret < 0) {
> >> -- 
> >> 2.7.4
> >> 
> 
> Preferrably with the parenthesis cleaned up:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

[Removed spurious parenthesis]

-Jeff