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

[Markus Armbruster]

Cc: Eric, because there's a relation to QAPI even though the patch
doesn't touch the schema.

Prasanna Kumar Kalever <address@hidden> writes:

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

Should we use QEMU_OPT_STRING here, for the same reasons we use 'str' in
the QAPI schema?

> However It is a good practice to use function like parse_uint_full()
> over atoi() to keep port self defended
>
> Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> ---
>  block/gluster.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index 01b479f..e2aa0f3 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;
> +    long long unsigned port;

unsigned long long, please.

>  
>      glfs = glfs_new(gconf->volume);
>      if (!glfs) {
> @@ -330,10 +332,16 @@ 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, 0) < 0) {

Should we require decimal by passing 10 rather than 0?  Port numbers are
commonly decimal.  For what it's worth, JSON numbers are always decimal.

> +                error_setg(errp, "can't convert port to a number: %s",
> +                           server->value->u.tcp.port);

Suggest "'%s' is not a valid port number".

> +                errno = EINVAL;
> +                goto out;
> +            }

Missing: a range check.  No need to get fancy, something like this might
do:

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

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