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

[Glenn Washburn]

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().

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)