[PATCH] ifplugd

Denys Vlasenko vda.linux at googlemail.com
Fri Apr 9 21:27:25 UTC 2010


On Wednesday 07 April 2010 20:02, Maxim Kryžanovský wrote:
> > > > Is this safe?
> > > > 
> > > > if (strncmp(G.iface, RTA_DATA(attr), len) == 0)
> > > > 
> > > > What if RTA_DATA(attr) = "if", len = 2, and G.iface = "if0"?
> > > > Or does kernel pass attr with NUL included?
> > 
> > > Yes, it is not safe. It needs to compare their lengths:
> > > int iface_len = strlen(G.iface);
> > > if (iface_len == len && strncmp(G.iface, RTA_DATA(attr), len) == 0)
> > 
> > Ok, now what if RTA_DATA(attr) = "if\0", len = 3, and G.iface = "if"?
> > Your code will think that strings aren't equal. Maybe
> > 
> > if (iface_len <= len && strncmp(G.iface, RTA_DATA(attr), len) == 0)
> 
> Hi Denis.
> 
> The kernel pass attr with NUL included, but I could not determine
> whether does this always. If so, the NUL can be omitted from the total
> length to avoid the need to compare strings with different lengths.
> 
> diff --git a/networking/ifplugd.c b/networking/ifplugd.c
> index 41b04c4..dcb6d4d 100644
> --- a/networking/ifplugd.c
> +++ b/networking/ifplugd.c
> @@ -525,10 +525,10 @@ static NOINLINE int check_existence_through_netlink(void)
>  
>                                 while (RTA_OK(attr, attr_len)) {
>                                         if (attr->rta_type == IFLA_IFNAME) {
> -                                               int len = RTA_PAYLOAD(attr);
> +                                               int len = RTA_PAYLOAD(attr) - 1;
>                                                 if (len > IFNAMSIZ)
>                                                         len = IFNAMSIZ;
> -                                               if (iface_len <= len
> +                                               if (iface_len == len
>                                                  && strncmp(G.iface, RTA_DATA(attr), len) == 0
>                                                 ) {
>                                                         G.iface_exists = (mhdr->nlmsg_type == RTM_NEWLINK);

This does not look right. Now if kernel reports "if1", len=3,
you will consider it == "if".

Current code treats "if1",len=3 and "if1\0",len=4 as the same string,
"if1", which seems to be intuitively correct for C conventions.

I just don't see the case you are trying to fix. Describe it,
give the values where it works wrongly:
G.iface = "??", RTA_DATA(attr) = "??", len = ??


Regrding:

+#if ENABLE_FEATURE_PIDFILE
 	write_pidfile(pidfile_name);
+#endif

This is not needed since libbb.h has

#define write_pidfile(path)  ((void)0)
#define remove_pidfile(path) ((void)0)

if ENABLE_FEATURE_PIDFILE is 0.

-- 
vda


More information about the busybox mailing list