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: 204 lines (+100/-20)
2 files modified
hipd/netdev.c (+98/-20)
hipd/netdev.h (+2/-0)
To merge this branch: bzr merge lp:~christian-roeller/hipl/whitelisting
Reviewer Review Type Date Requested Status
Diego Biurrun Needs Fixing
Stefan Götz (community) Approve
Review via email: mp+67982@code.launchpad.net

This proposal has been superseded by a proposal from 2011-08-10.

Commit message

Extension for the whitelist-functionality:
Alias-Interfaces have the same index as its physical interfaces for the linux kernel, so this extension considers also the interface-labels, when a new netlink event comes in and checks if this label is whitelisted or not.

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

Quick test report: seems to work without any troubles.

5983. By Christian Röller

a few changes due to policy violations and cleanup

5984. By Christian Röller

a few changes to impvove maintainability

5985. By Christian Röller

another maintainability improvement

Revision history for this message
Christian Röller (christian-roeller) wrote :
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 :
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...

5986. By Christian Röller

A few changes respecting style, policy and maintainability.

Revision history for this message
Christian Röller (christian-roeller) wrote :
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) :
review: Approve
Revision history for this message
Diego Biurrun (diego-biurrun) wrote :

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

Correct some code style violations.

Revision history for this message
Christian Röller (christian-roeller) wrote :

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

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 :

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

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 :

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 :

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

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

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 :

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 :

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

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 :

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

5988. By Christian Röller

cosmetic: shorten the hip_find_label_for_address function a little bit

5989. By Christian Röller

change the return description for the hip_find_label_for_address function

5990. By Christian Röller

cosmetic: shorten a few things

5991. By Christian Röller

cosmetic: typo in function description

5992. By Christian Röller

cosmetic: correct some typos

5993. By Christian Röller

cosmetic: alignments in function description hip_find_label_for_address

5994. By Christian Röller

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

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 'hipd/netdev.c'
--- hipd/netdev.c 2011-07-21 11:28:01 +0000
+++ hipd/netdev.c 2011-08-01 09:16:42 +0000
@@ -100,19 +100,26 @@
100 * size array.100 * size array.
101 * Free slots are signaled by the value -1.101 * Free slots are signaled by the value -1.
102 */102 */
103static int hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];103struct hip_netdev_whiteliste_entry {
104static int hip_netdev_white_list_count = 0;104 unsigned int if_index;
105 char if_label[IF_NAMESIZE];
106};
107static struct hip_netdev_whiteliste_entry hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST];
108static unsigned int hip_netdev_white_list_count = 0;
105109
106/**110/**
107 * Add a network interface index number to the list of white listed111 * Add a network interface index number plus label to the list of white listed
108 * network interfaces.112 * network interfaces.
109 *113 *
110 * @param if_index the network interface index to be white listed114 * @param if_index the network interface index to be white listed
115 * @param device_name the network interface label to be white listed
111 */116 */
112static void hip_netdev_white_list_add_index(int if_index)117static void hip_netdev_white_list_add_index_and_name(const unsigned int if_index, const char *const device_name)
113{118{
114 if (hip_netdev_white_list_count < HIP_NETDEV_MAX_WHITE_LIST) {119 if (hip_netdev_white_list_count < HIP_NETDEV_MAX_WHITE_LIST) {
115 hip_netdev_white_list[hip_netdev_white_list_count++] = if_index;120 hip_netdev_white_list[hip_netdev_white_list_count].if_index = if_index;
121 strncpy(hip_netdev_white_list[hip_netdev_white_list_count].if_label, device_name, sizeof(hip_netdev_white_list[0].if_label) - 1);
122 hip_netdev_white_list_count++;
116 } else {123 } else {
117 /* We should NEVER run out of white list slots!!! */124 /* We should NEVER run out of white list slots!!! */
118 HIP_DIE("Error: ran out of space for white listed interfaces!\n");125 HIP_DIE("Error: ran out of space for white listed interfaces!\n");
@@ -120,17 +127,20 @@
120}127}
121128
122/**129/**
123 * Test if the given network interface index is white listed.130 * Test if the given network interface index plus label is white listed.
124 *131 *
125 * @param if_index the index of the network interface to be tested132 * @param if_index the index of the network interface to be tested
133 * @param device_name the label of the network interface to be tested
126 * @return 1 if the index is whitelisted or zero otherwise134 * @return 1 if the index is whitelisted or zero otherwise
127 */135 */
128static int hip_netdev_is_in_white_list(int if_index)136static int hip_netdev_is_in_white_list(const unsigned int if_index, const char *const device_name)
129{137{
130 int i = 0;138 if (hip_netdev_white_list_count > 0) {
131 for (i = 0; i < hip_netdev_white_list_count; i++) {139 for (unsigned int i = 0; i < hip_netdev_white_list_count; i++) {
132 if (hip_netdev_white_list[i] == if_index) {140 if (hip_netdev_white_list[i].if_index == if_index &&
133 return 1;141 !strncmp(hip_netdev_white_list[i].if_label, device_name, sizeof(hip_netdev_white_list[0].if_label))) {
142 return 1;
143 }
134 }144 }
135 }145 }
136 return 0;146 return 0;
@@ -149,19 +159,18 @@
149 int sock = 0;159 int sock = 0;
150 int ret = 0;160 int ret = 0;
151161
152 strncpy(ifr.ifr_name, device_name, IF_NAMESIZE);162 strncpy(ifr.ifr_name, device_name, sizeof(ifr.ifr_name) - 1);
153 sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);163 sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
154164
155 if (ioctl(sock, SIOCGIFINDEX, &ifr) == 0) {165 if (ioctl(sock, SIOCGIFINDEX, &ifr) == 0) {
156 ret = 1;166 ret = 1;
157 hip_netdev_white_list_add_index(ifr.ifr_ifindex);167 hip_netdev_white_list_add_index_and_name(ifr.ifr_ifindex, device_name);
158 HIP_DEBUG("Adding device <%s> to white list with index <%i>.\n",168 HIP_DEBUG("Adding device <%s> to white list with index <%i>.\n",
159 device_name,169 device_name,
160 ifr.ifr_ifindex);170 ifr.ifr_ifindex);
161 } else {171 } else {
162 ret = 0;172 ret = 0;
163 }173 }
164
165 if (sock) {174 if (sock) {
166 close(sock);175 close(sock);
167 }176 }
@@ -635,7 +644,7 @@
635 HIP_IFEL(!(if_index = if_nametoindex(g_iface->ifa_name)),644 HIP_IFEL(!(if_index = if_nametoindex(g_iface->ifa_name)),
636 -1, "if_nametoindex failed\n");645 -1, "if_nametoindex failed\n");
637 /* Check if our interface is in the whitelist */646 /* Check if our interface is in the whitelist */
638 if ((hip_netdev_white_list_count > 0) && (!hip_netdev_is_in_white_list(if_index))) {647 if ((!hip_netdev_is_in_white_list(if_index, g_iface->ifa_name))) {
639 continue;648 continue;
640 }649 }
641650
@@ -1123,6 +1132,68 @@
1123}1132}
11241133
1125/**1134/**
1135 * This functions gives you the interface label for a given ip4 or ip6 addr.
1136 *
1137 * @param addr address for which you want to know the label
1138 * @param label pointer where the function stores the label
1139 * @return one on success, zero on error and write the label in param label
1140 */
1141int hip_find_label_for_address(const struct sockaddr *const addr, char *const label)
1142{
1143 int res = 0;
1144 struct ifaddrs *myaddrs, *ifa = NULL;
1145
1146 const struct sockaddr_in *s4_in = NULL;
1147 const struct sockaddr_in6 *s6_in = NULL;
1148 struct sockaddr_in *s4;
1149 struct sockaddr_in6 *s6;
1150
1151 if (addr->sa_family == AF_INET) {
1152 s4_in = (const struct sockaddr_in *const) (addr);
1153 }
1154 if (addr->sa_family == AF_INET6) {
1155 s6_in = (const struct sockaddr_in6 *const) (addr);
1156 }
1157
1158 getifaddrs(&myaddrs);
1159 for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
1160 if (ifa->ifa_addr == NULL) {
1161 continue;
1162 }
1163 if (ifa->ifa_addr->sa_family != AF_INET && ifa->ifa_addr->sa_family != AF_INET6) {
1164 continue;
1165 }
1166
1167 if (ifa->ifa_addr->sa_family == AF_INET) {
1168 s4 = (struct sockaddr_in *) (ifa->ifa_addr);
1169 if (!memcmp(s4_in, s4, sizeof(s4))) {
1170 if (strlen(ifa->ifa_name) <= sizeof(label) - 1) {
1171 strncpy(label, ifa->ifa_name, sizeof(label) - 1);
1172 res = 1;
1173 } else {
1174 res = 0;
1175 }
1176 break;
1177 }
1178 }
1179 if (ifa->ifa_addr->sa_family == AF_INET6) {
1180 s6 = (struct sockaddr_in6 *) (ifa->ifa_addr);
1181 if (!memcmp(s6_in, s6, sizeof(s6))) {
1182 if (strlen(ifa->ifa_name) <= sizeof(label) - 1) {
1183 strncpy(label, ifa->ifa_name, sizeof(label) - 1);
1184 res = 1;
1185 } else {
1186 res = 0;
1187 }
1188 break;
1189 }
1190 }
1191 }
1192 freeifaddrs(myaddrs);
1193 return res;
1194}
1195
1196/**
1126 * Netlink event handler. Handles IPsec acquire messages (triggering1197 * Netlink event handler. Handles IPsec acquire messages (triggering
1127 * of base exchange) and updates the cache of local addresses when1198 * of base exchange) and updates the cache of local addresses when
1128 * address changes occur.1199 * address changes occur.
@@ -1168,11 +1239,6 @@
1168 rta = IFA_RTA(ifa);1239 rta = IFA_RTA(ifa);
1169 l = msg->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa));1240 l = msg->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa));
11701241
1171 /* Check if our interface is in the whitelist */
1172 if ((hip_netdev_white_list_count > 0) &&
1173 (!hip_netdev_is_in_white_list(ifindex))) {
1174 continue;
1175 }
11761242
1177 if ((ifa->ifa_family != AF_INET) &&1243 if ((ifa->ifa_family != AF_INET) &&
1178 (ifa->ifa_family != AF_INET6)) {1244 (ifa->ifa_family != AF_INET6)) {
@@ -1214,6 +1280,18 @@
1214 HIP_DEBUG("Unknown addr family in addr\n");1280 HIP_DEBUG("Unknown addr family in addr\n");
1215 }1281 }
12161282
1283 /* find interface label for address and check if it is whitelisted or not */
1284 if (is_add) {
1285 char label[IF_NAMESIZE];
1286 if (hip_find_label_for_address(addr, label)) {
1287 if ((!hip_netdev_is_in_white_list(ifa->ifa_index, label))) {
1288 HIP_DEBUG("Interface:<%s> is not whitelisted\n", label);
1289 continue;
1290 }
1291 }
1292 }
1293
1294
1217 /* Trying to add an existing address or deleting a non-existing1295 /* Trying to add an existing address or deleting a non-existing
1218 * address */1296 * address */
1219 exists = hip_exists_address_in_list(addr, ifa->ifa_index);1297 exists = hip_exists_address_in_list(addr, ifa->ifa_index);
12201298
=== modified file 'hipd/netdev.h'
--- hipd/netdev.h 2011-05-18 15:12:07 +0000
+++ hipd/netdev.h 2011-08-01 09:16:42 +0000
@@ -56,4 +56,6 @@
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, char *const label);
60
59#endif /* HIP_HIPD_NETDEV_H */61#endif /* HIP_HIPD_NETDEV_H */

Subscribers

People subscribed via source and target branches