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

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

Actually you are right, but in this case the null-termination is guaranteed.
Because the max interface name length in linux is 15 and I am allocating 16 byte(IF_NAMESIZE)
for my hip_netdev_whitelist_entry struct. And in strncpy I always write IF_NAMESIZE many bytes,
so even in the case our system has a maximum interface name of 15, I would write at least one byte
more and terminate the string in this manner, because strncpy would fill up the buffer with nulls.
So if_label has at least one null-termination, possibly more. I know this is not the definition of
a c-string with only one null-termination at the end, but actually this should not result in any problems. If I am wrong please correct me...

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

Yes of course I can do it, thats because I didn't write this function I just adopt them.
So I tried to change as few as possible.

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

Same here I guess this was the actual auther of the function, but of course you are right this is duplicative. I just don't see it...

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

Again a case of adoption, the actual author use this syntax, so I adopt it.
This change was more the grammar owed, which should be adhered to in comments more
than for variable names. ;-) But I can change it of course lets see what the others
think about it...

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

Oops, this stands colloquially for Returns... ;-) Changed it...

>
> > + *
> > + * @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).

Yes you are right, but to make use of it we need a statically allocated buffer, which we use for calling this function. This would only make sense if the function itself is static, right?

The problem here is, that I couldn't determine the size of the passed char pointer array.
So I couldn't check if I would override the memory or not. This was the reason for the sizeoflabel parameter, which doesn't solve the problem, but makes it maybe more unlikely...

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

OK, I will have a look.

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

Yes...

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

Oh of course, thanks for the hint I have changed it...

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

Again due to adoption, hip_netdev_is_in_white_list was already there and for the function
hip_find_label_for_address I have changed it because at some point of this review it was told that this is standard.

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

« Back to merge proposal