[PATCH] libquadmath: printf: fix misaligned access on args

[PATCH] libquadmath: printf: fix misaligned access on args

[Simon Chopin]

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 alignement, 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))

base-commit: 39737cdf002637c7a652e9c3e36f369cfce581e5
-- 
2.43.0

Re: [PATCH] libquadmath: printf: fix misaligned access on args

[Florian Weimer]

* Simon Chopin:

> 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 alignement, whereas alloca() was
> probably more likely to be naturally aligned.

This glibc change appears to be incorrect.  I think we need to preserve
ABI alignment for types than can be passed through the vararg interface.
I'm not sure if this easily possible, though.  Certainly needs a
discussion on libc-alpha.

Thanks,
Florian

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

[Jakub Jelinek]

On Tue, Mar 12, 2024 at 08:03:52PM +0100, Simon Chopin wrote:
> 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 alignement, 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

The formatting was incorrect and we need to also change it in another
place.

Here is what I've committed instead:

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>

--- libquadmath/printf/printf_fp.c.jj	2020-01-12 11:54:39.787362505 +0100
+++ libquadmath/printf/printf_fp.c	2024-04-02 19:28:31.254670746 +0200
@@ -363,7 +363,7 @@ __quadmath_printf_fp (struct __quadmath_
 
   /* Fetch the argument value.	*/
     {
-      fpnum = **(const __float128 **) args[0];
+      memcpy (&fpnum, *(const void *const *) args[0], sizeof (fpnum));
 
       /* Check for special values: not a number or infinity.  */
       if (isnanq (fpnum))
--- libquadmath/printf/printf_fphex.c.jj	2020-01-12 11:54:39.787362505 +0100
+++ libquadmath/printf/printf_fphex.c	2024-04-02 19:29:03.968223151 +0200
@@ -163,7 +163,8 @@ __quadmath_printf_fphex (struct __quadma
 
   /* Fetch the argument value.	*/
     {
-      fpnum.value = **(const __float128 **) args[0];
+      memcpy (&fpnum.value, *(const void *const *) args[0],
+	      sizeof (fpnum.value));
 
       /* Check for special values: not a number or infinity.  */
       if (isnanq (fpnum.value))


	Jakub