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

Christof Mroz (christof-mroz) wrote :

Most of the following are just suggestions, though there's one stopper.

> -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));
> + hip_netdev_white_list_count++;

Sorry to bring up this issue again :)

I suspect you already know that the strncpy() may leave you with an
unterminated if_label here, and you're betting on a sensible length of
device_name. See e.g. [1] for an explanation.
It's always a good idea to add an assert() when gambling, for the sole
purpose of documentation if anything. But better still, I'd terminate
the string explicitly.

If I'm mistaken here, please comment the code (or the device_name
parameter) to clarify. Audit all the other instances of strncpy() you
touched, too.

Also, dropping the hip_netdev_ prefix, at least for static identifiers,
looks safe to me and it would vastly shorten line lengths everywhere in
this file.

[1] http://stackoverflow.com/questions/1453876/why-does-strncpy-not-null-terminate

> + if (hip_netdev_white_list_count > 0) {
> + for (unsigned int i = 0; i < hip_netdev_white_list_count; i++) {
> + if (hip_netdev_white_list[i].if_index == if_index &&
> + !strncmp(hip_netdev_white_list[i].if_label,
> + device_name, sizeof(hip_netdev_white_list[0].if_label))) {
> + return 1;
> + }
> }
> }

Drop the outer if(...): This check is performed on the first for(...) iteration
anyway.

     return 0;
 }

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

You replaced `white list' with `whitelist' in comments, so you should
change it in identifiers as well: `white_list' -> `whitelist'.

> /**
> + * Gives you the interface label for a given IPv4 or IPv6 address.

`Gives you' -> `Returns'

> + *
> + * @param addr Address for which you want to know the label
> + * @param label Pointer where the function stores the label.
> + * @param sizeoflabel Size of the passed label parameter, should be chosen big enough
> + * to be able to store every possible interface name plus null-termination.
> + * @return Zero on success, one on error and write the label
> + * in param label if the given address exists in the system.
> + * Label will be null terminated for sure, iff param label
> + * is chosen big enough(IF_NAMESIZE == 16).
> + */

I'd say it's reasonable to require `sizeof(label) >= IF_NAMESIZE' in the
documentation and drop the sizeoflabel parameter. Note that you can
specify

int hip_find_label_for_address(const struct sockaddr *const addr,
                               char label[IF_NAMESIZE])

to enable a minimal compile-time check (at least for statically
allocated buffers. Hey, better than nothing).

Also, refer to the doxygen section in HACKING on how to markup parameter
names (I don't remember right now).

> +int hip_find_label_for_address(const struct sockaddr *const addr,
> + char *const label, unsigned int const sizeoflabel)

Passing addresses by-value is preferred.

> + 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;
> + }

The NULL check is already inferred by the other checks.

> +
> + if (!memcmp(addr, ifa->ifa_addr, sizeof(addr))) {
> + if (strlen(ifa->ifa_name) < sizeoflabel) {
> + strncpy(label, ifa->ifa_name, sizeoflabel);
> + res = 0;
> + }
> + break;
> + }
> + }

Stopper: addr is a pointer, so sizeof(addr) is always sizeof(void*); you
meant sizeof(*addr).

This one may have slipped through your tests because accidentally,
sizeof(void*) = sizeof(struct sockaddr). Be sure to check your code with
IPv6, too.

> + if (!hip_find_label_for_address(addr, label, sizeof(label)) &&
> + !hip_netdev_is_in_white_list(ifa->ifa_index, label)) {
> + HIP_DEBUG("Interface:<%s> is not whitelisted\n", label);
> + continue;
> + }

Maybe it's just me, but you return zero both for success and failure
depending on the function, which confused me at first. Probably no big
deal since these functions are not public, though.

> === added file 'test/check_hipd.c'
> --- test/check_hipd.c 1970-01-01 00:00:00 +0000
> +++ test/check_hipd.c 2011-09-13 12:20:25 +0000
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2010 Aalto University and RWTH Aachen University.
> + *

2011

review: Needs Fixing

« Back to merge proposal