Merge lp:~christian-roeller/hipl/whitelisting into lp:hipl

[Christian Röller]

Change in the whitelisting of hipl:

The problem with the current whitelisting fuction is, that it only considers the interface-indexes. So when you for instance want to whitelist your eth0 interface, but there are also alias-interfaces for this interface on your machine, then a change for this alias-interfaces would also trigger an update, because these interfaces have the same interface-index for the kernel.

To have a more concrete possibility to whitelist interfaces, I extend the whitelist-structure with the interface-label. So that when you now whitelist your interface eth0, only changes on this interface would trigger an update and not for the alias-interfaces.

I did this by just checking for every incoming NEWADDR netlinkevent, if the corresponding label for this address is whitelisted or not.

So please have a look and just tell if you are fine with this...

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christian!

Thanks for this helpful contribution! My comments below:

> > === modified file 'hipd/netdev.c'
> > --- hipd/netdev.c 2011-05-31 18:21:28 +0000
> > +++ hipd/netdev.c 2011-07-14 15:34:41 +0000
> > -static void hip_netdev_white_list_add_index(int if_index)
> > +static void hip_netdev_white_list_add_index_and_name(int if_index, const char *const device_name)

[L] cleanup: please add a 'const' modifier to the 'if_index' argument
[L] cleanup: I guess that 'if_index' cannot be less than 0. If so, it would be
nice to change its type to unsigned to communicate this fact to the caller.

> > {
> > 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, IF_NAMESIZE);
[L] maintainability: it is more maintainable to use
'sizeof(hip_netdev_white_list[0].if_label)' than 'IF_NAMESIZE' because then the
size of the 'if_label' field can be changed in the struct declaration without
having to touch other code.

> > @@ -120,16 +127,18 @@
> > }
> >
> > /**
> > - * Test if the given network interface index is white listed.
> > + * Test if the given network interface index plus label is white listed.
> > *
> > * @param if_index the index of the network interface to be tested
> > + * @param device_name the label of the network interface to be tested
> > * @return 1 if the index is whitelisted or zero otherwise
> > */
> > -static int hip_netdev_is_in_white_list(int if_index)
> > +static int hip_netdev_is_in_white_list(int if_index, char *device_name)

[M] policy: please apply full const correctness to the function arguments.
[L] cleanup: it would be nice to change the type of 'if_index' to unsigned as it
can never be < 0 (I guess)

