Glenn Washburn <developm...@efficientek.com> writes:
> 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().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
signature.asc
Description: PGP signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
