Re: [PATCH 2/3] osdep/unix/getroot.c: Clean up redundant code

[Mate Kukri]

Dear Alec, and grub-devel,

I haven't checked the specific code in question, but do we really want
to be removing such null-assignments? (Thinking about multiple patches
exactly like this).

In correct code, they are of course redundant by definition, however
their intended purpose is that if the code happens to be incorrect,
they turn use-after-free bugs into zero page accesses.

Since static analysis of a language like C is inherently conservative,
it is entirely possible that it is detecting the redundant assignment,
but not the use after free it would have prevented.

With UEFI looking to gain more memory protections, including un-mapped
zero-page (alongside NX), it might even be a good idea to add more
such assignments instead of removing them.

Best regards,
Mate Kukri

On Sat, Jan 20, 2024 at 2:54 AM Alec Brown <alec.r.brown@oracle.com> wrote:
>
> In grub-core/osdep/unix/getroot.c, coverity spotted redundant code where the
> double pointer os_dev was being set to 0 and then being overwritten later
> without being used. Since this is unnecessary, we can remove the code that 
> sets
> os_dev to 0.
>
> Fixes: CID 428875
>
> Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
> ---
>  grub-core/osdep/unix/getroot.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/grub-core/osdep/unix/getroot.c b/grub-core/osdep/unix/getroot.c
> index cde821eb9..12b111634 100644
> --- a/grub-core/osdep/unix/getroot.c
> +++ b/grub-core/osdep/unix/getroot.c
> @@ -540,7 +540,6 @@ grub_guess_root_devices (const char *dir_in)
>        for (cur = os_dev; *cur; cur++)
>         free (*cur);
>        free (os_dev);
> -      os_dev = 0;
>      }
>
>    if (stat (dir, &st) < 0)
> --
> 2.27.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel