Discussion:
[PATCH] ofnet: Initialize structs in bootpath parser
(too old to reply)
Julian Andres Klode
2018-08-23 11:46:17 UTC
Permalink
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
---
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 = {};
grub_net_interface_flags_t flags = 0;
struct grub_net_network_level_interface *inter = NULL;
grub_uint16_t vlantag = 0;
--
2.17.1
--
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer i speak de, en
Julian Andres Klode
2018-08-23 12:12:56 UTC
Permalink
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 <***@canonical.com>
---
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 = {};
grub_net_interface_flags_t flags = 0;
struct grub_net_network_level_interface *inter = NULL;
grub_uint16_t vlantag = 0;
--
2.17.1
Paul Menzel
2018-08-24 20:38:29 UTC
Permalink
Post by Julian Andres Klode
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
succeed
Post by Julian Andres Klode
with random addresses has been added. This caused a weird
is added (I’d use present tense to describe a problem.)
This causes
Post by Julian Andres Klode
bug in Ubuntu, because when booting from network, we now
had two interfaces with the same name, and net_default_mac
have
Post by Julian Andres Klode
pointed to the random one.
points
Post by Julian Andres Klode
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1785859
---
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 = {};
grub_net_interface_flags_t flags = 0;
struct grub_net_network_level_interface *inter = NULL;
grub_uint16_t vlantag = 0;
The diff looks good.


Kind regards,

Paul
Michael Chang
2018-08-29 08:51:46 UTC
Permalink
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. :)
Post by Julian Andres Klode
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
---
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
Post by Julian Andres Klode
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
https://lists.gnu.org/mailman/listinfo/grub-devel
Julian Andres Klode
2018-08-29 09:43:04 UTC
Permalink
Post by 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. :)
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.
Post by Michael Chang
Post by Julian Andres Klode
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
---
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.
Post by Michael Chang
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
Continue reading on narkive:
Loading...