Re: [PATCH v8 4/6] Drop gnulib no-abort.patch

[Glenn Washburn]

On Thu, 03 Mar 2022 11:58:11 -0500
Robbie Harwood <rharwood@redhat.com> wrote:

> Glenn Washburn <development@efficientek.com> writes:
> 
> > On Wed,  2 Mar 2022 14:08:27 -0500
> > Robbie Harwood <rharwood@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 <rharwood@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().
> 
> If you have a patch that makes this work, I don't have a problem with
> it.  However, I was unable to make that work in practice.

Can you provide some specifics on what problem you were running in to?
Was it a link issue at build time? platform specific? Did it build fine,
but blew up in testing? Did you try using the linker options I suggested
above?

I don't mind trying to get this to work, but I'm not sure if it'll be
soon.

I'm not convinced that the patch as is is actually the right thing to
do, and could actually be introducing a bug / undesirable behavior. I
suppose the impact is small because abort is an edge case. If we
ultimately do go with this, I think a FIXME or TODO comment should be
added saying that we should be using grub_abort() so that platform
specific cleanup code can be run.

Glenn