| Message ID | 20200109182706.14970-1-aconole@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] netdev-dpdk: Avoid undefined behavior processing devargs | expand |
> > In "Use of library functions" in the C standard, the following statement > is written to apply to all library functions: > > If an argument to a function has an invalid value (such as ... a > null pointer ... the behavior is undefined. > > Later, under the "String handling" section, "Comparison functions" no > exception is listed for strcmp, which means NULL is invalid. It may > be possible for the smap_get to return NULL. > > Given the above, we must check that new_devargs is not null. The check > against NULL for new_devargs later in the function is still valid. > > Fixes: 55e075e65ef9 ("netdev-dpdk: Arbitrary 'dpdk' port naming") > Signed-off-by: Aaron Conole <aconole@redhat.com> LGTM. Acked-by: Ciara Loftus <ciara.loftus@intel.com> > --- > NOTE: This is just code-review caught while looking elsewhere, and I > didn't test it. > > lib/netdev-dpdk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 02120a379..00501f7d5 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1817,7 +1817,7 @@ netdev_dpdk_set_config(struct netdev *netdev, > const struct smap *args, > > new_devargs = smap_get(args, "dpdk-devargs"); > > - if (dev->devargs && strcmp(new_devargs, dev->devargs)) { > + if (dev->devargs && new_devargs && strcmp(new_devargs, dev- > >devargs)) { > /* The user requested a new device. If we return error, the caller > * will delete this netdev and try to recreate it. */ > err = EAGAIN; > -- > 2.21.0
On 1/13/2020 9:10 AM, Loftus, Ciara wrote: >> >> In "Use of library functions" in the C standard, the following statement >> is written to apply to all library functions: >> >> If an argument to a function has an invalid value (such as ... a >> null pointer ... the behavior is undefined. >> >> Later, under the "String handling" section, "Comparison functions" no >> exception is listed for strcmp, which means NULL is invalid. It may >> be possible for the smap_get to return NULL. >> >> Given the above, we must check that new_devargs is not null. The check >> against NULL for new_devargs later in the function is still valid. >> >> Fixes: 55e075e65ef9 ("netdev-dpdk: Arbitrary 'dpdk' port naming") >> Signed-off-by: Aaron Conole <aconole@redhat.com> > > LGTM. > > Acked-by: Ciara Loftus <ciara.loftus@intel.com> > Thanks, applied to master and backported from 2.12 to 2.7. Regards Ian >> --- >> NOTE: This is just code-review caught while looking elsewhere, and I >> didn't test it. >> >> lib/netdev-dpdk.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 02120a379..00501f7d5 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1817,7 +1817,7 @@ netdev_dpdk_set_config(struct netdev *netdev, >> const struct smap *args, >> >> new_devargs = smap_get(args, "dpdk-devargs"); >> >> - if (dev->devargs && strcmp(new_devargs, dev->devargs)) { >> + if (dev->devargs && new_devargs && strcmp(new_devargs, dev- >>> devargs)) { >> /* The user requested a new device. If we return error, the caller >> * will delete this netdev and try to recreate it. */ >> err = EAGAIN; >> -- >> 2.21.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Jan 09, 2020 at 01:27:06PM -0500, Aaron Conole wrote: > In "Use of library functions" in the C standard, the following statement > is written to apply to all library functions: > > If an argument to a function has an invalid value (such as ... a > null pointer ... the behavior is undefined. > > Later, under the "String handling" section, "Comparison functions" no > exception is listed for strcmp, which means NULL is invalid. It may > be possible for the smap_get to return NULL. It's not just a technicality for strcmp(), this actually causes SIGSEGV with most libc, including glibc. Anyway, I'm glad that this got fixed.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 02120a379..00501f7d5 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1817,7 +1817,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, new_devargs = smap_get(args, "dpdk-devargs"); - if (dev->devargs && strcmp(new_devargs, dev->devargs)) { + if (dev->devargs && new_devargs && strcmp(new_devargs, dev->devargs)) { /* The user requested a new device. If we return error, the caller * will delete this netdev and try to recreate it. */ err = EAGAIN;
In "Use of library functions" in the C standard, the following statement is written to apply to all library functions: If an argument to a function has an invalid value (such as ... a null pointer ... the behavior is undefined. Later, under the "String handling" section, "Comparison functions" no exception is listed for strcmp, which means NULL is invalid. It may be possible for the smap_get to return NULL. Given the above, we must check that new_devargs is not null. The check against NULL for new_devargs later in the function is still valid. Fixes: 55e075e65ef9 ("netdev-dpdk: Arbitrary 'dpdk' port naming") Signed-off-by: Aaron Conole <aconole@redhat.com> --- NOTE: This is just code-review caught while looking elsewhere, and I didn't test it. lib/netdev-dpdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)