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

[Robbie Harwood <rharwood@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5668 bytes --]

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.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]