* [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion
@ 2016-08-09 8:50 Prasanna Kumar Kalever
2016-08-10 1:05 ` Jeff Cody
0 siblings, 1 reply; 4+ messages in thread
From: Prasanna Kumar Kalever @ 2016-08-09 8:50 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, eblake, jcody, areis, vbellur, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion
2016-08-09 8:50 [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion Prasanna Kumar Kalever
@ 2016-08-10 1:05 ` Jeff Cody
2016-08-10 7:13 ` Markus Armbruster
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Cody @ 2016-08-10 1:05 UTC (permalink / raw)
To: Prasanna Kumar Kalever; +Cc: qemu-devel, armbru, eblake, areis, vbellur
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion
2016-08-10 1:05 ` Jeff Cody
@ 2016-08-10 7:13 ` Markus Armbruster
2016-10-29 12:20 ` Jeff Cody
0 siblings, 1 reply; 4+ messages in thread
From: Markus Armbruster @ 2016-08-10 7:13 UTC (permalink / raw)
To: Jeff Cody; +Cc: Prasanna Kumar Kalever, vbellur, areis, qemu-devel
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion
2016-08-10 7:13 ` Markus Armbruster
@ 2016-10-29 12:20 ` Jeff Cody
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Cody @ 2016-10-29 12:20 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Prasanna Kumar Kalever, vbellur, qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-10-29 12:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-09 8:50 [Qemu-devel] [PATCH v2 1/1] block/gluster: improve defense over string to int conversion Prasanna Kumar Kalever
2016-08-10 1:05 ` Jeff Cody
2016-08-10 7:13 ` Markus Armbruster
2016-10-29 12:20 ` Jeff Cody
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).