grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 00/13] error: Do compile-time format string checking on gr


From: Glenn Washburn
Subject: Re: [PATCH v4 00/13] error: Do compile-time format string checking on grub_error
Date: Sun, 28 Feb 2021 14:31:00 -0600

On Sat, 27 Feb 2021 13:19:17 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Feb 18, 2021 at 08:47:01PM -0600, Glenn Washburn wrote:
> > This patch series fixes all compile errors due to format string
> > issues on grub_error. This was tested against nearly all supported
> > platforms successfully. This is important because earlier versions
> > of these changes compiled successfully on x86 platforms, but had
> > issues on other ones not tested.
> 
> Could you give examples what kind of errors you get when you build
> non-x86 platforms?

More format string errors showed up when building for non-x86 platforms
because code which contained a format string issue, but is not compiled
for x86 was then compiled. For example, the diff for
grub-core/kern/arm64/dl.c in patch #10. That bug did not show up in my
original patch series because I was only building for x86 targets.

> > All patches except the last fix actual format string issues. The
> > last patch turns format string issues into errors. This is a good
> > idea because it will help to prevent introducing new format string
> > issues into the code. Since, I presume, Daniel does not do not do a
> > build test for all architectures before committing to master, this
> > will not ensure that no format string issues get introduced into
> > the code. However, it should flush out any format string
> 
> I am confused by these sentence. Anyway, I do build tests of all
> patches before committing for all architectures which I am aware of.
> I think difference between our results come from difference in build
> environments, flags and options to do tests.

I hope this did not sound like a criticism or that you missed something
due to the way you are build testing. You would have never gotten a
build failure for any of these bugs even if you were building for
all architectures (which it sounds like you were). I was merely unsure
if you did a build test for all known architectures for every update of
master. With patch #13 it can be more of an issue if you're not building
testing all architectures because of the situation I outlined above in
the first paragraph. In the worst case though, GRUB would fail to build
for certain non-tested architectures where patches with format string
bugs were introduced. Presumably, this would be discovered quickly by
the people here who use those architectures.

> > issues when the CI build is done.
> >
> > Many of these changes are fairly obvious. I tried to use the
> > PRI*GRUB_*_T macros as much as I could and to not cast arguments.
> > There are some notable exceptions. There is some code that uses the
> > same grub_error code in both 32 and 64 bit architectures (riscv),
> > so casting was needed to force the larger storage type. The second
> > to last patch for zfs is still confounding to me as to why the
> > macro PRIuGRUB_UINT64_T was not expanding to llu, and yet the
> > compiler was saying the argument was a long long unsigned.
> 
> For all the patches except #12 Reviewed-by: Daniel Kiper
> <daniel.kiper@oracle.com>
> 
> Though I will probably do not get #13 until #12 is updated and merged.

Yes definitely, #13 requires the issue in #12 be addressed in someway.

Glenn



reply via email to

[Prev in Thread] Current Thread [Next in Thread]