diff mbox series

[netifd] system-linux: initialize ifreq struct before using it

Message ID 1602156697-16669-1-git-send-email-alin.nastac@gmail.com
State Accepted
Headers show
Series [netifd] system-linux: initialize ifreq struct before using it | expand

Commit Message

Alin Nastac Oct. 8, 2020, 11:31 a.m. UTC
Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
---
 system-linux.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Hans Dedecker Oct. 8, 2020, 12:15 p.m. UTC | #1
Hi Alin,

On Thu, Oct 8, 2020 at 1:31 PM Alin Nastac <alin.nastac@gmail.com> wrote:
Could you add which issue this fixes ?

Thx
Hans
>
> Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
> ---
>  system-linux.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/system-linux.c b/system-linux.c
> index 6778b1d..9188899 100644
> --- a/system-linux.c
> +++ b/system-linux.c
> @@ -904,6 +904,8 @@ failure:
>  int system_if_resolve(struct device *dev)
>  {
>         struct ifreq ifr;
> +
> +       memset(&ifr, 0, sizeof(ifr));
>         strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1);
>         if (!ioctl(sock_ioctl, SIOCGIFINDEX, &ifr))
>                 return ifr.ifr_ifindex;
> --
> 2.7.4
>
Alin Nastac Oct. 8, 2020, 12:35 p.m. UTC | #2
Hi Hans,

The issue I have involved adding an external device to the lan bridge
through add_device ubus call. Sometimes this operation fails to add
the new bridge port with the following device debug traces:
Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]:
device_create_default(525): Create simple device 'wds1_1_2'
Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]:
device_init_virtual(478): Initialize device 'wds1_1_2'
Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]:
device_set_present(634): Network device 'wds1_1_2' is now present
Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]:
__device_add_user(710): Add user for device 'wds1_1_2', refcount=1
Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]: device_claim(413):
Claim Network device wds1_1_2, new active count: 1
Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]: device_claim(432):
claim Network device wds1_1_2 failed: -1

I'm not sure it fixes the problem I experience (the jury is still out
on that), but it is definitely caused by system_if_resolve() failure
to retrieve the interface index. Looking at the handling of
SIOCGIFINDEX in the kernel, it shouldn't matter if the rest of the
fields are uninitialized, but a variable should always be initialized
before its first use.

BR,
Alin

On Thu, Oct 8, 2020 at 2:15 PM Hans Dedecker <dedeckeh@gmail.com> wrote:
>
> Hi Alin,
>
> On Thu, Oct 8, 2020 at 1:31 PM Alin Nastac <alin.nastac@gmail.com> wrote:
> Could you add which issue this fixes ?
>
> Thx
> Hans
> >
> > Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
> > ---
> >  system-linux.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/system-linux.c b/system-linux.c
> > index 6778b1d..9188899 100644
> > --- a/system-linux.c
> > +++ b/system-linux.c
> > @@ -904,6 +904,8 @@ failure:
> >  int system_if_resolve(struct device *dev)
> >  {
> >         struct ifreq ifr;
> > +
> > +       memset(&ifr, 0, sizeof(ifr));
> >         strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1);
> >         if (!ioctl(sock_ioctl, SIOCGIFINDEX, &ifr))
> >                 return ifr.ifr_ifindex;
> > --
> > 2.7.4
> >
Alin Nastac Oct. 9, 2020, 7:48 a.m. UTC | #3
Hi Hans,

I have the confirmation, this change fixed my problem.

Given the fact that my commit fixes an obvious programming error, I
don't think I need to put a detailed explanation in the commit
description.

BR,
Alin

On Thu, Oct 8, 2020 at 2:35 PM Alin Năstac <alin.nastac@gmail.com> wrote:
>
> Hi Hans,
>
> The issue I have involved adding an external device to the lan bridge
> through add_device ubus call. Sometimes this operation fails to add
> the new bridge port with the following device debug traces:
> Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]:
> device_create_default(525): Create simple device 'wds1_1_2'
> Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]:
> device_init_virtual(478): Initialize device 'wds1_1_2'
> Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]:
> device_set_present(634): Network device 'wds1_1_2' is now present
> Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]:
> __device_add_user(710): Add user for device 'wds1_1_2', refcount=1
> Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]: device_claim(413):
> Claim Network device wds1_1_2, new active count: 1
> Wed Oct  7 04:22:57 2020 daemon.err netifd[2843]: device_claim(432):
> claim Network device wds1_1_2 failed: -1
>
> I'm not sure it fixes the problem I experience (the jury is still out
> on that), but it is definitely caused by system_if_resolve() failure
> to retrieve the interface index. Looking at the handling of
> SIOCGIFINDEX in the kernel, it shouldn't matter if the rest of the
> fields are uninitialized, but a variable should always be initialized
> before its first use.
>
> BR,
> Alin
>
> On Thu, Oct 8, 2020 at 2:15 PM Hans Dedecker <dedeckeh@gmail.com> wrote:
> >
> > Hi Alin,
> >
> > On Thu, Oct 8, 2020 at 1:31 PM Alin Nastac <alin.nastac@gmail.com> wrote:
> > Could you add which issue this fixes ?
> >
> > Thx
> > Hans
> > >
> > > Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
> > > ---
> > >  system-linux.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/system-linux.c b/system-linux.c
> > > index 6778b1d..9188899 100644
> > > --- a/system-linux.c
> > > +++ b/system-linux.c
> > > @@ -904,6 +904,8 @@ failure:
> > >  int system_if_resolve(struct device *dev)
> > >  {
> > >         struct ifreq ifr;
> > > +
> > > +       memset(&ifr, 0, sizeof(ifr));
> > >         strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1);
> > >         if (!ioctl(sock_ioctl, SIOCGIFINDEX, &ifr))
> > >                 return ifr.ifr_ifindex;
> > > --
> > > 2.7.4
> > >
diff mbox series

Patch

diff --git a/system-linux.c b/system-linux.c
index 6778b1d..9188899 100644
--- a/system-linux.c
+++ b/system-linux.c
@@ -904,6 +904,8 @@  failure:
 int system_if_resolve(struct device *dev)
 {
 	struct ifreq ifr;
+
+	memset(&ifr, 0, sizeof(ifr));
 	strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1);
 	if (!ioctl(sock_ioctl, SIOCGIFINDEX, &ifr))
 		return ifr.ifr_ifindex;