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

[Michael Chang]

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

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.

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

Thanks,
Michael

>    grub_net_interface_flags_t flags = 0;
>    struct grub_net_network_level_interface *inter = NULL;
>    grub_uint16_t vlantag = 0;
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel