Code review comment for lp:~christian-roeller/hipl/whitelisting

Hi,

sorry for the confusion.
I have just found out,that IF_NAMESIZE is big enough to fit the max. interface
name length plus null-termination. So with Rene's suggested change of using sizeof(*) instead of sizeof(* - 1) it is fine and guarantee a null-termination.

Christian

> > 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.

No need for this, if the chosen interfacename would be to long, then the interface can
obviously not exist and the ioctl will later fail.

>
> >
> > > /**
> > > + * 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/

« Back to merge proposal