Re: [PATCH resend] ofnet: Initialize structs in bootpath parser

[Julian Andres Klode]

On Wed, Aug 29, 2018 at 04:51:46PM +0800, Michael Chang wrote:
> Actually I also have a similar patch posted one month ago for the very same
> problem.
> 
> https://lists.gnu.org/archive/html/grub-devel/2018-07/msg00103.html
> 
> But as long as either way will do, I don't mind which one is going to be
> reviewed and eventually merged. :)

I need to send the one with the fixed message. The problem I see with
your approach is that some fields might still be uninitialized and maybe
used later, hence I went with the full-on zeroes everywhere approach.

> 
> On Thu, Aug 23, 2018 at 02:12:56PM +0200, Julian Andres Klode wrote:
> > Code later on checks if variables inside the struct are
> > 0 to see if they have been set, like if there were addresses
> > in the bootpath.
> > 
> > The variables were not initialized however, so the check
> > might suceed with uninitialized data, and a new interface
> > with random addresses has been added. This caused a weird
> > bug in Ubuntu, because when booting from network, we now
> > had two interfaces with the same name, and net_default_mac
> > pointed to the random one.
> > 
> > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1785859
> > Signed-off-by: Julian Andres Klode <address@hidden>
> > ---
> >  grub-core/net/drivers/ieee1275/ofnet.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/grub-core/net/drivers/ieee1275/ofnet.c 
> > b/grub-core/net/drivers/ieee1275/ofnet.c
> > index 002446be1..00abc64bb 100644
> > --- a/grub-core/net/drivers/ieee1275/ofnet.c
> > +++ b/grub-core/net/drivers/ieee1275/ofnet.c
> > @@ -153,8 +153,8 @@ grub_ieee1275_parse_bootpath (const char *devpath, char 
> > *bootpath,
> >    char *comma_char = 0;
> >    char *equal_char = 0;
> >    grub_size_t field_counter = 0;
> > -  grub_net_network_level_address_t client_addr, gateway_addr, subnet_mask;
> > -  grub_net_link_level_address_t hw_addr;
> > +  grub_net_network_level_address_t client_addr = {}, gateway_addr = {}, 
> > subnet_mask = {};
> > +  grub_net_link_level_address_t hw_addr = {};
> 
> Well. I personally don't like the idea to use empty initializer list as some
> compiler would reject it since it is not defined in ISO C standard to be 
> useful
> to initialize "rest" of the structure to zero, if only part of the initialzer
> is provided.  The gcc accepts it but is an extenstion syntax anyway.

grub requires gcc(-style) compilers, though, and makes use of GNU C throughout
the code.

> 
> I think better to have at least one initialzer on the list, like {0} or
> similar, which is standard compliant and also more readable.

Then you have a problem if the structure layout changes, unless you use
a named initializer.

-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en