> Hi,
> 
> I just got one issue that was raised by Stefan before, but has not been fixed
> so far (more inline).
> 
> On 22.08.2011, at 21:05, Christian Röller wrote:
> > Christian Röller has proposed merging lp:~christian-
> roeller/hipl/whitelisting into lp:hipl.
> >
> > Requested reviews:
> >  Stefan Götz (stefan.goetz)
> >  Diego Biurrun (diego-biurrun)
> >
> > For more details, see:
> > https://code.launchpad.net/~christian-roeller/hipl/whitelisting/+merge/72487
> 
> > /**
> > - * Add a network interface index number to the list of white listed
> > + * Add a network interface index number plus label to the list of
> whitelisted
> >  * network interfaces.
> >  *
> > - * @param if_index the network interface index to be white listed
> > + * @param if_index the network interface index to be whitelisted
> > + * @param device_name the network interface label to be whitelisted
> >  */
> > -static void hip_netdev_white_list_add_index(int if_index)
> > +static void hip_netdev_white_list_add_index_and_name(const unsigned int
> if_index,
> > +                                                     const char *const
> device_name)
> > {
> >     if (hip_netdev_white_list_count < HIP_NETDEV_MAX_WHITE_LIST) {
> > -        hip_netdev_white_list[hip_netdev_white_list_count++] = if_index;
> > +        hip_netdev_white_list[hip_netdev_white_list_count].if_index =
> if_index;
> > +
> strncpy(hip_netdev_white_list[hip_netdev_white_list_count].if_label,
> > +                device_name, sizeof(hip_netdev_white_list[0].if_label) -
> 1);
> 
> Why don't you strncpy the complete sizeof? Either the string is null-
> terminated, in which case you should copy the 0 as well, or it is not, in
> which case you will miss the last letter of the label. What happens with label
> is of length 1?

Yes, you are right, I should definitly use the complete sizeof here. But it is always the same with
strncpy. Either it only copies the start of the string if the source is too long or it fills up
the destination with zeros, which is actually time wasting. But I think the more worse problem is to
have no null-termination, which would be the case if device_name will be exactly as big as IF_NAMESIZE. 
So I should maybe increase:

struct hip_netdev_whitelist_entry {
    unsigned int if_index;
-    char         if_label[IF_NAMESIZE];
+    char         if_label[IF_NAMESIZE+1];
};

to guarantee a null-termination. If we assume, that device_name is not bigger than IF_NAMESIZE, which we could/should check in function hip_netdev_white_list_add. See below...
> 
> > /**
> > - * Add a network interface index number to the list of white listed
> > + * Add a network interface index number to the list of whitelisted
> >  * network interfaces by name.
> >  *
> > - * @param device_name the name of the device to be white listed
> > + * @param device_name the name of the device to be whitelisted
> >  * @return 1 on success, 0 on error
> >  */
> > int hip_netdev_white_list_add(const char *const device_name)
> > @@ -146,19 +160,18 @@
> >     int          sock = 0;
> >     int          ret  = 0;
> >
> > -    strncpy(ifr.ifr_name, device_name, IF_NAMESIZE);
> > +    strncpy(ifr.ifr_name, device_name, sizeof(ifr.ifr_name) - 1);
> 
> Same here.

This was actually not my code, but of course you are right is the same problem. 
I should also use here the complete sizeof. Maybe I should test here if device_name is 
not longer/bigger than sizeof(ifr.ifr_name) to ensure a null-termination. Because device_name comes from the command line and could have any length if I see that correct. It will be checked nowhere, wo we can do it here.

> 
> > /**
> > + * Gives you the interface label for a given IPv4 or IPv6 address.
> > + *
> > + * @param addr address for which you want to know the label
> > + * @param label pointer where the function stores the label
> > + * @return zero on success, one on error and write the label
> > + *         in param label if the given address exists in the system
> > + */
> > +int hip_find_label_for_address(const struct sockaddr *const addr,
> > +                               char *const label)
> > +{
> > +    int             res = 1;
> > +    struct ifaddrs *myaddrs, *ifa = NULL;
> > +
> > +    getifaddrs(&myaddrs);
> > +    for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> > +        if (ifa->ifa_addr == NULL ||
> > +            (ifa->ifa_addr->sa_family != AF_INET &&
> > +             ifa->ifa_addr->sa_family != AF_INET6)) {
> > +            continue;
> > +        }
> > +
> > +        if (!memcmp(addr, ifa->ifa_addr, sizeof(addr))) {
> > +            if (strlen(ifa->ifa_name) <= sizeof(label) - 1) {
> > +                strncpy(label, ifa->ifa_name, sizeof(label) - 1);
> 
> And here.

And the same here. Actually it makes no different here because I will check first if ifa->ifa_name would be not bigger/longer than sizeof(label) - 1. So there are not more than sizeof(label) - 1 bytes to write. But maybe it would be better to allocate IF_NAMESIZE +1 bytes for label before the calling and let the if statement like it is with the -1 but write sizeof(label) bytes with strncpy to guanrantee the null-termination again.


What do you say? Strings in C is always a confusing matter. ;-)

Christian
> 
> 
> 
> --
> Dipl.-Inform. Rene Hummen, Ph.D. Student
> Chair of Communication and Distributed Systems
> RWTH Aachen University, Germany
> tel: +49 241 80 20772
> web: http://www.comsys.rwth-aachen.de/team/rene-hummen/