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

Christof Mroz (christof-mroz) wrote :

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.

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

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

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

« Back to merge proposal