Re: [PATCH v6 4/5] Drop gnulib no-abort.patch

[Daniel Kiper]

On Wed, Mar 02, 2022 at 12:07:33PM -0600, Glenn Washburn wrote:
> On Wed, 2 Mar 2022 18:00:11 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Thu, Feb 24, 2022 at 01:37:19PM -0500, Robbie Harwood 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                                 |  3 +++
> > >  grub-core/lib/gnulib-patches/no-abort.patch | 26 ---------------------
> > >  4 files changed, 4 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..a1f2b63e6 100644
> > > --- a/config.h.in
> > > +++ b/config.h.in
> > > @@ -66,6 +66,9 @@
> > >
> > >  #  ifndef _GL_INLINE_HEADER_BEGIN
> > >  #    define _GL_ATTRIBUTE_CONST __attribute__ ((const))
> > > +
> > > +/* We don't have an abort() for gnulib to call in regexp. */
> > > +#    define abort __builtin_unreachable
> >
> > There are more abort() calls in the gnulib than just regexp one. So,
> > I think this definition is wrong. IMO we should print an error message
> > if possible and jump into endless loop or use __builtin_trap() to safely
> > reboot the machine.
>
> Is it naive to think we can use grub_abort()?

Yeah, it is. Please take a look at v7.

Daniel