Re: [PATCH 2/3] osdep/unix/getroot.c: Clean up redundant code
Le mar. 23 janv. 2024, 17:44, Daniel Kiper a écrit : > Mate, > > Next time please respond to all people/addresses in the original > email... > > On Mon, Jan 22, 2024 at 02:09:38PM +, Mate Kukri wrote: > > 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. > > We know the Coverity makes mistakes. So, we carefully check its reports. > These ones are not exceptions. Here the Coverity is correct and it is > safe to remove redundant code. I expect somebody was changing the code > at some point and forgot to drop surplus assignments. > Nope. It was to keep an invariant and ensure we don't double-free ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/3] osdep/unix/getroot.c: Clean up redundant code
Mate, Next time please respond to all people/addresses in the original email... On Mon, Jan 22, 2024 at 02:09:38PM +, Mate Kukri wrote: > 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. We know the Coverity makes mistakes. So, we carefully check its reports. These ones are not exceptions. Here the Coverity is correct and it is safe to remove redundant code. I expect somebody was changing the code at some point and forgot to drop surplus assignments. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/3] osdep/unix/getroot.c: Clean up redundant code
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 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 > --- > 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, ) < 0) > -- > 2.27.0 > > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/3] osdep/unix/getroot.c: Clean up redundant code
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 --- 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, ) < 0) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
