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

> On Tue, 13 Sep 2011 19:43:28 +0200, Christian Röller
> <email address hidden> wrote:
>
> > 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.
>
> Unless device_name points to a string that's bigger than the allowed
> interface name. Consider the following minimal example:
>
> char dst[2]; // expected to hold a one-character "string"
> char src[] = "ab"; // too long
> strncpy(dst, src, sizeof(dst)); // dst not NULL terminated
>
> Now if you checked the length already (in the calling function, for
> example), of course you can safely assume that the supplied string is
> not too long. In order to communicate your assumption to fellow
> programmers, add an assertion:
>
> assert(strlen(device_name) < sizeof(whitelist[0].if_label))
>
> If you didn't check the length beforehand however, you need to display
> an error message and exit gracefully because the interface name is a
> user-supplied value. Assertions are reserved for "programmer-supplied"
> values, you could say.

Yes, you are right I should somehow communicate this. I just want to say that it is not possible
to have here no null-termination, because device_name(src) is at most 15 and
hip_netdev_white_list[hip_netdev_white_list_count].if_label(dst) is 16 and I will write 16 bytes.
You are right the interface name, which should be whitelisted is user-supplied, but the hip_netdev_white_list_add_index_and_name function is only called if the interface name is really existing in the system. So it's valid, which would be only the case if it will be smaller than 16.
But you are right, this is maybe not directly obvious. But I think to comment this in the function
is enough, because an assertion would mean additional computations, which are not necessary because the assertion would never fire.

>
> > 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...
>
> No you're right, multiple \0 bytes don't matter.
>
> >> 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.
>
> Of course; if at all, this should be done in a designated branch since
> the diff will be huge.
>
> >> 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...
>
> Changing the comments was off-topic to begin with, so I'd be for
> finishing it (unless someone objects)
>
> >> 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?
>
> Maybe static wasn't the right term... I rather meant a "statically-sized
> buffer", as in
>
> char label[IF_NAMESIZE];
> hip_find_label_for_address(addr, label);
>
> In this case, the compiler could have complained if the label bfr was
> not big enough.

I tried this and the compiler doesn't complain. I could choose every size vor char label[].
Are you sure, that this will be checked from the compiler? I think this would only be the case,
if I will really declare label as static. Because only these variables are known to the compiler
during the compilephase. For label[IF_NAMESIZE] will first memory allocated during the execution.
Maybe this is the reason why the compiler doesn't complain. Although I would say this must be verifiable during the compilephase. But I am not sure if the compiler will do it.
But maybe I am totaly wrong here. What would you say?

>
> > 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...
>
> Yes, you can't determine the size of a buffer in C without passing it
> along in some way, so in general you can't drop the extra parameter. But
> in this particular case, I don't see how anything smaller than
> IF_NAMESIZE makes sense.

Yes that is true, the buffer should always be IF_NAMESIZE big.
I should make this clear in the comments.

>
> >> 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.
>
> OK, never mind then.