Re: [PATCH v8 04/22] libtasn1: changes for grub compatibility

[Gary Lin]

On Wed, Jan 17, 2024 at 05:51:58AM +0300, Vladimir 'phcoder' Serbinenko wrote:
> On Tue, Jan 16, 2024 at 12:23 PM Gary Lin via Grub-devel
> <grub-devel@gnu.org> wrote:
> >
> > From: Daniel Axtens <dja@axtens.net>
> >
> > Do a few things to make libtasn1 compile as part of grub:
> >
> >  - redefine _asn1_strcat. grub removed strcat so replace it with the
> >    appropriate calls to memcpy and strlen. Use this internally where
> >    strcat was used.
> >
> strcat is especially dangerous. If you can't easily replace it with
> strncat/strlcat, you probably have a buffer overflow or potential to
> have one after some code refactoring.strcpy isn't great either but a
> bit better. memcpy+strlen isn't a good replacement. Maybe we can
> upstream use of strncat, not based on GRUB needs but based on dangers
> of strcat.
I did a quick check of the use of strcat in libtasn1, and replacing
strcat with strncat looks doable since the length of the destination string
are given or can be calculated easily.

> >  - replace c_isdigit with grub_isdigit (and don't import c-ctype from
> >    gnulib) grub_isdigit provides the same functionality as c_isdigit: it
> >    determines if the input is an ASCII digit without regard for locale.
> Can we add c-ctype.h into posix compat?
> >
Sure.

> >  - replace GL_ATTRIBUTE_PURE with __attribute__((pure)) which been
> >    supported since gcc-2.96. This avoids messing around with gnulib.
> >
> Why not add -DGL_ATTRIBUTE_PURE=... into cppflags? Or even to posix
> wrap as long as we're not in gnulib? (e.g. #ifndef GNULIB ... #endif)
Adding the cppflag looks an easy fix. Thanks for the suggestion.

> >  - adjust libtasn1.h: drop the ASN1_API logic, it's not needed for our
> >    modules. Unconditionally support const and pure attributes and adjust
> >    header paths.
> Why not -DASN1_API= in cppflags?
> const/pure logic seems ok as it was. Did you encounter any problems?
I'm not sure about this part. Maybe Daniel had some problem with
ASN1_API.

> >       if (dest_tot_size > dest_size)
> >         {
> > -         strncat (dest, src, (dest_tot_size - dest_size) - 1);
> > +         memcpy (dest + dest_size, src, (dest_tot_size - dest_size) - 1);
> 
> Please add strncat into posix wrap instead. This is a bad change.
> 
Ok, will port strncat into posix wrap.

Thanks,

Gary Lin