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

Proposed by Christian Röller
Status: Superseded
Proposed branch: lp:~christian-roeller/hipl/whitelisting
Merge into: lp:hipl
Diff against target: 464 lines (+265/-32)
8 files modified
Makefile.am (+11/-2)
hipd/input.c (+1/-1)
hipd/netdev.c (+85/-28)
hipd/netdev.h (+3/-0)
test/check_hipd.c (+41/-0)
test/hipd/netdev.c (+89/-0)
test/hipd/test_suites.h (+34/-0)
test/lib/core/hostid.c (+1/-1)
To merge this branch: bzr merge lp:~christian-roeller/hipl/whitelisting
Reviewer Review Type Date Requested Status
René Hummen Needs Information
Diego Biurrun Approve
Stefan Götz Pending
Review via email: mp+72487@code.launchpad.net

This proposal supersedes a proposal from 2011-08-22.

This proposal has been superseded by a proposal from 2011-09-13.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : Posted in a previous version of this proposal
Download full text (7.4 KiB)

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

Read more...

review: Needs Fixing
Revision history for this message
Miika Komu (miika-iki) wrote : Posted in a previous version of this proposal

Quick test report: seems to work without any troubles.

Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal
Download full text (9.2 KiB)

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

Read more...

Revision history for this message
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : Posted in a previous version of this proposal
Download full text (4.4 KiB)

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

Read more...

Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal
Download full text (5.1 KiB)

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

Read more...

Revision history for this message
Stefan Götz (stefan.goetz-deactivatedaccount) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

 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
Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal

> 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

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal

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

Revision history for this message
René Hummen (rene-hummen) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal

> 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

Revision history for this message
René Hummen (rene-hummen) wrote : Posted in a previous version of this proposal

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: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Diego Biurrun (diego-biurrun) : Posted in a previous version of this proposal
review: Needs Fixing
Revision history for this message
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : Posted in a previous version of this proposal

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

review: Approve
Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal
Download full text (3.3 KiB)

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

Read more...

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

 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
Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal
Download full text (3.2 KiB)

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

Read more...

Revision history for this message
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal

> 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

Revision history for this message
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Diego Biurrun (diego-biurrun) wrote : Posted in a previous version of this proposal

 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
Revision history for this message
Christian Röller (christian-roeller) wrote : Posted in a previous version of this proposal

> 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

Revision history for this message
Diego Biurrun (diego-biurrun) wrote :

 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
5993. By Christian Röller

cosmetic: alignments in function description hip_find_label_for_address

Revision history for this message
René Hummen (rene-hummen) wrote :
Download full text (3.5 KiB)

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

Read more...

Revision history for this message
René Hummen (rene-hummen) :
review: Needs Information
Revision history for this message
Christian Röller (christian-roeller) wrote :
Download full text (5.3 KiB)

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

Read more...

5994. By Christian Röller

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

Revision history for this message
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Christian Röller (christian-roeller) wrote :
Download full text (6.0 KiB)

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

Read more...

5995. By Christian Röller

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

5996. By Christian Röller

Rebase with trunk.

5997. By Christian Röller

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

5998. By Christian Röller

Cosmetic issues in both files.

5999. By Christian Röller

Cosmetic issues in all files.

Unmerged revisions

5999. By Christian Röller

Cosmetic issues in all files.

5998. By Christian Röller

Cosmetic issues in both files.

5997. By Christian Röller

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

5996. By Christian Röller

Rebase with trunk.

5995. By Christian Röller

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

5994. By Christian Röller

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

5993. By Christian Röller

cosmetic: alignments in function description hip_find_label_for_address

5992. By Christian Röller

cosmetic: correct some typos

5991. By Christian Röller

cosmetic: typo in function description

5990. By Christian Röller

