Message ID | 20170812120510.28750-22-phil@nwl.cc |
---|---|
State | Changes Requested, archived |
Delegated to: | stephen hemminger |
Headers | show |
On Sat, 12 Aug 2017 14:04:40 +0200 Phil Sutter <phil@nwl.cc> wrote: > Both addattr_l() and rta_addattr_l() may be called with NULL data > pointer and 0 alen parameters. Avoid calling memcpy() in that case. > > Signed-off-by: Phil Sutter <phil@nwl.cc> What are you fixing. memcpy(dest, NULL, 0) should be harmless NOP
On Tue, Aug 15, 2017 at 08:15:55AM -0700, Stephen Hemminger wrote: > On Sat, 12 Aug 2017 14:04:40 +0200 > Phil Sutter <phil@nwl.cc> wrote: > > > Both addattr_l() and rta_addattr_l() may be called with NULL data > > pointer and 0 alen parameters. Avoid calling memcpy() in that case. > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > What are you fixing. memcpy(dest, NULL, 0) should be harmless NOP Yes, if that turns into a NOP this patch is not needed. Thanks, Phil
> From: "Phil Sutter" <phil@nwl.cc> > To: "Stephen Hemminger" <stephen@networkplumber.org> > Cc: netdev@vger.kernel.org > Sent: Tuesday, August 15, 2017 12:42:55 PM > Subject: Re: [iproute PATCH 21/51] lib/libnetlink: Don't pass NULL parameter to memcpy() > > On Tue, Aug 15, 2017 at 08:15:55AM -0700, Stephen Hemminger wrote: > > On Sat, 12 Aug 2017 14:04:40 +0200 > > Phil Sutter <phil@nwl.cc> wrote: > > > > > Both addattr_l() and rta_addattr_l() may be called with NULL data > > > pointer and 0 alen parameters. Avoid calling memcpy() in that case. > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > What are you fixing. memcpy(dest, NULL, 0) should be harmless NOP > > Yes, if that turns into a NOP this patch is not needed. > > Thanks, Phil > It is a NOP in this case, but it is also "undefined behavior" and can lead to the compiler assuming that dest != NULL, which would be problematic if dest were dereferenced later in the code (it isn't in this case, but might be in general). A small example with current gcc: foo.c: #include <stdio.h> extern void foo(char *, size_t); int main(int argc, char **argv) { char x[128]; foo(x, sizeof x); foo(NULL, 0); return 0; } bar.c: #include <stdio.h> #include <string.h> void foo(char *ptr, size_t len) { memset(ptr, 0, len); if (ptr) printf("ptr is non-null: %p\n", ptr); } Compile the code: $ gcc -o foobar -O2 foo.c bar.c Execute it (note second line of output, which might be surprising): $ ./foobar ptr is non-null: 0x7ffdc47daef0 ptr is non-null: (nil) Regards, Lance Richardson
diff --git a/lib/libnetlink.c b/lib/libnetlink.c index 81a344abff27a..5e3adade77077 100644 --- a/lib/libnetlink.c +++ b/lib/libnetlink.c @@ -866,7 +866,8 @@ int addattr_l(struct nlmsghdr *n, int maxlen, int type, const void *data, rta = NLMSG_TAIL(n); rta->rta_type = type; rta->rta_len = len; - memcpy(RTA_DATA(rta), data, alen); + if (alen) + memcpy(RTA_DATA(rta), data, alen); n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(len); return 0; } @@ -953,7 +954,8 @@ int rta_addattr_l(struct rtattr *rta, int maxlen, int type, subrta = (struct rtattr *)(((char *)rta) + RTA_ALIGN(rta->rta_len)); subrta->rta_type = type; subrta->rta_len = len; - memcpy(RTA_DATA(subrta), data, alen); + if (alen) + memcpy(RTA_DATA(subrta), data, alen); rta->rta_len = NLMSG_ALIGN(rta->rta_len) + RTA_ALIGN(len); return 0; }
Both addattr_l() and rta_addattr_l() may be called with NULL data pointer and 0 alen parameters. Avoid calling memcpy() in that case. Signed-off-by: Phil Sutter <phil@nwl.cc> --- lib/libnetlink.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)