* [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()
@ 2019-05-03 17:06 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2019-05-03 17:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang, Peter Maydell
net_client_init() uses a variable length array to store the prefix
of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
is 'fec0::0').
Since the IPv6 prefix can be at most as long as an IPv6 address,
we can use an array with fixed size equals to INET6_ADDRSTRLEN.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
net/net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/net.c b/net/net.c
index f3a3c5444c..2e5f27e121 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1118,7 +1118,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
if (ip6_net) {
- char buf[strlen(ip6_net) + 1];
+ char buf[INET6_ADDRSTRLEN];
if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
/* Default 64bit prefix length. */
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()
@ 2019-05-03 17:06 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2019-05-03 17:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang
net_client_init() uses a variable length array to store the prefix
of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
is 'fec0::0').
Since the IPv6 prefix can be at most as long as an IPv6 address,
we can use an array with fixed size equals to INET6_ADDRSTRLEN.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
net/net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/net.c b/net/net.c
index f3a3c5444c..2e5f27e121 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1118,7 +1118,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
if (ip6_net) {
- char buf[strlen(ip6_net) + 1];
+ char buf[INET6_ADDRSTRLEN];
if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
/* Default 64bit prefix length. */
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()
2019-05-03 17:06 ` Stefano Garzarella
(?)
@ 2019-05-06 13:23 ` Markus Armbruster
2019-05-06 17:42 ` Stefano Garzarella
-1 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2019-05-06 13:23 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: Peter Maydell, Jason Wang, qemu-devel
Stefano Garzarella <sgarzare@redhat.com> writes:
> net_client_init() uses a variable length array to store the prefix
> of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> is 'fec0::0').
> Since the IPv6 prefix can be at most as long as an IPv6 address,
> we can use an array with fixed size equals to INET6_ADDRSTRLEN.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> net/net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/net.c b/net/net.c
> index f3a3c5444c..2e5f27e121 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1118,7 +1118,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
> const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
>
> if (ip6_net) {
> - char buf[strlen(ip6_net) + 1];
> + char buf[INET6_ADDRSTRLEN];
>
> if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
> /* Default 64bit prefix length. */
Hmm.
Parameter "ipv6-net" is of the form ADDRESS[/PREFIX-SIZE]. If
/PREFIX-SIZE is present, get_str_sep() copies the ADDRESS part to buf[].
However, nothing stops the user from passing in an ADDRESS longer than
INET6_ADDRSTRLEN, say by adding a enough leading zeros. get_str_sep()
will then silently truncate ADDRESS.
Suggest to avoid get_str_sep() like this (not even compile-tested):
if (ip6_net) {
char *slashp = strchr(ip6_net, '/');
if (!slashp) {
/* Default 64bit prefix length. */
qemu_opt_set(opts, "ipv6-prefix", ip6_net, &error_abort);
qemu_opt_set_number(opts, "ipv6-prefixlen", 64, &error_abort);
} else {
/* User-specified prefix length. */
unsigned long len;
int err;
char *addr = g_strndup(ip6_net, slashp - ip6_net);
qemu_opt_set(opts, "ipv6-prefix", addr, &error_abort);
g_free(addr);
err = qemu_strtoul(slashp + 1, NULL, 10, &len);
if (err) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
"ipv6-prefix", "a number");
} else {
qemu_opt_set_number(opts, "ipv6-prefixlen", len,
&error_abort);
}
}
qemu_opt_unset(opts, "ipv6-net");
}
}
I'd be tempted to clean up further; de-duplicate the qemu_opt_set() and
qemu_opt_set_number().
There's just one more use of get_str_sep(), in parse_host_port(), and it
looks just as prone to silent truncation.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()
2019-05-06 13:23 ` Markus Armbruster
@ 2019-05-06 17:42 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2019-05-06 17:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Peter Maydell, Jason Wang, qemu-devel
On Mon, May 06, 2019 at 03:23:08PM +0200, Markus Armbruster wrote:
> Stefano Garzarella <sgarzare@redhat.com> writes:
>
> > net_client_init() uses a variable length array to store the prefix
> > of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> > is 'fec0::0').
> > Since the IPv6 prefix can be at most as long as an IPv6 address,
> > we can use an array with fixed size equals to INET6_ADDRSTRLEN.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > net/net.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/net.c b/net/net.c
> > index f3a3c5444c..2e5f27e121 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1118,7 +1118,7 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
> > const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
> >
> > if (ip6_net) {
> > - char buf[strlen(ip6_net) + 1];
> > + char buf[INET6_ADDRSTRLEN];
> >
> > if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
> > /* Default 64bit prefix length. */
>
> Hmm.
>
> Parameter "ipv6-net" is of the form ADDRESS[/PREFIX-SIZE]. If
> /PREFIX-SIZE is present, get_str_sep() copies the ADDRESS part to buf[].
>
> However, nothing stops the user from passing in an ADDRESS longer than
> INET6_ADDRSTRLEN, say by adding a enough leading zeros. get_str_sep()
> will then silently truncate ADDRESS.
>
> Suggest to avoid get_str_sep() like this (not even compile-tested):
>
> if (ip6_net) {
> char *slashp = strchr(ip6_net, '/');
>
> if (!slashp) {
> /* Default 64bit prefix length. */
> qemu_opt_set(opts, "ipv6-prefix", ip6_net, &error_abort);
> qemu_opt_set_number(opts, "ipv6-prefixlen", 64, &error_abort);
> } else {
> /* User-specified prefix length. */
> unsigned long len;
> int err;
> char *addr = g_strndup(ip6_net, slashp - ip6_net);
>
> qemu_opt_set(opts, "ipv6-prefix", addr, &error_abort);
> g_free(addr);
> err = qemu_strtoul(slashp + 1, NULL, 10, &len);
> if (err) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "ipv6-prefix", "a number");
> } else {
> qemu_opt_set_number(opts, "ipv6-prefixlen", len,
> &error_abort);
> }
> }
> qemu_opt_unset(opts, "ipv6-net");
> }
> }
Thank you for the suggestion! It seems much better to me.
I'll follow your idea for the v2.
>
> I'd be tempted to clean up further; de-duplicate the qemu_opt_set() and
> qemu_opt_set_number().
Yes, it seems simple to de-duplicate in this way:
if (ip6_net) {
char *slashp = strchr(ip6_net, '/');
char *addr = ip6_net;
unsigned long len = 64;
if (slashp) {
...
}
qemu_opt_set(opts, "ipv6-prefix", addr, &error_abort);
qemu_opt_set_number(opts, "ipv6-prefixlen", len, &error_abort);
qemu_opt_unset(opts, "ipv6-net");
}
>
> There's just one more use of get_str_sep(), in parse_host_port(), and it
> looks just as prone to silent truncation.
I'll check it.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()
2019-05-03 17:06 ` Stefano Garzarella
(?)
(?)
@ 2019-05-06 17:54 ` Eric Blake
2019-05-07 7:53 ` Stefano Garzarella
-1 siblings, 1 reply; 6+ messages in thread
From: Eric Blake @ 2019-05-06 17:54 UTC (permalink / raw)
To: Stefano Garzarella, qemu-devel; +Cc: Peter Maydell, Jason Wang
[-- Attachment #1: Type: text/plain, Size: 558 bytes --]
On 5/3/19 12:06 PM, Stefano Garzarella wrote:
> net_client_init() uses a variable length array to store the prefix
> of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> is 'fec0::0').
> Since the IPv6 prefix can be at most as long as an IPv6 address,
> we can use an array with fixed size equals to INET6_ADDRSTRLEN.
In addition to Markus' comments, on the subject line:
s/avoid to use/avoid using/
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init()
2019-05-06 17:54 ` Eric Blake
@ 2019-05-07 7:53 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2019-05-07 7:53 UTC (permalink / raw)
To: Eric Blake; +Cc: Peter Maydell, Jason Wang, qemu-devel
On Mon, May 06, 2019 at 12:54:20PM -0500, Eric Blake wrote:
> On 5/3/19 12:06 PM, Stefano Garzarella wrote:
> > net_client_init() uses a variable length array to store the prefix
> > of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> > is 'fec0::0').
> > Since the IPv6 prefix can be at most as long as an IPv6 address,
> > we can use an array with fixed size equals to INET6_ADDRSTRLEN.
>
> In addition to Markus' comments, on the subject line:
>
> s/avoid to use/avoid using/
>
Thanks, I'll change it!
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-07 7:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-03 17:06 [Qemu-devel] [PATCH] net: avoid to use variable length array in net_client_init() Stefano Garzarella
2019-05-03 17:06 ` Stefano Garzarella
2019-05-06 13:23 ` Markus Armbruster
2019-05-06 17:42 ` Stefano Garzarella
2019-05-06 17:54 ` Eric Blake
2019-05-07 7:53 ` Stefano Garzarella
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.