cosmetic: shorten a few things

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Makefile.am'
--- Makefile.am 2011-08-10 15:03:07 +0000
+++ Makefile.am 2011-09-13 12:13:33 +0000
@@ -71,11 +71,15 @@
71TESTS = test/check_firewall \71TESTS = test/check_firewall \
72 test/check_lib_core \72 test/check_lib_core \
73 test/check_lib_tool \73 test/check_lib_tool \
74 test/check_modules_midauth74 test/check_modules_midauth \
75 test/check_lib_tool \
76 test/check_hipd
75check_PROGRAMS = test/check_firewall \77check_PROGRAMS = test/check_firewall \
76 test/check_lib_core \78 test/check_lib_core \
77 test/check_lib_tool \79 test/check_lib_tool \
78 test/check_modules_midauth80 test/check_modules_midauth \
81 test/check_lib_tool \
82 test/check_hipd
79endif83endif
8084
8185
@@ -207,6 +211,10 @@
207lib_core_libhipcore_la_SOURCES += lib/core/performance.c211lib_core_libhipcore_la_SOURCES += lib/core/performance.c
208endif212endif
209213
214test_check_hipd_SOURCES = test/check_hipd.c \
215 test/hipd/netdev.c \
216 modules/midauth/hipd/midauth.c \
217 $(hipd_hipd_sources)
210218
211test_check_firewall_SOURCES = test/check_firewall.c \219test_check_firewall_SOURCES = test/check_firewall.c \
212 test/mocks.c \220 test/mocks.c \
@@ -240,6 +248,7 @@
240hipd_hipd_LDADD = lib/core/libhipcore.la248hipd_hipd_LDADD = lib/core/libhipcore.la
241test_auth_performance_LDADD = lib/core/libhipcore.la249test_auth_performance_LDADD = lib/core/libhipcore.la
242test_check_firewall_LDADD = lib/core/libhipcore.la250test_check_firewall_LDADD = lib/core/libhipcore.la
251test_check_hipd_LDADD = lib/core/libhipcore.la
243test_check_lib_core_LDADD = lib/core/libhipcore.la252test_check_lib_core_LDADD = lib/core/libhipcore.la
244test_check_lib_tool_LDADD = lib/core/libhipcore.la253test_check_lib_tool_LDADD = lib/core/libhipcore.la
245test_check_modules_midauth_LDADD = lib/core/libhipcore.la254test_check_modules_midauth_LDADD = lib/core/libhipcore.la
246255
=== modified file 'hipd/input.c'
--- hipd/input.c 2011-08-25 12:12:27 +0000
+++ hipd/input.c 2011-09-13 12:13:33 +0000
@@ -521,7 +521,7 @@
521 /* RVS/Relay is handled later in the code. */521 /* RVS/Relay is handled later in the code. */
522 if (hip_relay_get_status() == HIP_RELAY_OFF)522 if (hip_relay_get_status() == HIP_RELAY_OFF)
523#endif523#endif
524 return -1;524 return -1;
525 }525 }
526526
527 /* Debug printing of received packet information. All received HIP527 /* Debug printing of received packet information. All received HIP
528528
=== modified file 'hipd/netdev.c'
--- hipd/netdev.c 2011-08-15 14:11:56 +0000
+++ hipd/netdev.c 2011-09-13 12:13:33 +0000
@@ -79,7 +79,7 @@
7979
80/**80/**
81 * We really don't expect more than a handfull of interfaces to be on81 * We really don't expect more than a handfull of interfaces to be on
82 * our white list.82 * our whitelist.
83 */83 */
84#define HIP_NETDEV_MAX_WHITE_LIST 584#define HIP_NETDEV_MAX_WHITE_LIST 5
8585
@@ -90,54 +90,68 @@
90#define FAM_STR_MAX 3290#define FAM_STR_MAX 32
9191
92/**92/**
93 * This is the white list. For every interface, which is in our white list,93 * This is the whitelist. For every interface, which is in our whitelist,
94 * this array has a fixed size, because there seems to be no need at this94 * this array has a fixed size, because there seems to be no need at this
95 * moment to deal with dynamic memory - which would complicate the code95 * moment to deal with dynamic memory - which would complicate the code
96 * and cost size and performance at least equal if not more to this fixed96 * and cost size and performance at least equal if not more to this fixed
97 * size array.97 * size array.
98 * Free slots are signaled by the value -1.98 * Free slots are signaled by the value -1.
99 */99 */
100static int hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];100struct hip_netdev_whitelist_entry {
101static int hip_netdev_white_list_count = 0;101 unsigned int if_index;
102 char if_label[IF_NAMESIZE];
103};
104static struct hip_netdev_whitelist_entry hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
105static unsigned int hip_netdev_white_list_count = 0;
102106
103/**107/**
104 * Add a network interface index number to the list of white listed108 * Add a network interface index number plus label to the list of whitelisted
105 * network interfaces.109 * network interfaces.
106 *110 *
107 * @param if_index the network interface index to be white listed111 * @param if_index the network interface index to be whitelisted
112 * @param device_name the network interface label to be whitelisted
108 */113 */
109static void hip_netdev_white_list_add_index(int if_index)114static void hip_netdev_white_list_add_index_and_name(const unsigned int if_index,
115 const char *const device_name)
110{116{
111 if (hip_netdev_white_list_count < HIP_NETDEV_MAX_WHITE_LIST) {117 if (hip_netdev_white_list_count < HIP_NETDEV_MAX_WHITE_LIST) {
112 hip_netdev_white_list[hip_netdev_white_list_count++] = if_index;118 hip_netdev_white_list[hip_netdev_white_list_count].if_index = if_index;
119 strncpy(hip_netdev_white_list[hip_netdev_white_list_count].if_label,
120 device_name, sizeof(hip_netdev_white_list[0].if_label));
121 hip_netdev_white_list_count++;
113 } else {122 } else {
114 /* We should NEVER run out of white list slots!!! */123 /* We should NEVER run out of whitelist slots!!! */
115 HIP_DIE("Error: ran out of space for white listed interfaces!\n");124 HIP_DIE("Error: ran out of space for whitelisted interfaces!\n");
116 }125 }
117}126}
118127
119/**128/**
120 * Test if the given network interface index is white listed.129 * Test if the given network interface index plus label is whitelisted.
121 *130 *
122 * @param if_index the index of the network interface to be tested131 * @param if_index the index of the network interface to be tested
132 * @param device_name the label of the network interface to be tested
123 * @return 1 if the index is whitelisted or zero otherwise133 * @return 1 if the index is whitelisted or zero otherwise
124 */134 */
125static int hip_netdev_is_in_white_list(int if_index)135static int hip_netdev_is_in_white_list(const unsigned int if_index,
136 const char *const device_name)
126{137{
127 int i = 0;138 if (hip_netdev_white_list_count > 0) {
128 for (i = 0; i < hip_netdev_white_list_count; i++) {139 for (unsigned int i = 0; i < hip_netdev_white_list_count; i++) {
129 if (hip_netdev_white_list[i] == if_index) {140 if (hip_netdev_white_list[i].if_index == if_index &&
130 return 1;141 !strncmp(hip_netdev_white_list[i].if_label,
142 device_name, sizeof(hip_netdev_white_list[0].if_label))) {
143 return 1;
144 }
131 }145 }
132 }146 }
133 return 0;147 return 0;
134}148}
135149
136/**150/**
137 * Add a network interface index number to the list of white listed151 * Add a network interface index number to the list of whitelisted
138 * network interfaces by name.152 * network interfaces by name.
139 *153 *
140 * @param device_name the name of the device to be white listed154 * @param device_name the name of the device to be whitelisted
141 * @return 1 on success, 0 on error155 * @return 1 on success, 0 on error
142 */156 */
143int hip_netdev_white_list_add(const char *const device_name)157int hip_netdev_white_list_add(const char *const device_name)
@@ -146,19 +160,18 @@
146 int sock = 0;160 int sock = 0;
147 int ret = 0;161 int ret = 0;
148162
149 strncpy(ifr.ifr_name, device_name, IF_NAMESIZE);163 strncpy(ifr.ifr_name, device_name, sizeof(ifr.ifr_name));
150 sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);164 sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
151165
152 if (ioctl(sock, SIOCGIFINDEX, &ifr) == 0) {166 if (ioctl(sock, SIOCGIFINDEX, &ifr) == 0) {
153 ret = 1;167 ret = 1;
154 hip_netdev_white_list_add_index(ifr.ifr_ifindex);168 hip_netdev_white_list_add_index_and_name(ifr.ifr_ifindex, device_name);
155 HIP_DEBUG("Adding device <%s> to white list with index <%i>.\n",169 HIP_DEBUG("Adding device <%s> to whitelist with index <%i>.\n",
156 device_name,170 device_name,
157 ifr.ifr_ifindex);171 ifr.ifr_ifindex);
158 } else {172 } else {
159 ret = 0;173 ret = 0;
160 }174 }
161
162 if (sock) {175 if (sock) {
163 close(sock);176 close(sock);
164 }177 }
@@ -632,7 +645,7 @@
632 HIP_IFEL(!(if_index = if_nametoindex(g_iface->ifa_name)),645 HIP_IFEL(!(if_index = if_nametoindex(g_iface->ifa_name)),
633 -1, "if_nametoindex failed\n");646 -1, "if_nametoindex failed\n");
634 /* Check if our interface is in the whitelist */647 /* Check if our interface is in the whitelist */
635 if ((hip_netdev_white_list_count > 0) && (!hip_netdev_is_in_white_list(if_index))) {648 if ((!hip_netdev_is_in_white_list(if_index, g_iface->ifa_name))) {
636 continue;649 continue;
637 }650 }
638651
@@ -1120,6 +1133,44 @@
1120}1133}
11211134
1122/**1135/**
1136 * Gives you the interface label for a given IPv4 or IPv6 address.
1137 *
1138 * @param addr Address for which you want to know the label
1139 * @param label Pointer where the function stores the label.
1140 * @param sizeoflabel Size of the passed label parameter, should be chosen big enough
1141 * to be able to store every possible interface name plus null-termination.
1142 * @return Zero on success, one on error and write the label
1143 * in param label if the given address exists in the system.
1144 * Label will be null terminated for sure, iff param label
1145 * is chosen big enough(IF_NAMESIZE == 16).
1146 */
1147int hip_find_label_for_address(const struct sockaddr *const addr,
1148 char *const label, unsigned int const sizeoflabel)
1149{
1150 int res = 1;
1151 struct ifaddrs *myaddrs, *ifa = NULL;
1152
1153 getifaddrs(&myaddrs);
1154 for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
1155 if (ifa->ifa_addr == NULL ||
1156 (ifa->ifa_addr->sa_family != AF_INET &&
1157 ifa->ifa_addr->sa_family != AF_INET6)) {
1158 continue;
1159 }
1160
1161 if (!memcmp(addr, ifa->ifa_addr, sizeof(addr))) {
1162 if (strlen(ifa->ifa_name) < sizeoflabel) {
1163 strncpy(label, ifa->ifa_name, sizeoflabel);
1164 res = 0;
1165 }
1166 break;
1167 }
1168 }
1169 freeifaddrs(myaddrs);
1170 return res;
1171}
1172
1173/**
1123 * Netlink event handler. Handles IPsec acquire messages (triggering1174 * Netlink event handler. Handles IPsec acquire messages (triggering
1124 * of base exchange) and updates the cache of local addresses when1175 * of base exchange) and updates the cache of local addresses when
1125 * address changes occur.1176 * address changes occur.
@@ -1165,11 +1216,6 @@
1165 rta = IFA_RTA(ifa);1216 rta = IFA_RTA(ifa);
1166 l = msg->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa));1217 l = msg->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa));
11671218
1168 /* Check if our interface is in the whitelist */
1169 if ((hip_netdev_white_list_count > 0) &&
1170 (!hip_netdev_is_in_white_list(ifindex))) {
1171 continue;
1172 }
11731219
1174 if ((ifa->ifa_family != AF_INET) &&1220 if ((ifa->ifa_family != AF_INET) &&
1175 (ifa->ifa_family != AF_INET6)) {1221 (ifa->ifa_family != AF_INET6)) {
@@ -1211,6 +1257,17 @@
1211 HIP_DEBUG("Unknown addr family in addr\n");1257 HIP_DEBUG("Unknown addr family in addr\n");
1212 }1258 }
12131259
1260 /* find interface label for address and check if it is whitelisted or not */
1261 if (is_add) {
1262 char label[IF_NAMESIZE];
1263 if (!hip_find_label_for_address(addr, label, sizeof(label)) &&
1264 !hip_netdev_is_in_white_list(ifa->ifa_index, label)) {
1265 HIP_DEBUG("Interface:<%s> is not whitelisted\n", label);
1266 continue;
1267 }
1268 }
1269
1270
1214 /* Trying to add an existing address or deleting a non-existing1271 /* Trying to add an existing address or deleting a non-existing
1215 * address */1272 * address */
1216 exists = hip_exists_address_in_list(addr, ifa->ifa_index);1273 exists = hip_exists_address_in_list(addr, ifa->ifa_index);
12171274
=== modified file 'hipd/netdev.h'
--- hipd/netdev.h 2011-05-18 15:12:07 +0000
+++ hipd/netdev.h 2011-09-13 12:13:33 +0000
@@ -56,4 +56,7 @@
56int hip_map_id_to_addr(const hip_hit_t *hit, const hip_lsi_t *lsi,56int hip_map_id_to_addr(const hip_hit_t *hit, const hip_lsi_t *lsi,
57 struct in6_addr *addr);57 struct in6_addr *addr);
5858
59int hip_find_label_for_address(const struct sockaddr *const addr,
60 char *const label, unsigned int const sizeoflabel);
61
59#endif /* HIP_HIPD_NETDEV_H */62#endif /* HIP_HIPD_NETDEV_H */
6063
=== added file 'test/check_hipd.c'
--- test/check_hipd.c 1970-01-01 00:00:00 +0000
+++ test/check_hipd.c 2011-09-13 12:13:33 +0000
@@ -0,0 +1,41 @@
1/*
2 * Copyright (c) 2010 Aalto University and RWTH Aachen University.
3 *
4 * Permission is hereby granted, free of charge, to any person
5 * obtaining a copy of this software and associated documentation
6 * files (the "Software"), to deal in the Software without
7 * restriction, including without limitation the rights to use,
8 * copy, modify, merge, publish, distribute, sublicense, and/or sell
9 * copies of the Software, and to permit persons to whom the
10 * Software is furnished to do so, subject to the following
11 * conditions:
12 *
13 * The above copyright notice and this permission notice shall be
14 * included in all copies or substantial portions of the Software.
15 *
16 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
17 * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
18 * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
19 * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
20 * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
21 * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
22 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
23 * OTHER DEALINGS IN THE SOFTWARE.
24 */
25
26#include <check.h>
27#include <stdlib.h>
28
29#include "test/hipd/test_suites.h"
30
31int main(void)
32{
33 int number_failed;
34 SRunner *sr = srunner_create(NULL);
35 srunner_add_suite(sr, hipd_netdev());
36
37 srunner_run_all(sr, CK_NORMAL);
38 number_failed = srunner_ntests_failed(sr);
39 srunner_free(sr);
40 return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
41}
042
=== 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-13 12:13:33 +0000
@@ -0,0 +1,89 @@
1/*
2 * Copyright (c) 2011 Aalto University and RWTH Aachen University.
3 *
4 * Permission is hereby granted, free of charge, to any person
5 * obtaining a copy of this software and associated documentation
6 * files (the "Software"), to deal in the Software without
7 * restriction, including without limitation the rights to use,
8 * copy, modify, merge, publish, distribute, sublicense, and/or sell
9 * copies of the Software, and to permit persons to whom the
10 * Software is furnished to do so, subject to the following
11 * conditions:
12 *
13 * The above copyright notice and this permission notice shall be
14 * included in all copies or substantial portions of the Software.
15 *
16 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
17 * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
18 * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
19 * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
20 * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
21 * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
22 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
23 * OTHER DEALINGS IN THE SOFTWARE.
24 */
25
26#define _BSD_SOURCE
27
28#include <check.h>
29#include <stdlib.h>
30
31#include "hipd/netdev.h"
32#include <ifaddrs.h>
33#include <net/if.h>
34#include "lib/core/debug.h"
35#include "lib/core/prefix.h"
36#include "test_suites.h"
37
38
39
40START_TEST(test_hip_find_label_for_address_parameters)
41{
42 struct ifaddrs *myaddrs, *ifa = NULL;
43 char label[IF_NAMESIZE];
44
45 /* The passed label parameter for the function hip_find_label_for_address
46 * must be at least 1 greater than the found label in the function. So
47 * a size of 1 would be for sure too short to store at least the shortest
48 * label of only 1 character.
49 */
50 char label_too_short[1];
51
52 getifaddrs(&myaddrs);
53 for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
54 if (ifa->ifa_addr == NULL ||
55 (ifa->ifa_addr->sa_family != AF_INET &&
56 ifa->ifa_addr->sa_family != AF_INET6)) {
57 continue;
58 }
59 HIP_DEBUG("Size of chosen label:%d\n", sizeof(label));
60 HIP_DEBUG_INADDR("Try to find label for addr...",
61 hip_cast_sa_addr((struct sockaddr *) (ifa->ifa_addr)));
62
63 fail_unless(hip_find_label_for_address(ifa->ifa_addr, label, sizeof(label)) == 0,
64 "Could not find label although address must be known.");
65
66 HIP_DEBUG("Found: %s\n", label);
67
68 HIP_DEBUG("Size of chosen label:%d\n", sizeof(label_too_short));
69 HIP_DEBUG("Try to store in the too short chosen label parameter, return has to be 1\n");
70
71 fail_unless(hip_find_label_for_address(ifa->ifa_addr, label_too_short, sizeof(label_too_short)) == 1,
72 "Not enough memory allocated for the label parameter.");
73
74 HIP_DEBUG("Return of function: %d \n",
75 hip_find_label_for_address(ifa->ifa_addr, label_too_short, sizeof(label_too_short)));
76 }
77}
78END_TEST
79
80Suite *hipd_netdev(void)
81{
82 Suite *s = suite_create("hipd/netdev");
83
84 TCase *tc_netdev = tcase_create("netdev");
85 tcase_add_test(tc_netdev, test_hip_find_label_for_address_parameters);
86 suite_add_tcase(s, tc_netdev);
87
88 return s;
89}
090
=== 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-13 12:13:33 +0000
@@ -0,0 +1,34 @@
1/*
2 * Copyright (c) 2010 Aalto University and RWTH Aachen University.
3 *
4 * Permission is hereby granted, free of charge, to any person
5 * obtaining a copy of this software and associated documentation
6 * files (the "Software"), to deal in the Software without
7 * restriction, including without limitation the rights to use,
8 * copy, modify, merge, publish, distribute, sublicense, and/or sell
9 * copies of the Software, and to permit persons to whom the
10 * Software is furnished to do so, subject to the following
11 * conditions:
12 *
13 * The above copyright notice and this permission notice shall be
14 * included in all copies or substantial portions of the Software.
15 *
16 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
17 * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
18 * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
19 * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
20 * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
21 * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
22 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
23 * OTHER DEALINGS IN THE SOFTWARE.
24 */
25
26#ifndef HIP_TEST_HIPD_TEST_SUITES_H
27#define HIP_TEST_HIPD_TEST_SUITES_H
28
29#include <check.h>
30
31Suite *hipd_netdev(void);
32
33
34#endif /* HIP_TEST_HIPD_TEST_SUITES_H */
035
=== 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-13 12:13:33 +0000
@@ -65,7 +65,7 @@
65START_TEST(test_serialize_deserialize_rsa_keys)65START_TEST(test_serialize_deserialize_rsa_keys)
66{66{
67 unsigned int i, keyrr_len = 0;67 unsigned int i, keyrr_len = 0;
68 int bits[3] = {1024, 2048, 3072};68 int bits[3] = { 1024, 2048, 3072 };
69 RSA *key = NULL, *key_deserialized = NULL;69 RSA *key = NULL, *key_deserialized = NULL;
70 EVP_PKEY *key_a = NULL, *key_b = NULL;70 EVP_PKEY *key_a = NULL, *key_b = NULL;
71 unsigned char *keyrr;71 unsigned char *keyrr;

Subscribers

People subscribed via source and target branches