| Message ID | 20190503170631.228487-1-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: avoid to use variable length array in net_client_init() | expand |
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.
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
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/
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
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. */
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(-)