On Wed, 2 Mar 2022 14:08:27 -0500 Robbie Harwood <rharw...@redhat.com> wrote:
> Originally added in db7337a3d353a817ffe9eb4a3702120527100be9, this > patched out all relevant invocations of abort() in gnulib. While it was > not documented why at the time, testing suggests that there's no abort() > implementation available for gnulib to use. > > gnulib's position is that the use of abort() is correct here, since it > happens when input violates a "shall" from POSIX. Additionally, the > code in question is probably not reachable. Since abort() is more > friendly to user-space, they prefer to make no change, so we can just > carry a define instead. (Suggested by Paul Eggert.) > > Signed-off-by: Robbie Harwood <rharw...@redhat.com> > --- > bootstrap.conf | 2 +- > conf/Makefile.extra-dist | 1 - > config.h.in | 10 ++++++++ > grub-core/lib/gnulib-patches/no-abort.patch | 26 --------------------- > 4 files changed, 11 insertions(+), 28 deletions(-) > delete mode 100644 grub-core/lib/gnulib-patches/no-abort.patch > > diff --git a/bootstrap.conf b/bootstrap.conf > index 21a8cf15d..71acbeeb1 100644 > --- a/bootstrap.conf > +++ b/bootstrap.conf > @@ -82,7 +82,7 @@ cp -a INSTALL INSTALL.grub > bootstrap_post_import_hook () { > set -e > for patchname in fix-null-deref fix-null-state-deref > fix-regcomp-uninit-token \ > - fix-regexec-null-deref fix-uninit-structure fix-unused-value fix-width > no-abort; do > + fix-regexec-null-deref fix-uninit-structure fix-unused-value > fix-width; do > patch -d grub-core/lib/gnulib -p2 \ > < "grub-core/lib/gnulib-patches/$patchname.patch" > done > diff --git a/conf/Makefile.extra-dist b/conf/Makefile.extra-dist > index 15a9b74e9..4ddc3c8f7 100644 > --- a/conf/Makefile.extra-dist > +++ b/conf/Makefile.extra-dist > @@ -35,7 +35,6 @@ EXTRA_DIST += > grub-core/lib/gnulib-patches/fix-regexec-null-deref.patch > EXTRA_DIST += grub-core/lib/gnulib-patches/fix-uninit-structure.patch > EXTRA_DIST += grub-core/lib/gnulib-patches/fix-unused-value.patch > EXTRA_DIST += grub-core/lib/gnulib-patches/fix-width.patch > -EXTRA_DIST += grub-core/lib/gnulib-patches/no-abort.patch > > EXTRA_DIST += grub-core/lib/libgcrypt > EXTRA_DIST += grub-core/lib/libgcrypt-grub/mpi/generic > diff --git a/config.h.in b/config.h.in > index 965eaffce..209e27449 100644 > --- a/config.h.in > +++ b/config.h.in > @@ -66,6 +66,16 @@ > > # ifndef _GL_INLINE_HEADER_BEGIN > # define _GL_ATTRIBUTE_CONST __attribute__ ((const)) > + > +/* > + * We don't have an abort() for gnulib to call in regexp. Because gnulib is > + * built as a separate object that is then linked with, it doesn't have any > + * of our headers or functions around to use - so we unfortunately can't > + * print anything. Builds of emu include the system's stdlib, which includes > + * a prototype for abort(), so leave this as a macro that doesn't take > + * arguments. > + */ > +# define abort __builtin_trap I asked earlier if we couldn't use grub_abort for gnulib's abort and Daniel referred me to subsequent patch series. I suppose this is the relevant section he was thinking of, however, its still not clear to me why we can't use grub_abort(). And actually looking more into it, its seems to me that using grub_abort() is probably what we should do because it has platform specific cleanup code. It appears that __builtin_trap is target dependent[1], but I do not believe that it is or can be platform dependent. Is the problem with grub_abort() that it provides some exit guarantees[2] and we're looking for the equivalent of a machine crash? That doesn't seem right to me. It is true that grub_abort calls grub_exit which is platform specific. So for instance for x86_64-efi, this will not call EFI boot_services->exit. Not being an EFI expert, I'm not sure of the implications of that. If my suspicion is correct and an abort in gnulib with this patch would not properly exit the EFI app and not allow returning control to another EFI app, then I would consider this patch undesirable. And I notice that other platforms have special code run on grub_exit. I suspect that GCC's __builtin_trap is more geared toward user-space programs and not for our use case. If the problem is that gnulib, as stated above, is built as a separate object and then linked to GRUB, which does not have an abort symbol (thus causing a link failure). Then I think the right solution is to create a symbol in the GRUB kernel named abort that has the same info as grub_abort. I have a patch I've been meaning to send which solves this problem for GDB which in some cases look for symbols free() and malloc(). When building the GRUB kernel add "-Wl,--defsym=abort=grub_abort" to the LDFLAGS_KERNEL variable in conf/Makefile.common. I haven't tested this, so I'm interested in hearing why this won't work or isn't a good solution. I believe this should work for user-space platforms like emu because of the platform specific grub_exit(). The one problem I see is that for the emu platform exit guarantees may be undesirable. This this case, perhaps grub_abort() should call libc's abort(). Glenn [1] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005ftrap [2] https://stackoverflow.com/a/35734843/2108011 > # endif /* !_GL_INLINE_HEADER_BEGIN */ > > #endif > diff --git a/grub-core/lib/gnulib-patches/no-abort.patch > b/grub-core/lib/gnulib-patches/no-abort.patch > deleted file mode 100644 > index e469c4762..000000000 > --- a/grub-core/lib/gnulib-patches/no-abort.patch > +++ /dev/null > @@ -1,26 +0,0 @@ > -diff --git a/lib/regcomp.c b/lib/regcomp.c > -index cc85f35ac..de45ebb5c 100644 > ---- a/lib/regcomp.c > -+++ b/lib/regcomp.c > -@@ -528,9 +528,9 @@ regerror (int errcode, const regex_t *__restrict preg, > char *__restrict errbuf, > - to this routine. If we are given anything else, or if other regex > - code generates an invalid error code, then the program has a bug. > - Dump core so we can fix it. */ > -- abort (); > -- > -- msg = gettext (__re_error_msgid + __re_error_msgid_idx[errcode]); > -+ msg = gettext ("unknown regexp error"); > -+ else > -+ msg = gettext (__re_error_msgid + __re_error_msgid_idx[errcode]); > - > - msg_size = strlen (msg) + 1; /* Includes the null. */ > - > -@@ -1136,7 +1136,7 @@ optimize_utf8 (re_dfa_t *dfa) > - } > - break; > - default: > -- abort (); > -+ break; > - } > - > - if (mb_chars || has_period) _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel