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

[Vladimir 'phcoder' Serbinenko]

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

-- 
Regards
Vladimir 'phcoder' Serbinenko