[Bug libquadmath/114533] New: libquadmath: printf: fix misaligned access on args

[Bug libquadmath/114533] New: libquadmath: printf: fix misaligned access on args

[doko at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

            Bug ID: 114533
           Summary: libquadmath: printf: fix misaligned access on args
           Product: gcc
           Version: 13.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libquadmath
          Assignee: unassigned at gcc dot gnu.org
          Reporter: doko at gcc dot gnu.org
  Target Milestone: ---

reported at
https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647635.html

On x86, this compiles into movdqa which segfaults on unaligned access.

This kind of failure has been seen when running against glibc 2.39,
which incidentally changed the printf implementation to move away from
alloca() for this data to instead append it at the end of an existing
"scratch buffer", with arbitrary alignment, whereas alloca() was
probably more likely to be naturally aligned.

Tested by adding the patch to the Ubuntu gcc-14 package in
https://launchpad.net/~schopin/+archive/ubuntu/libquadmath

Signed-off-by: Simon Chopin <simon.chopin@canonical.com>
---
 libquadmath/printf/printf_fp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libquadmath/printf/printf_fp.c b/libquadmath/printf/printf_fp.c
index 8effcee88fa..d86aa650d38 100644
--- a/libquadmath/printf/printf_fp.c
+++ b/libquadmath/printf/printf_fp.c
@@ -363,7 +363,7 @@ __quadmath_printf_fp (struct __quadmath_printf_file *fp,

   /* Fetch the argument value. */
     {
-      fpnum = **(const __float128 **) args[0];
+      memcpy(&fpnum, *(void* const *) args[0], sizeof(fpnum));

       /* Check for special values: not a number or infinity.  */
       if (isnanq (fpnum))

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |ABI
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
The question is whether the caller misbehaves according to the ABI here? 
There's likely a known alignment present we could re-instantiate with a
__builtin_assume_aligned?

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
From what I can see, glibc uses there the same thing as libquadmath does, so
why is it ok on the glibc side and not on the libquadmath side?

I mean
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/printf_fp.c;h=e75706f089bba3baabbcfb6bcf41514bad0a9dcb;hb=HEAD#l222
and
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/printf_fp.c;h=e75706f089bba3baabbcfb6bcf41514bad0a9dcb;hb=HEAD#l191

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #3 from Andreas Schwab <schwab@linux-m68k.org> ---
Is the stack properly aligned at this point?

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #4 from Florian Weimer <fw at gcc dot gnu.org> ---
This looks like a glibc bug to me.

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2024-04-02
     Ever confirmed|0                           |1

--- Comment #5 from Andreas Schwab <schwab@linux-m68k.org> ---
Without a test case it is hard to tell.

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |UNCONFIRMED
     Ever confirmed|1                           |0

--- Comment #6 from Andreas Schwab <schwab@linux-m68k.org> ---
Looks like the issue is that args_pa_user is not kept aligned.  On the other
hand, flt128_va already uses memcpy, so it does not expect the memory to be
aligned.

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[jsm28 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #7 from Joseph S. Myers <jsm28 at gcc dot gnu.org> ---
Note also that in glibc, _Float128 support in printf code can only be used in
limited circumstances: either on powerpc64le, as one of the multiple supported
long double formats there, or through the sharing of the printf code with the
implementation of strfromf128.

In particular, there are no glibc printf formats corresponding directly to
_FloatN / _FloatNx types. There was support in principle at the WG14 meeting in
Strasbourg in January for having some form of printf/scanf support for such
types in C2y, but major work is still needed on the wording that was proposed
in N3184.

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I guess we should go with the above patch after fixing formatting, but it isn't
enough,
printf_fphex.c has similar code.
Even in glibc which doesn't support printing _Float128 nor any other type which
would require similar alignment, the hooks only register a function to fill in
some mem and allows specification of size, but can't specify alignment.

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[jvdelisle at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

Jerry DeLisle <jvdelisle at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jvdelisle at gcc dot gnu.org

--- Comment #9 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> ---
Adding myself here as I need hex format for gfortran EX format specifiers.

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 57853
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57853&action=edit
gcc14-pr114533.patch

Untested fix.  Unfortunately, we don't have any testsuite for libquadmath, hope
it will be tested during libgfortran testing.

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[doko at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #11 from Matthias Klose <doko at gcc dot gnu.org> ---
while not a test case, that was seen when running autopkg tests of the evolver
package against glibc 2.39 packages.

https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/2052929

the failing evolver test is:

echo "g 5; v; r ; g 10; v;" | evolver-nox-q cube

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #12 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:8455d6f6cd43b7b143ab9ee19437452fceba9cc9

commit r14-9769-g8455d6f6cd43b7b143ab9ee19437452fceba9cc9
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 3 10:02:35 2024 +0200

    libquadmath: Don't assume the storage for __float128 arguments is aligned
[PR114533]

    With the
register_printf_type/register_printf_modifier/register_printf_specifier
    APIs the C library is just told the size of the argument and is provided
with
    a callback to fetch the argument from va_list using va_arg into C library
provided
    memory.  The C library isn't told what alignment requirement it has, but we
were
    using direct load of a __float128 value from that memory which assumes
    __alignof (__float128) alignment.

    The following patch fixes that by using memcpy instead.

    I haven't been able to reproduce an actual crash, tried
     #include <quadmath.h>
     #include <stdlib.h>
     #include <stdio.h>

    int main ()
    {
      __float128 r;
      int prec = 20;
      int width = 46;
      char buf[128];

      r = 2.0q;
      r = sqrtq (r);
      int n = quadmath_snprintf (buf, sizeof buf, "%+-#*.20Qe", width, r);
      if ((size_t) n < sizeof buf)
        printf ("%s\n", buf);
        /* Prints: +1.41421356237309504880e+00 */
      quadmath_snprintf (buf, sizeof buf, "%Qa", r);
      if ((size_t) n < sizeof buf)
        printf ("%s\n", buf);
        /* Prints: 0x1.6a09e667f3bcc908b2fb1366ea96p+0 */
      n = quadmath_snprintf (NULL, 0, "%+-#46.*Qe", prec, r);
      if (n > -1)
        {
          char *str = malloc (n + 1);
          if (str)
            {
              quadmath_snprintf (str, n + 1, "%+-#46.*Qe", prec, r);
              printf ("%s\n", str);
              /* Prints: +1.41421356237309504880e+00 */
            }
          free (str);
        }
      printf ("%+-#*.20Qe\n", width, r);
      printf ("%Qa\n", r);
      printf ("%+-#46.*Qe\n", prec, r);
      printf ("%d %Qe %d %Qe %d %Qe\n", 1, r, 2, r, 3, r);
      return 0;
    }
    In any case, I think memcpy for loading from it is right.

    2024-04-03  Simon Chopin  <simon.chopin@canonical.com>
                Jakub Jelinek  <jakub@redhat.com>

            PR libquadmath/114533
            * printf/printf_fp.c (__quadmath_printf_fp): Use memcpy to copy
            __float128 out of args.
            * printf/printf_fphex.c (__quadmath_printf_fphex): Likewise.

    Signed-off-by: Simon Chopin <simon.chopin@canonical.com>

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #13 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:cc39bd532d4de1ba0b2785246fb6fdd63ec2e92c

commit r13-8625-gcc39bd532d4de1ba0b2785246fb6fdd63ec2e92c
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 3 10:02:35 2024 +0200

    libquadmath: Don't assume the storage for __float128 arguments is aligned
[PR114533]

    With the
register_printf_type/register_printf_modifier/register_printf_specifier
    APIs the C library is just told the size of the argument and is provided
with
    a callback to fetch the argument from va_list using va_arg into C library
provided
    memory.  The C library isn't told what alignment requirement it has, but we
were
    using direct load of a __float128 value from that memory which assumes
    __alignof (__float128) alignment.

    The following patch fixes that by using memcpy instead.

    I haven't been able to reproduce an actual crash, tried
     #include <quadmath.h>
     #include <stdlib.h>
     #include <stdio.h>

    int main ()
    {
      __float128 r;
      int prec = 20;
      int width = 46;
      char buf[128];

      r = 2.0q;
      r = sqrtq (r);
      int n = quadmath_snprintf (buf, sizeof buf, "%+-#*.20Qe", width, r);
      if ((size_t) n < sizeof buf)
        printf ("%s\n", buf);
        /* Prints: +1.41421356237309504880e+00 */
      quadmath_snprintf (buf, sizeof buf, "%Qa", r);
      if ((size_t) n < sizeof buf)
        printf ("%s\n", buf);
        /* Prints: 0x1.6a09e667f3bcc908b2fb1366ea96p+0 */
      n = quadmath_snprintf (NULL, 0, "%+-#46.*Qe", prec, r);
      if (n > -1)
        {
          char *str = malloc (n + 1);
          if (str)
            {
              quadmath_snprintf (str, n + 1, "%+-#46.*Qe", prec, r);
              printf ("%s\n", str);
              /* Prints: +1.41421356237309504880e+00 */
            }
          free (str);
        }
      printf ("%+-#*.20Qe\n", width, r);
      printf ("%Qa\n", r);
      printf ("%+-#46.*Qe\n", prec, r);
      printf ("%d %Qe %d %Qe %d %Qe\n", 1, r, 2, r, 3, r);
      return 0;
    }
    In any case, I think memcpy for loading from it is right.

    2024-04-03  Simon Chopin  <simon.chopin@canonical.com>
                Jakub Jelinek  <jakub@redhat.com>

            PR libquadmath/114533
            * printf/printf_fp.c (__quadmath_printf_fp): Use memcpy to copy
            __float128 out of args.
            * printf/printf_fphex.c (__quadmath_printf_fphex): Likewise.

    Signed-off-by: Simon Chopin <simon.chopin@canonical.com>
    (cherry picked from commit 8455d6f6cd43b7b143ab9ee19437452fceba9cc9)

[Bug libquadmath/114533] libquadmath: printf: fix misaligned access on args

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114533

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 13.3 too.