> > {
> > - int i = 0;
> > + int i;
> > for (i = 0; i < hip_netdev_white_list_count; i++) {

[L] style: you could even place the declaration of 'i' in the for() header.

> > - if (hip_netdev_white_list[i] == if_index) {
> > + if (hip_netdev_white_list[i].if_index == if_index &&
> > + !strncmp(hip_netdev_white_list[i].if_label, device_name, IF_NAMESIZE)) {

[L] maintainability: 'sizeof()' instead of 'IF_NAMESIZE', see above

> > @@ -637,7 +645,7 @@
> > HIP_IFEL(!(if_index = if_nametoindex(g_iface->ifa_name)),
> > -1, "if_nametoindex failed\n");
> > /* Check if our interface is in the whitelist */
> > - if ((hip_netdev_white_list_count > 0) && (!hip_netdev_is_in_white_list(if_index))) {
> > + if ((hip_netdev_white_list_count > 0) && (!hip_netdev_is_in_white_list(if_index, g_iface->ifa_name))) {

[L] style: just a general comment: it would make more sense to move the '> 0'
check into the 'hip_netdev_is_in_white_list()' function so the caller does not
need to have internal knowledge about the list implementation. Furthermore it's
a really silly performance optimization.

> > @@ -1125,6 +1133,55 @@
> > }
> >
> > /**
> > + * find interface label for a given ip4 or ip6 addr
...

review: Needs Fixing

[Miika Komu (miika-iki)]

Quick test report: seems to work without any troubles.

[Christian Röller (christian-roeller)]

Once my exam is over I have finally found the time to work on your improvement suggestions...

> Hi Christian!
>
> Thanks for this helpful contribution! My comments below:
>
> > > === modified file 'hipd/netdev.c'
> > > --- hipd/netdev.c 2011-05-31 18:21:28 +0000
> > > +++ hipd/netdev.c 2011-07-14 15:34:41 +0000
> > > -static void hip_netdev_white_list_add_index(int if_index)
> > > +static void hip_netdev_white_list_add_index_and_name(int if_index, const
> char *const device_name)
>
> [L] cleanup: please add a 'const' modifier to the 'if_index' argument
> [L] cleanup: I guess that 'if_index' cannot be less than 0. If so, it would be
> nice to change its type to unsigned to communicate this fact to the caller.

Both done, you are right no reason here for dont make it unsigned and const...

>
> > > {
> > > 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, IF_NAMESIZE);
> [L] maintainability: it is more maintainable to use
> 'sizeof(hip_netdev_white_list[0].if_label)' than 'IF_NAMESIZE' because then
> the
> size of the 'if_label' field can be changed in the struct declaration without
> having to touch other code.

Good point I didnt take this into account, its changed now

>
> > > @@ -120,16 +127,18 @@
> > > }
> > >
> > > /**
> > > - * Test if the given network interface index is white listed.
> > > + * Test if the given network interface index plus label is white listed.
> > > *
> > > * @param if_index the index of the network interface to be tested
> > > + * @param device_name the label of the network interface to be tested
> > > * @return 1 if the index is whitelisted or zero otherwise
> > > */
> > > -static int hip_netdev_is_in_white_list(int if_index)
> > > +static int hip_netdev_is_in_white_list(int if_index, char *device_name)
>
> [M] policy: please apply full const correctness to the function arguments.
> [L] cleanup: it would be nice to change the type of 'if_index' to unsigned as
> it
> can never be < 0 (I guess)

Same as above, so done...

>
> > > {
> > > - int i = 0;
> > > + int i;
> > > for (i = 0; i < hip_netdev_white_list_count; i++) {
>
> [L] style: you could even place the declaration of 'i' in the for() header.

Yes I can, so I did... ;-) The actual writer did it like this, so I just adopt it...

>
> > > - if (hip_netdev_white_list[i] == if_index) {
> > > + if (hip_netdev_white_list[i].if_index == if_index &&
> > > + !strncmp(hip_netdev_white_list[i].if_label, device_name,
> IF_NAMESIZE)) {
>
> [L] maintainability: 'sizeof()' instead of 'IF_NAMESIZE', see above

Done...but I use sizeof()-1 to do not override the null-termination

>
> > > @@ -637,7 +645,7 @@
> > > HIP_IFEL(!(if_index = if_nametoindex(g_iface->ifa_name)),
> > > -1, "if_nametoindex failed\n");
> > > /* Check if our interface is in the whitelist */
> > > - if ((hip_netdev_white_list_count ...

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christian!

Thanks for your detailed reply! I was on holidays so that's why my answer is
quite late:

>>>> - if (hip_netdev_white_list[i] == if_index) {
>>>> + if (hip_netdev_white_list[i].if_index == if_index &&
>>>> + !strncmp(hip_netdev_white_list[i].if_label, device_name,
>> IF_NAMESIZE)) {
>>
>> [L] maintainability: 'sizeof()' instead of 'IF_NAMESIZE', see above
>
> Done...but I use sizeof()-1 to do not override the null-termination

Hm - in the case of the read-only 'strncmp()' function, I'm pretty sure that you
want to check both strings to their full extent, including the null-terminating
character. Otherwise, you can get false positives with unterminated strings (I
know, that's a pathological case, but still, passing 'sizeof() - 1' to
'strncmp()' seems logically incorrect to me).

>>>> +int hip_find_label_for_address(struct sockaddr *addr, char *label)
>>
>> [M] policy: please ensure full const correctness
>
> I write to label and cast addr, so I cannot declare const, can I?

You can use 'const struct sockaddr *const addr' and cast it to 'const struct
sockaddr_in const* s4_in'. Similarly, you can use 'char *const label' as you do
not intend to or do change the pointer value.

>>>> + res = getifaddrs(&myaddrs);
>>>> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
>>>> + if (ifa->ifa_addr == NULL) continue;
>>>> + if (ifa->ifa_addr->sa_family != AF_INET &&
>> ifa->ifa_addr->sa_family != AF_INET6) continue;
>>
>> [M] policy: HIPL always uses "long" if statements including braces and
>> newlines.
>> Please adhere to that style. Do you use the pre-commit hook for checking your
>> code style? I'm asking because I'm surprised that the checker does not catch
>> this.
>
> For me the statement doesnt include braces or a new line. Maybe I have changed it and forgot to commit it...maybe you can check it again...

Exactly my point: it *should* include braces and newlines but currently it does
not :-) It should read

if (ifa->ifa_addr == NULL) {
 continue;
}

etc.

>> [L] maintainability: similar to the above cases, 'sizeof(s4_in)' would be more
>> easily maintainable and more obvious to read than 'sizeof(struct
>> sockaddr_in)'.
>> The same applies to the code below.
>>
>>>> + strncpy(label, ifa->ifa_name, IF_NAMESIZE);
>>
>> [L] 'sizeof()'
>
> Both done, like described above...

Just like 'strncmp()', 'strncpy()' should use the full character array - the
code should also check that the string to copy is not longer than the given
character array - 1 or otherwise return an error from the function. Only this
combination will ensure that the string is either copied correctly or an error
is detected under all circumstances.

>>>> + freeifaddrs(myaddrs);
>>>> + return 1;
>>>> + }
>>>> + }
>>>> + if (ifa->ifa_addr->sa_family == AF_INET6) {
>>>> + s6 = (struct sockaddr_in6 *)(ifa->ifa_addr);
>>>> + if (!memcmp(s6_in, s6, sizeof(struct sockaddr_in6))) {
>>>> + strncpy(label, ifa->ifa_name, IF_NAMESIZE);
>>>> + freeifaddrs(myaddrs);
>>>> + return 1;
>>
>> [L] styl...

[Christian Röller (christian-roeller)]

> Hi Christian!
>
> Thanks for your detailed reply! I was on holidays so that's why my answer is
> quite late:

No problem my first reply has also taken a while due to my exam preparations ;-)

>
> >>>> - if (hip_netdev_white_list[i] == if_index) {
> >>>> + if (hip_netdev_white_list[i].if_index == if_index &&
> >>>> + !strncmp(hip_netdev_white_list[i].if_label, device_name,
> >> IF_NAMESIZE)) {
> >>
> >> [L] maintainability: 'sizeof()' instead of 'IF_NAMESIZE', see above
> >
> > Done...but I use sizeof()-1 to do not override the null-termination
>
> Hm - in the case of the read-only 'strncmp()' function, I'm pretty sure that
> you
> want to check both strings to their full extent, including the null-
> terminating
> character. Otherwise, you can get false positives with unterminated strings (I
> know, that's a pathological case, but still, passing 'sizeof() - 1' to
> 'strncmp()' seems logically incorrect to me).

Yes of course...I never did it in the code. This was just a comment at the wrong position.

>
> >>>> +int hip_find_label_for_address(struct sockaddr *addr, char *label)
> >>
> >> [M] policy: please ensure full const correctness
> >
> > I write to label and cast addr, so I cannot declare const, can I?
>
> You can use 'const struct sockaddr *const addr' and cast it to 'const struct
> sockaddr_in const* s4_in'. Similarly, you can use 'char *const label' as you
> do
> not intend to or do change the pointer value.

Yes you are right, did it.

>
> >>>> + res = getifaddrs(&myaddrs);
> >>>> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> >>>> + if (ifa->ifa_addr == NULL) continue;
> >>>> + if (ifa->ifa_addr->sa_family != AF_INET &&
> >> ifa->ifa_addr->sa_family != AF_INET6) continue;
> >>
> >> [M] policy: HIPL always uses "long" if statements including braces and
> >> newlines.
> >> Please adhere to that style. Do you use the pre-commit hook for checking
> your
> >> code style? I'm asking because I'm surprised that the checker does not
> catch
> >> this.
> >
> > For me the statement doesnt include braces or a new line. Maybe I have
> changed it and forgot to commit it...maybe you can check it again...
>
> Exactly my point: it *should* include braces and newlines but currently it
> does
> not :-) It should read
>
> if (ifa->ifa_addr == NULL) {
> continue;
> }
>
> etc.

Ah OK now I got your point...sorry ;-) I changed it

>
> >> [L] maintainability: similar to the above cases, 'sizeof(s4_in)' would be
> more
> >> easily maintainable and more obvious to read than 'sizeof(struct
> >> sockaddr_in)'.
> >> The same applies to the code below.
> >>
> >>>> + strncpy(label, ifa->ifa_name, IF_NAMESIZE);
> >>
> >> [L] 'sizeof()'
> >
> > Both done, like described above...
>
> Just like 'strncmp()', 'strncpy()' should use the full character array - the
> code should also check that the string to copy is not longer than the given
> character array - 1 or otherwise return an error from the function. Only this
> combination will ensure that the string is either copied correctly or an error
> is detected under all circumstances.

OK, I changed it in the following way:
I chec...

[Stefan Götz (stefan.goetz-deactivatedaccount)]

review: Approve

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Thu, Jul 14, 2011 at 03:34:45PM +0000, Christian Röller wrote:
> Christian Röller has proposed merging lp:~christian-roeller/hipl/whitelisting into lp:hipl.

Formatting is messed-up. Didn't you install the uncrustify pre-commit
hook as described in doc/HACKING? Why?

Diego

review: Needs Fixing

[Christian Röller (christian-roeller)]

> review needs-fixing
>
> On Thu, Jul 14, 2011 at 03:34:45PM +0000, Christian Röller wrote:
> > Christian Röller has proposed merging lp:~christian-
> roeller/hipl/whitelisting into lp:hipl.
>
> Formatting is messed-up. Didn't you install the uncrustify pre-commit
> hook as described in doc/HACKING? Why?
>
> Diego

Sorry for that, I checked now my modifications with the stylechecker and correct everything it complained about.

[Diego Biurrun (diego-biurrun)]

On Mon, Aug 01, 2011 at 09:19:26AM +0000, Christian Röller wrote:
> > On Thu, Jul 14, 2011 at 03:34:45PM +0000, Christian Röller wrote:
> > > Christian Röller has proposed merging lp:~christian-
> > roeller/hipl/whitelisting into lp:hipl.
> >
> > Formatting is messed-up. Didn't you install the uncrustify pre-commit
> > hook as described in doc/HACKING? Why?
>
> Sorry for that, I checked now my modifications with the stylechecker
> and correct everything it complained about.

I'm not trying to pass blame around. I want to know why some of our
processes are not working as intended, so that we can fix them.

Why were you not running uncrustify? Did you just forget to install
the hook (no big deal, we all make mistakes and we have review for a
reason) or did you not know about it? Where would we have to advertise
it so that the next newcomer is informed of its existence?

Diego

[Christian Röller (christian-roeller)]

> On Mon, Aug 01, 2011 at 09:19:26AM +0000, Christian Röller wrote:
> > > On Thu, Jul 14, 2011 at 03:34:45PM +0000, Christian Röller wrote:
> > > > Christian Röller has proposed merging lp:~christian-
> > > roeller/hipl/whitelisting into lp:hipl.
> > >
> > > Formatting is messed-up. Didn't you install the uncrustify pre-commit
> > > hook as described in doc/HACKING? Why?
> >
> > Sorry for that, I checked now my modifications with the stylechecker
> > and correct everything it complained about.
>
> I'm not trying to pass blame around. I want to know why some of our
> processes are not working as intended, so that we can fix them.
>
> Why were you not running uncrustify? Did you just forget to install
> the hook (no big deal, we all make mistakes and we have review for a
> reason) or did you not know about it? Where would we have to advertise
> it so that the next newcomer is informed of its existence?
>
> Diego

I have heard that there is a hook for this, but as this is my first share,
I just forgot to install it.
So in this case it was just my fault...
But for the next newcomers, it would be nice to advertise the HACKING doc a
little bit more, because it is very helpful and I don't know about it. I just
heard about the stylechecker through the grapevine.

So sorry again and thanks for the infos...
Christian

[Diego Biurrun (diego-biurrun)]

On Mon, Aug 01, 2011 at 09:59:28AM +0000, Christian Röller wrote:
> > On Mon, Aug 01, 2011 at 09:19:26AM +0000, Christian Röller wrote:
> > > > On Thu, Jul 14, 2011 at 03:34:45PM +0000, Christian Röller wrote:
> > > > > Christian Röller has proposed merging lp:~christian-
> > > > roeller/hipl/whitelisting into lp:hipl.
> > > >
> > > > Formatting is messed-up. Didn't you install the uncrustify pre-commit
> > > > hook as described in doc/HACKING? Why?
> > >
> > > Sorry for that, I checked now my modifications with the stylechecker
> > > and correct everything it complained about.
> >
> > I'm not trying to pass blame around. I want to know why some of our
> > processes are not working as intended, so that we can fix them.
> >
> > Why were you not running uncrustify? Did you just forget to install
> > the hook (no big deal, we all make mistakes and we have review for a
> > reason) or did you not know about it? Where would we have to advertise
> > it so that the next newcomer is informed of its existence?
>
> I have heard that there is a hook for this, but as this is my first share,
> I just forgot to install it.
> So in this case it was just my fault...

I repeat: I'm not trying to pass blame around. Stop apologizing please.

> But for the next newcomers, it would be nice to advertise the HACKING doc a
> little bit more, because it is very helpful and I don't know about it. I just
> heard about the stylechecker through the grapevine.

I've updated INSTALL and the PISA wiki slightly. There's still room for
improvement though.

Diego

[Christian Röller (christian-roeller)]

Hi,

sorry for my long absence, it was again exam-time ;-)

So I correct everything, which were mentioned. Are there more improvement suggestions or any other reasons why it should not be merged in this version?
Otherwise I would do so.
I merged it locally and as there are only a few changes in two files, it works without any trouble.
The same applies for the compiling.

Regards,
Christian

[René Hummen (rene-hummen)]

Diego, have your comments been tackled? If so, please approve the merge request.

[Diego Biurrun (diego-biurrun)]

On Wed, Aug 10, 2011 at 02:07:38PM +0000, Christian Röller wrote:
>
> So I correct everything, which were mentioned. Are there more
> improvement suggestions or any other reasons why it should not be
> merged in this version?
> Otherwise I would do so.

The general idea is that you resubmit a fixed merge proposal.

Diego

[Diego Biurrun (diego-biurrun)]

On Wed, Aug 10, 2011 at 02:39:35PM +0000, René Hummen wrote:
> Diego, have your comments been tackled? If so, please approve the merge request.

There was no updated merge request, so the answer is no.

Diego

[Christian Röller (christian-roeller)]

> On Wed, Aug 10, 2011 at 02:39:35PM +0000, René Hummen wrote:
> > Diego, have your comments been tackled? If so, please approve the merge
> request.
>
> There was no updated merge request, so the answer is no.
>
> Diego

I correct the code style violations with revision 5987.
What do you mean with updated merge request? I answered on your needs-fixing comment and correct it.
I did the same for Stefan's needs-fixing request and he approved it.
What should I rather have done?

Christian

[René Hummen (rene-hummen)]

On 10.08.2011, at 17:11, Christian Röller wrote:
>> On Wed, Aug 10, 2011 at 02:39:35PM +0000, René Hummen wrote:
>>> Diego, have your comments been tackled? If so, please approve the merge
>> request.
>>
>> There was no updated merge request, so the answer is no.
>>
>> Diego
>
> I correct the code style violations with revision 5987.
> What do you mean with updated merge request? I answered on your needs-fixing comment and correct it.
> I did the same for Stefan's needs-fixing request and he approved it.
> What should I rather have done?

In the top right corner of your merge proposal page on the Launchpad website, you can follow the link "Resubmit proposal". Don't check the option to "start afresh" in order to keep the discussion history intact and for others to follow the whole conversation.

Ciao,
René

--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: [phone]
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/

[Diego Biurrun (diego-biurrun)]

On Wed, Aug 10, 2011 at 03:11:23PM +0000, Christian Röller wrote:
> > On Wed, Aug 10, 2011 at 02:39:35PM +0000, René Hummen wrote:
> > > Diego, have your comments been tackled? If so, please approve the merge
> > request.
> >
> > There was no updated merge request, so the answer is no.
>
> I correct the code style violations with revision 5987.
> What do you mean with updated merge request? I answered on your
> needs-fixing comment and correct it.
> I did the same for Stefan's needs-fixing request and he approved it.
> What should I rather have done?

Maybe you did fix it in your branch, but how would we know? We never
had a chance to see and review it. What you should do now is send
another merge request from your updated branch.

Diego

[Diego Biurrun (diego-biurrun)]

On Wed, Aug 10, 2011 at 03:28:27PM +0000, Christian Röller wrote:
> Christian Röller has proposed merging lp:~christian-roeller/hipl/whitelisting into lp:hipl.

Did you run

  make alltests
  tools/hipl_autobuild.sh

with this branch?

> --- hipd/netdev.c 2011-05-25 09:22:19 +0000
> +++ hipd/netdev.c 2011-08-10 15:28:20 +0000
> @@ -99,19 +99,26 @@
> -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)

Please keep lines below 80 characters where easily possible, like here.

More examples below...

> @@ -1122,6 +1131,68 @@
>
> /**
> + * This functions gives you the interface label for a given ip4 or ip6 addr.

IPv4, IPv6 address

> + * @param addr address for which you want to know the label
> + * @param label pointer where the function stores the label
> + * @return one on success, zero on error and write the label in param label

Is the label written in both cases?

> +int hip_find_label_for_address(const struct sockaddr *const addr, char *const label)
> +{
> +
> + if (addr->sa_family == AF_INET) {
> + s4_in = (const struct sockaddr_in *const) (addr);
> + }
> + if (addr->sa_family == AF_INET6) {
> + s6_in = (const struct sockaddr_in6 *const) (addr);
> + }

An else will save you a comparison here.

> + getifaddrs(&myaddrs);
> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> + if (ifa->ifa_addr == NULL) {
> + continue;
> + }
> + if (ifa->ifa_addr->sa_family != AF_INET && ifa->ifa_addr->sa_family != AF_INET6) {
> + continue;
> + }

These two could be merged.

> + if (ifa->ifa_addr->sa_family == AF_INET) {
> + s4 = (struct sockaddr_in *) (ifa->ifa_addr);

pointless ()

> + if (ifa->ifa_addr->sa_family == AF_INET6) {
> + s6 = (struct sockaddr_in6 *) (ifa->ifa_addr);

pointless ()

As above, if you use else here, you could save a comparison.

> + if (!memcmp(s4_in, s4, sizeof(s4))) {
> + if (strlen(ifa->ifa_name) <= sizeof(label) - 1) {
> + strncpy(label, ifa->ifa_name, sizeof(label) - 1);
> + res = 1;
> + } else {
> + res = 0;
> + }
> + break;

> + if (!memcmp(s6_in, s6, sizeof(s6))) {
> + if (strlen(ifa->ifa_name) <= sizeof(label) - 1) {
> + strncpy(label, ifa->ifa_name, sizeof(label) - 1);
> + res = 1;
> + } else {
> + res = 0;
> + }
> + break;

Isn't there a way to avoid this code duplication?

Diego

[Diego Biurrun (diego-biurrun)]

review: Needs Fixing

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Fixing Diego's comments sure won't hurt, but otherwise I'm okay with this.

review: Approve

[Christian Röller (christian-roeller)]

> On Wed, Aug 10, 2011 at 03:28:27PM +0000, Christian Röller wrote:
> > Christian Röller has proposed merging lp:~christian-
> roeller/hipl/whitelisting into lp:hipl.
>
> Did you run
>
> make alltests
> tools/hipl_autobuild.sh

Yes I run make alltests. It seems to be OK, I get no errors and it results with "archives ready for distribution".
I am not really sure how to use the autobuild script. How and from which machine can I run this script manually to build my branch?
Can it come to problems, although make and make alltests run without ones?

>
> with this branch?
>
> > --- hipd/netdev.c 2011-05-25 09:22:19 +0000
> > +++ hipd/netdev.c 2011-08-10 15:28:20 +0000
> > @@ -99,19 +99,26 @@
> > -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)
>
> Please keep lines below 80 characters where easily possible, like here.

Done...

>
> More examples below...
>
> > @@ -1122,6 +1131,68 @@
> >
> > /**
> > + * This functions gives you the interface label for a given ip4 or ip6
> addr.
>
> IPv4, IPv6 address

Done...

>
> > + * @param addr address for which you want to know the label
> > + * @param label pointer where the function stores the label
> > + * @return one on success, zero on error and write the label in param label
>
> Is the label written in both cases?

Done...only if the address exists in the system

>
> > +int hip_find_label_for_address(const struct sockaddr *const addr, char
> *const label)
> > +{
> > +
> > + if (addr->sa_family == AF_INET) {
> > + s4_in = (const struct sockaddr_in *const) (addr);
> > + }
> > + if (addr->sa_family == AF_INET6) {
> > + s6_in = (const struct sockaddr_in6 *const) (addr);
> > + }
>
> An else will save you a comparison here.
>
> > + getifaddrs(&myaddrs);
> > + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> > + if (ifa->ifa_addr == NULL) {
> > + continue;
> > + }
> > + if (ifa->ifa_addr->sa_family != AF_INET && ifa->ifa_addr->sa_family
> != AF_INET6) {
> > + continue;
> > + }
>
> These two could be merged.
>
> > + if (ifa->ifa_addr->sa_family == AF_INET) {
> > + s4 = (struct sockaddr_in *) (ifa->ifa_addr);
>
> pointless ()
>
> > + if (ifa->ifa_addr->sa_family == AF_INET6) {
> > + s6 = (struct sockaddr_in6 *) (ifa->ifa_addr);
>
> pointless ()
>
> As above, if you use else here, you could save a comparison.
>
> > + if (!memcmp(s4_in, s4, sizeof(s4))) {
> > + if (strlen(ifa->ifa_name) <= sizeof(label) - 1) {
> > + strncpy(label, ifa->ifa_name, sizeof(label) - 1);
> > + res = 1;
> > + } else {
> > + res = 0;
> > + }
> > + break;
>
> > + if (!memcmp(s6_in, s6, sizeof(s6))) {
> > + if (strlen(ifa->ifa_name) <= sizeof(label) - 1) {
> > + strncpy(label, ifa->ifa_name, sizeof(label) - 1);
> > + res = 1;
> > + } else {
> > + ...

[Diego Biurrun (diego-biurrun)]

On Thu, Aug 11, 2011 at 11:47:26AM +0000, Christian Röller wrote:
> > On Wed, Aug 10, 2011 at 03:28:27PM +0000, Christian Röller wrote:
> > > Christian Röller has proposed merging lp:~christian-
> > roeller/hipl/whitelisting into lp:hipl.
> >
> > Did you run
> >
> > make alltests
> > tools/hipl_autobuild.sh
>
> Yes I run make alltests. It seems to be OK, I get no errors and it
> results with "archives ready for distribution". I am not really sure
> how to use the autobuild script. How and from which machine can I run
> this script manually to build my branch?

Did you try running or reading the script?

> Can it come to problems, although make and make alltests run without ones?

Yes.

Diego

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Thu, Aug 11, 2011 at 11:48:47AM +0000, Christian Röller wrote:
> Christian Röller has proposed merging lp:~christian-roeller/hipl/whitelisting into lp:hipl.
>
> --- hipd/netdev.c 2011-05-25 09:22:19 +0000
> +++ hipd/netdev.c 2011-08-11 11:48:38 +0000
> @@ -99,19 +99,27 @@
> 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);

This is an opportunity for breaking a line, as is

> @@ -119,17 +127,20 @@
>
> -static int hip_netdev_is_in_white_list(int if_index)
> +static int hip_netdev_is_in_white_list(const unsigned int if_index, const char *const device_name)

this one

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

and this one.

> @@ -1122,6 +1132,40 @@
>
> /**
> + * This functions gives you the interface label for a given IPv4 or IPv6 address.

This sentence explains why starting a sentence with "this sentence" is
redundant and weird.

> + * @param addr address for which you want to know the label
> + * @param label pointer where the function stores the label
> + * @return one on success, zero on error and write the label in param label
> + * if the given addr exists in the system

address

The usual convention is to return zero on success, not one.

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

long line

> +{
> + int res = 0;
> + 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)) {

long line

> @@ -1213,6 +1252,18 @@
>
> + /* find interface label for address and check if it is whitelisted or not */
> + if (is_add) {
> + char label[IF_NAMESIZE];
> + if (hip_find_label_for_address(addr, label)) {
> + if ((!hip_netdev_is_in_white_list(ifa->ifa_index, label))) {
> + HIP_DEBUG("Interface:<%s> is not whitelisted\n", label);
> + continue;

The conditions can be merged.

> --- hipd/netdev.h 2011-05-18 15:12:07 +0000
> +++ hipd/netdev.h 2011-08-11 11:48:38 +0000
> @@ -56,4 +56,6 @@
>
> +int hip_find_label_for_address(const struct sockaddr *const addr, char *const label);

long line

Diego

review: Needs Fixing

[Christian Röller (christian-roeller)]

> review needs-fixing
>
> On Thu, Aug 11, 2011 at 11:48:47AM +0000, Christian Röller wrote:
> > Christian Röller has proposed merging lp:~christian-
> roeller/hipl/whitelisting into lp:hipl.
> >
> > --- hipd/netdev.c 2011-05-25 09:22:19 +0000
> > +++ hipd/netdev.c 2011-08-11 11:48:38 +0000
> > @@ -99,19 +99,27 @@
> > 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);
>
> This is an opportunity for breaking a line, as is
>
> > @@ -119,17 +127,20 @@
> >
> > -static int hip_netdev_is_in_white_list(int if_index)
> > +static int hip_netdev_is_in_white_list(const unsigned int if_index, const
> char *const device_name)
>
> this one
>
> > + 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))) {
>
> and this one.
>
> > @@ -1122,6 +1132,40 @@
> >
> > /**
> > + * This functions gives you the interface label for a given IPv4 or IPv6
> address.
>
> This sentence explains why starting a sentence with "this sentence" is
> redundant and weird.
>
> > + * @param addr address for which you want to know the label
> > + * @param label pointer where the function stores the label
> > + * @return one on success, zero on error and write the label in param label
> > + * if the given addr exists in the system
>
> address
>
> The usual convention is to return zero on success, not one.
>
> > +int hip_find_label_for_address(const struct sockaddr *const addr, char
> *const label)
>
> long line
>
> > +{
> > + int res = 0;
> > + 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)) {
>
> long line
>
> > @@ -1213,6 +1252,18 @@
> >
> > + /* find interface label for address and check if it is
> whitelisted or not */
> > + if (is_add) {
> > + char label[IF_NAMESIZE];
> > + if (hip_find_label_for_address(addr, label)) {
> > + if ((!hip_netdev_is_in_white_list(ifa->ifa_index,
> label))) {
> > + HIP_DEBUG("Interface:<%s> is not whitelisted\n",
> label);
> > + continue;
>
> The conditions can be merged.
>
> > --- hipd/netdev.h 2011-05-18 15:12:07 +0000
> > +++ hipd/netdev.h 2011-08-11 11:48:38 +0000
> > @@ -56,4 +56,6 @@
> >
> > +int hip_find_label_for_address(const struct sockaddr *const addr, char
> *const label);
>
> long line
>
> Diego

Everything done...
I also adapt the autobuild script to my local system conditi...

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christian!

Sorry for coming back late with required fixes :-(

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

[M] safety: this function is not very safe in how it handles the length of the memory buffer pointed to by 'label'. Please either document clearly how large the memory buffer needs to be and what happens if it is not or add code that helps to avoid buffer overflows here.

[M] policy: since this is a new function, it needs to be checked by a unit test. I know it's a PITA but unit tests, for example, immediately point to such issues as the memory buffer handling.

Regards,
 Stefan

review: Needs Fixing

[Christian Röller (christian-roeller)]

> Hi Christian!

Hi

>
> Sorry for coming back late with required fixes :-(

no problem...

>
> > /**
> > + * 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)
>
> [M] safety: this function is not very safe in how it handles the length of the
> memory buffer pointed to by 'label'. Please either document clearly how large
> the memory buffer needs to be and what happens if it is not or add code that
> helps to avoid buffer overflows here.

What is exactly the safety problem here? Do you mean I should additionally mention that label
sould be at least as big as IF_NAMESIZE, so that every possible interfacelabel length would fit?
Because right now I check whether the found interfacelabel is not bigger than the associated memory for label. And only if this is the case I will write to label. And I never write more than sizeof(label) bytes to label. Is this not enough to avoid a buffer overflow?
Right now the function will just return 1 if label would be to small. Maybe I should introduce here a special return value to make the debugging easier for the caller. What do you think?

>
> [M] policy: since this is a new function, it needs to be checked by a unit
> test. I know it's a PITA but unit tests, for example, immediately point to
> such issues as the memory buffer handling.
>
> Regards,
> Stefan

Christian

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christian!

> > > +int hip_find_label_for_address(const struct sockaddr *const addr,
> > > + char *const label)
> >
> > [M] safety: this function is not very safe in how it handles the length of
[...]
>
> What is exactly the safety problem here?

Nothing - sorry, I simply misread the code.

Stefan

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Mon, Aug 22, 2011 at 11:47:44AM +0000, Christian Röller wrote:
> Christian Röller has proposed merging lp:~christian-roeller/hipl/whitelisting into lp:hipl.

The word is "whitelist", not "white list". This is not exactly very
important, but since you are changing some parts of it, you might fix it
on the lines you are touching anyway.

> --- hipd/netdev.c 2011-08-15 14:11:56 +0000
> +++ hipd/netdev.c 2011-08-22 11:47:21 +0000
> @@ -97,19 +97,28 @@
> -static int hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
> -static int hip_netdev_white_list_count = 0;
> +struct hip_netdev_whiteliste_entry {
> + unsigned int if_index;
> + char if_label[IF_NAMESIZE];
> +};
> +static struct hip_netdev_whiteliste_entry hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
> +static unsigned int hip_netdev_white_list_count = 0;

whitelist, not whitelistE

> /**
> - * Add a network interface index number to the list of white listed
> + * Add a network interface index number plus label to the list of white listed

whitelisted

> * @param if_index the network interface index to be white listed
> + * @param device_name the network interface label to be white listed

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)

whitelist

> @@ -117,17 +126,22 @@
>
> /**
> - * Test if the given network interface index is white listed.
> + * Test if the given network interface index plus label is white listed.

whitelisted

> @@ -1120,6 +1133,40 @@
>
> /**
> + * 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

nit: please vertically align the descriptions.

Diego

review: Needs Fixing

[Christian Röller (christian-roeller)]

> review needs-fixing
>
> On Mon, Aug 22, 2011 at 11:47:44AM +0000, Christian Röller wrote:
> > Christian Röller has proposed merging lp:~christian-
> roeller/hipl/whitelisting into lp:hipl.
>
> The word is "whitelist", not "white list". This is not exactly very
> important, but since you are changing some parts of it, you might fix it
> on the lines you are touching anyway.
>
> > --- hipd/netdev.c 2011-08-15 14:11:56 +0000
> > +++ hipd/netdev.c 2011-08-22 11:47:21 +0000
> > @@ -97,19 +97,28 @@
> > -static int hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
> > -static int hip_netdev_white_list_count = 0;
> > +struct hip_netdev_whiteliste_entry {
> > + unsigned int if_index;
> > + char if_label[IF_NAMESIZE];
> > +};
> > +static struct hip_netdev_whiteliste_entry
> hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
> > +static unsigned int hip_netdev_white_list_count = 0;
>
> whitelist, not whitelistE
>
> > /**
> > - * Add a network interface index number to the list of white listed
> > + * Add a network interface index number plus label to the list of white
> listed
>
> whitelisted
>
> > * @param if_index the network interface index to be white listed
> > + * @param device_name the network interface label to be white listed
>
> 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)
>
> whitelist
>
> > @@ -117,17 +126,22 @@
> >
> > /**
> > - * Test if the given network interface index is white listed.
> > + * Test if the given network interface index plus label is white listed.
>
> whitelisted
>
> > @@ -1120,6 +1133,40 @@
> >
> > /**
> > + * 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
>
> nit: please vertically align the descriptions.
>
> Diego

Correct the typos...

Christian

[Diego Biurrun (diego-biurrun)]

 review approve

On Mon, Aug 22, 2011 at 07:05:35PM +0000, Christian Röller wrote:
> Christian Röller has proposed merging lp:~christian-roeller/hipl/whitelisting into lp:hipl.
>
> --- hipd/netdev.c 2011-08-15 14:11:56 +0000
> +++ hipd/netdev.c 2011-08-22 19:05:27 +0000
> @@ -1120,6 +1133,40 @@
> }
>
> /**
> + * 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

What I tried to suggest here was that you could vertically align the
descriptions, i.e.

  * @param addr address for which you want to know the label
  * @param label pointer where the function stores the label

Witness how "address" and "pointer" start on the same column.
Feel free to implement or ignore this nitpick at your discretion.

Diego

review: Approve

[René Hummen (rene-hummen)]

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?

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

> /**
> + * 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;
> + }
>...

[René Hummen (rene-hummen)]

review: Needs Information

[Christian Röller (christian-roeller)]

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

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christian!

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

[M] policy: since this is a new function, it needs to be checked by a unit test.

Regards,
 Stefan

review: Needs Fixing

[Christian Röller (christian-roeller)]

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

[Christian Röller (christian-roeller)]

I have added the unit tests for the new function.

Regarding this I have updated the branch to have a latest code version and edit the Makefile.am to let the tests compile.

Additionally it occurs to me, that I have to change the hip_find_label_for_address function, because the "sizeof-check", I did before to determine the size of the passed label parameter, turns out to be pointless. For that reason I added a new parameter to the function, which indicates the size of the passed char pointer.

This will maybe not solve the problem of a buffer overflow, because the responsibility of the value of the new parameter is based on the programmer, but it could be helpful to not forget to choose the value big enough.

Just tell me what you think about it...

[Christof Mroz (christof-mroz)]

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

review: Needs Fixing

[Christian Röller (christian-roeller)]

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

[Christof Mroz (christof-mroz)]

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

[Christian Röller (christian-roeller)]

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

[Christof Mroz (christof-mroz)]

On Tue, 13 Sep 2011 21:09:23 +0200, Christian Röller
<email address hidden> wrote:

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

That's the point of using an assertion versus a regular check: if
performance matters (in release builds etc.), you should be able to
undef the HIP_ASSERT macro (and thus don't compile the checks) without
breaking anything. In this sense, assertions don't cost anything.

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

Just tried it myself, and you're right: no sanity checks when passing
buffers around. Not even bounds checking works for parameter arrays;
it's as if the size info is discarded altogether. Which can't be true
because the compiler needs to carry it around for multidimensional
arrays, where all but one dimension must be specified. Meh, never
trust compilers...

Why did I think it would work in the first place? Because gcc actually
does (static, i.e. no-cost) bounds-checking e.g. for stack-allocated
arrays if told so. But Parameters are a whole other affair, it seems.

That said: feel free to omit the size, though I'd say it improves
documentation.

[Diego Biurrun (diego-biurrun)]

On Tue, Sep 13, 2011 at 05:43:28PM +0000, Christian Röller 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.
>
> 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 you are essentially saying that if HIPL gets ported to something else
than Linux, this will possibly introduce a subtle and hard to find bug? :)

Diego

[Diego Biurrun (diego-biurrun)]

On Tue, Sep 13, 2011 at 03:41:04PM +0000, Christof Mroz wrote:
>
> > +
> > + 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.

good catch!

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

You can find this all over HIPL, annoying as heck if you ask me :)

Somebody should open a bug about this and add it to the eternal todo
list... :)

Diego

[Diego Biurrun (diego-biurrun)]

On Tue, Sep 13, 2011 at 06:34:23PM +0000, Christof Mroz wrote:
> On Tue, 13 Sep 2011 19:43:28 +0200, Christian Röller
> <email address hidden> wrote:
>
> >> 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'm for finishing it right away on trunk, outside the scope of this
branch :)

Diego

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Tue, Sep 13, 2011 at 12:20:34PM +0000, Christian Röller wrote:
>
> === modified file 'Makefile.am'
> --- Makefile.am 2011-08-10 15:03:07 +0000
> +++ Makefile.am 2011-09-13 12:20:25 +0000
> @@ -71,11 +71,15 @@
> TESTS = test/check_firewall \
> test/check_lib_core \
> test/check_lib_tool \
> - test/check_modules_midauth
> + test/check_modules_midauth \
> + test/check_lib_tool \
> + test/check_hipd
> check_PROGRAMS = test/check_firewall \
> test/check_lib_core \
> test/check_lib_tool \
> - test/check_modules_midauth
> + test/check_modules_midauth \
> + test/check_lib_tool \
> + test/check_hipd

duplicate entries

alphabetical order

> @@ -207,6 +211,10 @@
>
> +test_check_hipd_SOURCES = test/check_hipd.c \
> + test/hipd/netdev.c \
> + modules/midauth/hipd/midauth.c \
> + $(hipd_hipd_sources)

Please align the '\' vertically, same as in the rest of the file.

> test_check_firewall_SOURCES = test/check_firewall.c \
> test/mocks.c \

Like here for example :)

> --- hipd/netdev.c 2011-08-15 14:11:56 +0000
> +++ hipd/netdev.c 2011-09-13 12:20:25 +0000
> @@ -90,54 +90,68 @@
>
> -static int hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
> -static int hip_netdev_white_list_count = 0;
> +struct hip_netdev_whitelist_entry {
> + unsigned int if_index;
> + char if_label[IF_NAMESIZE];
> +};
> +static struct hip_netdev_whitelist_entry hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
> +static unsigned int hip_netdev_white_list_count = 0;
>
> -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)

These unnecessary double prefixes (hip_ and netdev_) for symbols not
visible outside of this file are annoying.

In a perfect world, you would do a quick search and replace on trunk and
merge that back into your branch, but I don't want to pile too much
extra work on you. However, if you have a minute and the motivation..

Maybe you could drop them for the new code, but feel free to ignore me.

> -static int hip_netdev_is_in_white_list(int if_index)
> +static int hip_netdev_is_in_white_list(const unsigned int if_index,
> + const char *const device_name)

Oh look, this file only had a hip_ prefix - yay for consistency in HIPL,
even in the same file :)

> @@ -1120,6 +1133,44 @@
>
> /**
> + * 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.
> + * @param sizeoflabel Size of the passed label parameter, should be chosen big enough
> + * to be able to store...

review: Needs Fixing

[Christian Röller (christian-roeller)]

> On Tue, Sep 13, 2011 at 05:43:28PM +0000, Christian Röller 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.
> >
> > 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 you are essentially saying that if HIPL gets ported to something else
> than Linux, this will possibly introduce a subtle and hard to find bug? :)
>
> Diego

I had to limit the buffer length somehow, so I choose an OS-specific characteristic.
Is there a better way?

But isn't the whole whitelisting approach very os-specific(in this case linux)? I mean we base the whole whitelisting on OS-specific characteristics like interface-indeces and interface-label-names.
So maybe the whole whitelisting approach is a bug in this manner!? :)

But to be honest, I have no other idea than to treat the whitelisting separately for every OS. You?

[Christian Röller (christian-roeller)]

I have submitted the code a cosmetic treatment and tried to implement all your improvement suggestions. Maybe you can have another look when you find the time...thanks

[Christof Mroz (christof-mroz)]

Almost there:

> + assert(strlen(device_name) < sizeof(whitelist[0].if_label));
> + if (whitelist_count < HIP_NETDEV_MAX_WHITE_LIST) {
> + whitelist[whitelist_count].if_index = if_index;
> + strncpy(whitelist[whitelist_count].if_label,
> + device_name, sizeof(whitelist[0].if_label));
> + whitelist_count++;

There's a HIP_ASSERT() macro that takes care of logging violations, and it's
correctly disabled in non-debug builds.

> - /* We should NEVER run out of white list slots!!! */
> - HIP_DIE("Error: ran out of space for white listed interfaces!\n");
> + /* We should NEVER run out of whitelist slots!!! */
> + HIP_DIE("Error: ran out of space for whitelisted interfaces!\n");

I know you didn't introduce the comment, but it's stupid nevertheless.

> - strncpy(ifr.ifr_name, device_name, IF_NAMESIZE);
> + strncpy(ifr.ifr_name, device_name, sizeof(ifr.ifr_name));

Would a HIP_ASSERT be appropriate here as well?

> * @return interface index if the network address is bound to one, zero if
> - * no interface index was found.
> + * no interface index was found.
> */

Unrelated: is zero a valid index, according to netlink? eth0 is, after all.
Changing the return value on failure to a negative number won't hurt
in any case.

> +START_TEST(test_hip_netdev_find_label_for_address_parameters)
> +{
> + struct ifaddrs *myaddrs, *ifa = NULL;
> + char label[IF_NAMESIZE];
> +
> + /* The passed label parameter for the function hip_find_label_for_address
> + * must be at least 1 greater than the found label in the function. So
> + * a size of 1 would be for sure too short to store at least the shortest
> + * label of only 1 character.
> + */
> + char label_too_short[1];
> +
> + getifaddrs(&myaddrs);

You should warn that this test was skipped if getifaddrs() doesn't return anything.
Using mock functions would be more correct, but in this case it's probably not
worth the effort.

review: Needs Fixing

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Wed, Sep 14, 2011 at 12:50:08PM +0000, Christian Röller wrote:
> Christian Röller has proposed merging lp:~christian-roeller/hipl/whitelisting into lp:hipl.

The size of this merge proposal has grown from 19k to 30k overnight.
This is a problem. Note that review time grows more than linearly
with change size. Note further that most of the reviewers work on
HIPL in their free time.

You have added many completely unrelated changes to this merge
proposal. I know your intentions are good, but unfortunately
the result is not.

This merge proposal is about whitelisting. Changes not related to
whitelisting do *not* belong here. They should either go in a
separate merge proposal or be committed directly to trunk.

I bet you have only a single branch and do all your work in there, right?
This is a very common workstyle for people coming from centralized
revision control systems, but not a good way to work with Bazaar.
Start unleashing its power and branch merrily. I have had days where
I created and threw away a dozen or more branches.

Now please go ahead and create a separate branch for your whitespace
changes. You will be surprised how quickly that will get approved
and how much quicker your whitelisting branch will get approval :)

Happy branching, Diego

review: Needs Fixing

[Stefan Götz (stefan.goetz-deactivatedaccount)]

I second Christof's comments; please address them. As for Diego's comments, I agree wholeheartedly but I'd let you get away with a 'keep unrelated changes out of your next branch/merge proposal'. No further comments from my side.

review: Needs Fixing

[Diego Biurrun (diego-biurrun)]

On Wed, Sep 14, 2011 at 07:43:47PM +0000, Stefan Götz wrote:
> Review: Needs Fixing
>
> As for Diego's comments, I agree wholeheartedly but I'd let you get
> away with a 'keep unrelated changes out of your next branch/merge
> proposal'.

All of this really should not be that hard with the help of the rebase,
merge and replay bzr commands or even with diff and patch. The more
practice one gets using these tools, the better :)

In all the projects I worked, unrelated changes in a patch were an
automatic reject, no further questions asked...

Diego

[Christian Röller (christian-roeller)]

> On Wed, Sep 14, 2011 at 07:43:47PM +0000, Stefan Götz wrote:
> > Review: Needs Fixing
> >
> > As for Diego's comments, I agree wholeheartedly but I'd let you get
> > away with a 'keep unrelated changes out of your next branch/merge
> > proposal'.
>
> All of this really should not be that hard with the help of the rebase,
> merge and replay bzr commands or even with diff and patch. The more
> practice one gets using these tools, the better :)
>
> In all the projects I worked, unrelated changes in a patch were an
> automatic reject, no further questions asked...
>
> Diego

I totally agree to that Diego, it was no good idea to mix up here different things.
I just did it because I have changed all comments for the whitelisting relevant functions
and just wanted to take care of consistency in the whole file.

You are right a little bit more practice with bazaar couldn't be bad :),
because this is the first time I really use it. But is it really worthwhile this time
to create an own branch for it? I mean I would just change a few comment alignment issues in one file. But as I said in the beginning I agree to that in principle.
So if it is wished, I will try to do it. Even though this could take a few days, because I have to
do first a lot of other things this week.

Regards,
Christian

[Diego Biurrun (diego-biurrun)]

On Thu, Sep 15, 2011 at 01:43:18PM +0000, Christian Röller wrote:
> > On Wed, Sep 14, 2011 at 07:43:47PM +0000, Stefan Götz wrote:
> > > Review: Needs Fixing
> > >
> > > As for Diego's comments, I agree wholeheartedly but I'd let you get
> > > away with a 'keep unrelated changes out of your next branch/merge
> > > proposal'.
> >
> > All of this really should not be that hard with the help of the rebase,
> > merge and replay bzr commands or even with diff and patch. The more
> > practice one gets using these tools, the better :)
> >
> > In all the projects I worked, unrelated changes in a patch were an
> > automatic reject, no further questions asked...
>
> I totally agree to that Diego, it was no good idea to mix up here different things.
> I just did it because I have changed all comments for the whitelisting relevant functions
> and just wanted to take care of consistency in the whole file.
>
> You are right a little bit more practice with bazaar couldn't be bad :),
> because this is the first time I really use it. But is it really worthwhile this time
> to create an own branch for it?

Yes.

> I mean I would just change a few comment alignment issues in one file.
> But as I said in the beginning I agree to that in principle.
> So if it is wished, I will try to do it. Even though this could take a
> few days, because I have to do first a lot of other things this week.

It really should not take you long. You can poke me on IRC if you have
trouble.

Diego

[René Hummen (rene-hummen)]

On 15.09.2011, at 15:43, Christian Röller wrote:
>> On Wed, Sep 14, 2011 at 07:43:47PM +0000, Stefan Götz wrote:
>>> Review: Needs Fixing
>>>
>>> As for Diego's comments, I agree wholeheartedly but I'd let you get
>>> away with a 'keep unrelated changes out of your next branch/merge
>>> proposal'.
>>
>> All of this really should not be that hard with the help of the rebase,
>> merge and replay bzr commands or even with diff and patch. The more
>> practice one gets using these tools, the better :)
>>
>> In all the projects I worked, unrelated changes in a patch were an
>> automatic reject, no further questions asked...
>>
>> Diego
>
> I totally agree to that Diego, it was no good idea to mix up here different things.
> I just did it because I have changed all comments for the whitelisting relevant functions
> and just wanted to take care of consistency in the whole file.
>
> You are right a little bit more practice with bazaar couldn't be bad :),
> because this is the first time I really use it. But is it really worthwhile this time
> to create an own branch for it? I mean I would just change a few comment alignment issues in one file. But as I said in the beginning I agree to that in principle.
> So if it is wished, I will try to do it. Even though this could take a few days, because I have to
> do first a lot of other things this week.

Please address Christof's comments. I agree with Stefan that for this time your branch is ok, but please take care to only propose related issues.

Ciao,
René

--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: [phone]
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/

Unmerged revisions

5999. By Christian Röller on 2011-09-14

Cosmetic issues in all files.

5998. By Christian Röller on 2011-09-14

Cosmetic issues in both files.

5997. By Christian Röller on 2011-09-13

A few cosmetic changes and correct one mistake regarding the use of sizeof.

5996. By Christian Röller on 2011-09-13

Rebase with trunk.

5995. By Christian Röller on 2011-08-30

Add Unit-Tests for the new function hip_find_label_for_address in /hipd/netdev.c

5994. By Christian Röller on 2011-08-24

Increase the n by 1 in strncpy regarding the writing of the interfacelabel to guarantee null-termination.

5993. By Christian Röller on 2011-08-23

cosmetic: alignments in function description hip_find_label_for_address

5992. By Christian Röller on 2011-08-22

cosmetic: correct some typos

5991. By Christian Röller on 2011-08-12

cosmetic: typo in function description

5990. By Christian Röller on 2011-08-12

cosmetic: shorten a few things

=== modified file 'Makefile.am'
--- Makefile.am	2011-08-10 15:03:07 +0000
+++ Makefile.am	2011-09-14 12:49:45 +0000
@@ -68,13 +68,15 @@
 
 ### tests ###
 if HIP_UNITTESTS
-TESTS           = test/check_firewall \
-                  test/check_lib_core \
-                  test/check_lib_tool \
-                  test/check_modules_midauth
-check_PROGRAMS  = test/check_firewall \
-                  test/check_lib_core \
-                  test/check_lib_tool \
+TESTS           = test/check_firewall  \
+                  test/check_hipd      \
+                  test/check_lib_core  \
+                  test/check_lib_tool  \
+                  test/check_modules_midauth	
+check_PROGRAMS  = test/check_firewall  \
+                  test/check_hipd      \
+                  test/check_lib_core  \
+                  test/check_lib_tool  \
                   test/check_modules_midauth
 endif
 
@@ -207,6 +209,10 @@
 lib_core_libhipcore_la_SOURCES += lib/core/performance.c
 endif
 
+test_check_hipd_SOURCES = modules/midauth/hipd/midauth.c  \
+                          test/check_hipd.c               \
+                          test/hipd/netdev.c              \
+                          $(hipd_hipd_sources)
 
 test_check_firewall_SOURCES = test/check_firewall.c          \
                               test/mocks.c                   \
@@ -240,6 +246,7 @@
 hipd_hipd_LDADD                          = lib/core/libhipcore.la
 test_auth_performance_LDADD              = lib/core/libhipcore.la
 test_check_firewall_LDADD                = lib/core/libhipcore.la
+test_check_hipd_LDADD                    = lib/core/libhipcore.la
 test_check_lib_core_LDADD                = lib/core/libhipcore.la
 test_check_lib_tool_LDADD                = lib/core/libhipcore.la
 test_check_modules_midauth_LDADD         = lib/core/libhipcore.la

=== modified file 'hipd/input.c'
--- hipd/input.c	2011-08-25 12:12:27 +0000
+++ hipd/input.c	2011-09-14 12:49:45 +0000
@@ -521,7 +521,7 @@
         /* RVS/Relay is handled later in the code. */
         if (hip_relay_get_status() == HIP_RELAY_OFF)
 #endif
-            return -1;
+        return -1;
     }
 
     /* Debug printing of received packet information. All received HIP

=== modified file 'hipd/netdev.c'
--- hipd/netdev.c	2011-08-15 14:11:56 +0000
+++ hipd/netdev.c	2011-09-14 12:49:45 +0000
@@ -53,6 +53,7 @@
 #include <openssl/rand.h>
 #include <sys/ioctl.h>
 #include <linux/rtnetlink.h>
+#include <assert.h>
 
 #include "lib/core/builder.h"
 #include "lib/core/common.h"
@@ -79,7 +80,7 @@
 
 /**
  * We really don't expect more than a handfull of interfaces to be on
- * our white list.
+ * our whitelist.
  */
 #define HIP_NETDEV_MAX_WHITE_LIST 5
 
@@ -90,43 +91,56 @@
 #define FAM_STR_MAX               32
 
 /**
- * This is the white list. For every interface, which is in our white list,
+ * This is the whitelist. For every interface, which is in our whitelist,
  * this array has a fixed size, because there seems to be no need at this
  * moment to deal with dynamic memory - which would complicate the code
  * and cost size and performance at least equal if not more to this fixed
  * size array.
  * Free slots are signaled by the value -1.
  */
-static int hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
-static int hip_netdev_white_list_count = 0;
+struct whitelist_entry {
+    unsigned int if_index;
+    char         if_label[IF_NAMESIZE];
+};
+static struct whitelist_entry whitelist[HIP_NETDEV_MAX_WHITE_LIST];
+static unsigned int           whitelist_count = 0;
 
 /**
- * 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 whitelist_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;
+    assert(strlen(device_name) < sizeof(whitelist[0].if_label));
+    if (whitelist_count < HIP_NETDEV_MAX_WHITE_LIST) {
+        whitelist[whitelist_count].if_index = if_index;
+        strncpy(whitelist[whitelist_count].if_label,
+                device_name, sizeof(whitelist[0].if_label));
+        whitelist_count++;
     } else {
-        /* We should NEVER run out of white list slots!!! */
-        HIP_DIE("Error: ran out of space for white listed interfaces!\n");
+        /* We should NEVER run out of whitelist slots!!! */
+        HIP_DIE("Error: ran out of space for whitelisted interfaces!\n");
     }
 }
 
 /**
- * Test if the given network interface index is white listed.
+ * Test if the given network interface index plus label is whitelisted.
  *
- * @param if_index the index of the network interface to be tested
- * @return 1 if the index is whitelisted or zero otherwise
+ * @param if_index    the index of the network interface to be tested
+ * @param device_name the label of the network interface to be tested
+ * @return            1 if the index is whitelisted or zero otherwise
  */
-static int hip_netdev_is_in_white_list(int if_index)
+static int is_in_whitelist(const unsigned int if_index,
+                           const char *const device_name)
 {
-    int i = 0;
-    for (i = 0; i < hip_netdev_white_list_count; i++) {
-        if (hip_netdev_white_list[i] == if_index) {
+    for (unsigned int i = 0; i < whitelist_count; i++) {
+        if (whitelist[i].if_index == if_index &&
+            !strncmp(whitelist[i].if_label,
+                     device_name, sizeof(whitelist[0].if_label))) {
             return 1;
         }
     }
@@ -134,11 +148,11 @@
 }
 
 /**
- * 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
- * @return 1 on success, 0 on error
+ * @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));
     sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
 
     if (ioctl(sock, SIOCGIFINDEX, &ifr) == 0) {
         ret = 1;
-        hip_netdev_white_list_add_index(ifr.ifr_ifindex);
-        HIP_DEBUG("Adding device <%s> to white list with index <%i>.\n",
+        whitelist_add_index_and_name(ifr.ifr_ifindex, device_name);
+        HIP_DEBUG("Adding device <%s> to whitelist with index <%i>.\n",
                   device_name,
                   ifr.ifr_ifindex);
     } else {
         ret = 0;
     }
-
     if (sock) {
         close(sock);
     }
@@ -169,7 +182,7 @@
  * hash function for the addresses hash table
  *
  * @param ptr a pointer to a netdev_address structure
- * @return the calculated hash to index the parameter
+ * @return    the calculated hash to index the parameter
  */
 static unsigned long hip_netdev_hash(const void *ptr)
 {
@@ -195,7 +208,7 @@
  *
  * @param ptr1 a pointer to a netdev_address structure
  * @param ptr2 a pointer to a netdev_address structure
- * @return 0 if the given pointers match or 1 otherwise
+ * @return     0 if the given pointers match or 1 otherwise
  */
 static int hip_netdev_match(const void *ptr1, const void *ptr2)
 {
@@ -300,7 +313,7 @@
  * parties should have a matching IP address family.
  *
  * @param addr addr the address to be tested (IPv4 address in IPv6 mapped format) for family
- * @return one if the address is recorded in the cache and zero otherwise
+ * @return     one if the address is recorded in the cache and zero otherwise
  */
 static int hip_exists_address_family_in_list(const struct in6_addr *addr)
 {
@@ -323,10 +336,10 @@
 /**
  * Test if the given address with the given network interface index exists in the cache
  *
- * @param addr A sockaddr structure containing the address to be checked. An IPv6 socket
- *             address structure can also contain an IPv4 address in IPv6-mapped format.
+ * @param addr    A sockaddr structure containing the address to be checked. An IPv6 socket
+ *                address structure can also contain an IPv4 address in IPv6-mapped format.
  * @param ifindex the network interface index
- * @return one if the index exists in the cache or zero otherwise
+ * @return        one if the index exists in the cache or zero otherwise
  */
 int hip_exists_address_in_list(struct sockaddr *addr, int ifindex)
 {
@@ -393,9 +406,9 @@
  * advertisements (UPDATE control message with a LOCATOR parameter) to
  * peers.
  *
- * @param addr a pointer to a socket address structure.
+ * @param addr    a pointer to a socket address structure.
  * @param ifindex network device interface index.
- * @param flags flags
+ * @param flags   flags
  */
 void hip_add_address_to_list(struct sockaddr *addr, int ifindex, int flags)
 {
@@ -442,8 +455,8 @@
 /**
  * Delete an address from address cache of localhost addresses
  *
- * @param addr A sockaddr structure containing the address to be deleted.
- *             IPv4 addresses can be in IPv6-mapped format.
+ * @param addr    A sockaddr structure containing the address to be deleted.
+ *                IPv4 addresses can be in IPv6-mapped format.
  * @param ifindex the network interface on which the address is attached to
  */
 static void hip_delete_address_from_list(struct sockaddr *addr, int ifindex)
@@ -522,10 +535,10 @@
 /**
  * Get the interface index of a socket address.
  *
- * @param addr a pointer to a socket address whose interface index is to be
- *              searched.
+ * @param addr       a pointer to a socket address whose interface index is to be
+ *                   searched.
  * @return interface index if the network address is bound to one, zero if
- *         no interface index was found.
+ *                   no interface index was found.
  */
 static int hip_netdev_find_if(struct sockaddr *addr)
 {
@@ -587,12 +600,12 @@
  * a function that gets the ifindex of the network device which has the address
  * @c addr.
  *
- * @param  addr a pointer to an IPv6 address whose interface index is to be
- *              searched.
- * @return interface index if the network address is bound to one, zero if
- *         no interface index was found and negative in error case.
- * @todo The caller of this should be generalized to both IPv4 and IPv6
- *       so that this function can be removed (tkoponen).
+ * @param addr a pointer to an IPv6 address whose interface index is to be
+ *             searched.
+ * @return     interface index if the network address is bound to one, zero if
+ *             no interface index was found and negative in error case.
+ * @todo       The caller of this should be generalized to both IPv4 and IPv6
+ *             so that this function can be removed (tkoponen).
  */
 int hip_devaddr2ifindex(struct in6_addr *addr)
 {
@@ -606,9 +619,9 @@
  * Initialize the address cache of localhost addresses
  *
  * @return zero on success and non-zero on error
- * @todo This creates a new NETLINK socket (via getifaddrs), so this has to be
- *       run before the global NETLINK socket is opened. We did not have the time
- *       and energy to import all of the necessary functionality from iproute2.
+ * @todo   This creates a new NETLINK socket (via getifaddrs), so this has to be
+ *         run before the global NETLINK socket is opened. We did not have the time
+ *         and energy to import all of the necessary functionality from iproute2.
  */
 int hip_netdev_init_addresses(void)
 {
@@ -632,7 +645,7 @@
         HIP_IFEL(!(if_index = if_nametoindex(g_iface->ifa_name)),
                  -1, "if_nametoindex failed\n");
         /* Check if our interface is in the whitelist */
-        if ((hip_netdev_white_list_count > 0) && (!hip_netdev_is_in_white_list(if_index))) {
+        if ((!is_in_whitelist(if_index, g_iface->ifa_name))) {
             continue;
         }
 
@@ -650,12 +663,12 @@
  * Try to map a given HIT or an LSI to a routable IP address using local host association
  * data base, hosts files or DNS (in the presented order).
  *
- * @param hit a HIT to map to a LSI
- * @param lsi an LSI to map to an IP address
+ * @param hit  a HIT to map to a LSI
+ * @param lsi  an LSI to map to an IP address
  * @param addr output argument to which this function writes the address if found
- * @return zero on success and non-zero on error
- * @note Either HIT or LSI must be given. If both are given, the HIT is preferred.
- * @todo move this to some other file (this file contains local IP address management, not remote)
+ * @return     zero on success and non-zero on error
+ * @note       Either HIT or LSI must be given. If both are given, the HIT is preferred.
+ * @todo       move this to some other file (this file contains local IP address management, not remote)
  */
 int hip_map_id_to_addr(const hip_hit_t *hit, const hip_lsi_t *lsi,
                        struct in6_addr *addr)
@@ -726,21 +739,21 @@
  * arguments. This function also supports HIP-based loopback
  * connectivity and hiccups (data packet) extensions.
  *
- * @param src_hit_in The source HIT for the I1. Alternatively, NULL if default
- *                   HIT is suitable
- * @param dst_hit_in The destination HIT. This HIT cannot be a "pseudo HIT" as
- *                   used by the opportunistic mode. Use hip_send_i1() function
- *                   instead with opportunistic mode.
- * @param src_lsi_in Optional source LSI corresponding to the source HIT
- * @param dst_lsi_in Optional destination LSI corresponding to the destination HIT
+ * @param src_hit_in  The source HIT for the I1. Alternatively, NULL if default
+ *                    HIT is suitable
+ * @param dst_hit_in  The destination HIT. This HIT cannot be a "pseudo HIT" as
+ *                    used by the opportunistic mode. Use hip_send_i1() function
+ *                    instead with opportunistic mode.
+ * @param src_lsi_in  Optional source LSI corresponding to the source HIT
+ * @param dst_lsi_in  Optional destination LSI corresponding to the destination HIT
  * @param src_addr_in Source address for the I1 (IPv4 address in IPv6 mapped format)
  * @param dst_addr_in Destination address for the I1 (IPv4 address in IPv6 mapped format)
- * @return zero on success and non-zero on error
- * @note HITs can be NULL if the LSIs are non-NULL (and vice versa).
- * @note The locators (addresses) can be NULL. This function will
- *       try to map the HITs or LSIs to IP addresses. IPv4 broadcast
- *       will be used as a last resort.
- * @todo move this function to some other file
+ * @return            zero on success and non-zero on error
+ * @note              HITs can be NULL if the LSIs are non-NULL (and vice versa).
+ * @note              The locators (addresses) can be NULL. This function will
+ *                    try to map the HITs or LSIs to IP addresses. IPv4 broadcast
+ *                    will be used as a last resort.
+ * @todo              move this function to some other file
  */
 static int hip_netdev_trigger_bex(const hip_hit_t *src_hit_in,
                                   const hip_hit_t *dst_hit_in,
@@ -966,8 +979,8 @@
  * Handle an "acquire" message from the kernel by triggering a base exchange.
  *
  * @param msg a netlink "acquire" message
- * @return zero on success and non-zero on error
- * @todo move this to some other file
+ * @return    zero on success and non-zero on error
+ * @todo      move this to some other file
  */
 static int hip_netdev_handle_acquire(struct nlmsghdr *msg)
 {
@@ -1012,8 +1025,8 @@
  *
  * @param msg the HIP user message containing HITs, LSIs and addresses as
  *            parameters
- * @return zero on success and non-zero on error
- * @todo move this to some other file
+ * @return    zero on success and non-zero on error
+ * @todo      move this to some other file
  */
 int hip_netdev_trigger_bex_msg(const struct hip_common *msg)
 {
@@ -1120,6 +1133,44 @@
 }
 
 /**
+ * Returns 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.
+ *                   For the label should be at least 16 bytes allocated.
+ * @param label_size 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(at least 16 bytes).
+ */
+int hip_netdev_find_label_for_address(const struct sockaddr *const addr,
+                                      char *const label, const unsigned int label_size)
+{
+    int             res = 1;
+    struct ifaddrs *myaddrs, *ifa = NULL;
+
+    getifaddrs(&myaddrs);
+    for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
+        if (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) < label_size) {
+                strncpy(label, ifa->ifa_name, label_size);
+                res = 0;
+            }
+            break;
+        }
+    }
+    freeifaddrs(myaddrs);
+    return res;
+}
+
+/**
  * Netlink event handler. Handles IPsec acquire messages (triggering
  * of base exchange) and updates the cache of local addresses when
  * address changes occur.
@@ -1127,7 +1178,7 @@
  * @param msg a netlink message
  * @param len the length of the netlink message in bytes
  * @param arg argument to pass (needed because of the callaback nature)
- * @return zero on success and non-zero on error
+ * @return    zero on success and non-zero on error
  */
 int hip_netdev_event(struct nlmsghdr *msg, int len, UNUSED void *arg)
 {
@@ -1165,11 +1216,6 @@
             rta = IFA_RTA(ifa);
             l   = msg->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa));
 
-            /* Check if our interface is in the whitelist */
-            if ((hip_netdev_white_list_count > 0) &&
-                (!hip_netdev_is_in_white_list(ifindex))) {
-                continue;
-            }
 
             if ((ifa->ifa_family != AF_INET) &&
                 (ifa->ifa_family != AF_INET6)) {
@@ -1211,6 +1257,17 @@
                 HIP_DEBUG("Unknown addr family in addr\n");
             }
 
+            /* find interface label for address and check if it is whitelisted or not */
+            if (is_add) {
+                char label[IF_NAMESIZE];
+                if (!hip_netdev_find_label_for_address(addr, label, sizeof(label)) &&
+                    !is_in_whitelist(ifa->ifa_index, label)) {
+                    HIP_DEBUG("Interface:<%s> is not whitelisted\n", label);
+                    continue;
+                }
+            }
+
+
             /* Trying to add an existing address or deleting a non-existing
              * address */
             exists = hip_exists_address_in_list(addr, ifa->ifa_index);
@@ -1303,7 +1360,7 @@
  * @param local_hit The HIT to be removed.
  * @return          Zero on success, non-zero on failure.
  *
- * @see hip_manage_iface_local_hit()
+ * @see             hip_manage_iface_local_hit()
  */
 static int hip_remove_iface_local_hit(const hip_hit_t *const local_hit)
 {
@@ -1340,7 +1397,7 @@
  *               signature.
  * @return       Zero on success, non-zero on failure.
  *
- * @see hip_remove_iface_all_local_hits()
+ * @see          hip_remove_iface_all_local_hits()
  */
 static int hip_remove_iface_hit_by_host_id(struct local_host_id *entry,
                                            UNUSED void *opaque)
@@ -1360,7 +1417,7 @@
  * Add a route to a local HIT
  *
  * @param local_hit the local HIT for which a route should be added
- * @return zero on success and non-zero on error
+ * @return          zero on success and non-zero on error
  */
 int hip_add_iface_local_route(const hip_hit_t *local_hit)
 {
@@ -1387,7 +1444,7 @@
  *            will be in IPv6-mapped format.
  * @param dst The destination address. IPv4 addresses must be in
  *            in IPv6-mapped format.
- * @return zero on success and non-zero on failure
+ * @return    zero on success and non-zero on failure
  */
 int hip_select_source_address(struct in6_addr *src, const struct in6_addr *dst)
 {

=== modified file 'hipd/netdev.h'
--- hipd/netdev.h	2011-05-18 15:12:07 +0000
+++ hipd/netdev.h	2011-09-14 12:49:45 +0000
@@ -56,4 +56,7 @@
 int hip_map_id_to_addr(const hip_hit_t *hit, const hip_lsi_t *lsi,
                        struct in6_addr *addr);
 
+int hip_netdev_find_label_for_address(const struct sockaddr *const addr,
+                                      char *const label, const unsigned int label_size);
+
 #endif /* HIP_HIPD_NETDEV_H */

=== added file 'test/check_hipd.c'
--- test/check_hipd.c	1970-01-01 00:00:00 +0000
+++ test/check_hipd.c	2011-09-14 12:49:45 +0000
@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2011 Aalto University and RWTH Aachen University.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <check.h>
+#include <stdlib.h>
+
+#include "test/hipd/test_suites.h"
+
+int main(void)
+{
+    int      number_failed;
+    SRunner *sr = srunner_create(NULL);
+    srunner_add_suite(sr, hipd_netdev());
+
+    srunner_run_all(sr, CK_NORMAL);
+    number_failed = srunner_ntests_failed(sr);
+    srunner_free(sr);
+    return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+}

=== added directory 'test/hipd'
=== added file 'test/hipd/netdev.c'
--- test/hipd/netdev.c	1970-01-01 00:00:00 +0000
+++ test/hipd/netdev.c	2011-09-14 12:49:45 +0000
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2011 Aalto University and RWTH Aachen University.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <check.h>
+#include <stdlib.h>
+#include <ifaddrs.h>
+#include <net/if.h>
+
+#include "hipd/netdev.h"
+#include "lib/core/debug.h"
+#include "lib/core/prefix.h"
+#include "test_suites.h"
+
+
+
+START_TEST(test_hip_netdev_find_label_for_address_parameters)
+{
+    struct ifaddrs *myaddrs, *ifa = NULL;
+    char            label[IF_NAMESIZE];
+
+    /* The passed label parameter for the function hip_find_label_for_address
+     * must be at least 1 greater than the found label in the function. So
+     * a size of 1 would be for sure too short to store at least the shortest
+     * label of only 1 character.
+     */
+    char label_too_short[1];
+
+    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;
+        }
+        HIP_DEBUG("Size of chosen label: %d\n", sizeof(label));
+        if (ifa->ifa_addr->sa_family == AF_INET) {
+            HIP_DEBUG_INADDR("Try to find label for addr...",
+                             hip_cast_sa_addr((struct sockaddr *) (ifa->ifa_addr)));
+        } else {
+            HIP_DEBUG_IN6ADDR("Try to find label for addr...",
+                              hip_cast_sa_addr((struct sockaddr *) (ifa->ifa_addr)));
+        }
+
+        fail_unless(hip_netdev_find_label_for_address(ifa->ifa_addr,  label, sizeof(label)) == 0,
+                    "Could not find label although address must be known.");
+
+        HIP_DEBUG("Found: %s\n", label);
+
+        HIP_DEBUG("Size of chosen label: %d\n", sizeof(label_too_short));
+        HIP_DEBUG("Try to store in the too short chosen label parameter, return has to be 1\n");
+
+        fail_unless(hip_netdev_find_label_for_address(ifa->ifa_addr, label_too_short, sizeof(label_too_short)) == 1,
+                    "Not enough memory allocated for the label parameter.");
+
+        HIP_DEBUG("Return of function: %d \n",
+                  hip_netdev_find_label_for_address(ifa->ifa_addr, label_too_short, sizeof(label_too_short)));
+    }
+}
+END_TEST
+
+Suite *hipd_netdev(void)
+{
+    Suite *s = suite_create("hipd/netdev");
+
+    TCase *tc_netdev = tcase_create("netdev");
+    tcase_add_test(tc_netdev, test_hip_netdev_find_label_for_address_parameters);
+    suite_add_tcase(s, tc_netdev);
+
+    return s;
+}

=== added file 'test/hipd/test_suites.h'
--- test/hipd/test_suites.h	1970-01-01 00:00:00 +0000
+++ test/hipd/test_suites.h	2011-09-14 12:49:45 +0000
@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) 2011 Aalto University and RWTH Aachen University.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef HIP_TEST_HIPD_TEST_SUITES_H
+#define HIP_TEST_HIPD_TEST_SUITES_H
+
+#include <check.h>
+
+Suite *hipd_netdev(void);
+
+#endif /* HIP_TEST_HIPD_TEST_SUITES_H */

=== modified file 'test/lib/core/hostid.c'
--- test/lib/core/hostid.c	2011-09-05 08:46:53 +0000
+++ test/lib/core/hostid.c	2011-09-14 12:49:45 +0000
@@ -65,7 +65,7 @@
 START_TEST(test_serialize_deserialize_rsa_keys)
 {
     unsigned int            i, keyrr_len = 0;
-    int                     bits[3] = {1024, 2048, 3072};
+    int                     bits[3] = { 1024, 2048, 3072 };
     RSA                    *key     = NULL, *key_deserialized = NULL;
     EVP_PKEY               *key_a   = NULL, *key_b = NULL;
     unsigned char          *keyrr;