[PATCH] elf: fix handling of negative numbers in dl-printf

[PATCH] elf: fix handling of negative numbers in dl-printf

[Roy Eldar]

_dl_debug_vdprintf is a bare-bones printf implementation; currently
printing a signed integer (using "%d" format specifier) behaves
incorrectly when the number is negative, as it just prints the
corresponding unsigned integer, preceeded by a minus sign.

For example, _dl_printf("%d", -1) would print '-4294967295'.

Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
 elf/dl-printf.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/elf/dl-printf.c b/elf/dl-printf.c
index e8b9900370..977ac330b6 100644
--- a/elf/dl-printf.c
+++ b/elf/dl-printf.c
@@ -150,19 +150,25 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 		    if (long_mod)
 		      {
 			if ((long int) num < 0)
-			  negative = true;
+			  {
+			    num = -num;
+			    negative = true;
+			  }
 		      }
 		    else
 		      {
 			if ((int) num < 0)
 			  {
-			    num = (unsigned int) num;
+			    num = -(unsigned int) num;
 			    negative = true;
 			  }
 		      }
 #else
 		    if ((int) num < 0)
-		      negative = true;
+		      {
+			num = -num;
+			negative = true;
+		      }
 #endif
 		  }
 
-- 
2.30.2

Re: [PATCH] elf: fix handling of negative numbers in dl-printf

[Florian Weimer]

* Roy Eldar via Libc-alpha:

> _dl_debug_vdprintf is a bare-bones printf implementation; currently
> printing a signed integer (using "%d" format specifier) behaves
> incorrectly when the number is negative, as it just prints the
> corresponding unsigned integer, preceeded by a minus sign.
>
> For example, _dl_printf("%d", -1) would print '-4294967295'.
>
> Signed-off-by: Roy Eldar <royeldar0@gmail.com>

We've discussed this during our patch review call.

Ideally we'd want a separate test case for this routine.  You could add
a statically-linked test case that calls _dl_debug_vdprintf directly
with a pipe descriptor.  Is this something you'd be interested in
working on?

Thanks,
Florian

Re: [PATCH] elf: fix handling of negative numbers in dl-printf

[Roy E]

Hi,

First of all, thanks for the quick response.

Florian Weimer <fweimer@redhat.com> wrote:

> Ideally we'd want a separate test case for this routine.  You could add
> a statically-linked test case that calls _dl_debug_vdprintf directly
> with a pipe descriptor.  Is this something you'd be interested in
> working on?

I'll be happy to work on that.

Thanks,
Roy

Re: [PATCH] elf: fix handling of negative numbers in dl-printf

[Roy E]

Hi,

Is there any update on that?

I've written a test case for _dl_debug_vdprintf in a different patch [1].

[1] https://public-inbox.org/libc-alpha/20230515202942.8307-1-royeldar0@gmail.com/

[PATCH] elf: add test for dl-printf

[Roy Eldar]

This patch checks _dl_debug_vdprintf, by passing various inputs to
_dl_dprintf and comparing the output with invocations of snprintf.

Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
 elf/Makefile               |  1 +
 elf/tst-dl-printf-static.c | 75 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)
 create mode 100644 elf/tst-dl-printf-static.c

diff --git a/elf/Makefile b/elf/Makefile
index 396ec51424..90ccd65db1 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -275,6 +275,7 @@ tests-static-normal := \
 
 tests-static-internal := \
   tst-dl_find_object-static \
+  tst-dl-printf-static \
   tst-ptrguard1-static \
   tst-stackguard1-static \
   tst-tls1-static \
diff --git a/elf/tst-dl-printf-static.c b/elf/tst-dl-printf-static.c
new file mode 100644
index 0000000000..a31759a006
--- /dev/null
+++ b/elf/tst-dl-printf-static.c
@@ -0,0 +1,75 @@
+/* Check _dl_debug_vdprintf.
+   Copyright (C) 2016-2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <ldsodefs.h>
+#include <limits.h>
+#include <stdarg.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+
+#define BUFSZ 64
+
+#define TEST(fmt, ...) do {                                                   \
+    char str1[BUFSZ], str2[BUFSZ];                                            \
+    int len1 = snprintf (str1, BUFSZ, fmt, __VA_ARGS__);                      \
+    TEST_VERIFY_EXIT (len1 >= 0);                                             \
+    TEST_VERIFY_EXIT (len1 < BUFSZ);                                          \
+    _dl_dprintf (fds[1], fmt, __VA_ARGS__);                                   \
+    ssize_t len2 = read (fds[0], str2, BUFSZ);                                \
+    TEST_VERIFY_EXIT (len2 >= 0);                                             \
+    TEST_VERIFY_EXIT (len2 < BUFSZ);                                          \
+    str2[len2] = '\0';                                                        \
+    TEST_COMPARE_STRING(str1, str2);                                          \
+  } while (0)
+
+static int
+do_test (void)
+{
+  int fds[2];
+  xpipe (fds);
+  TEST ("%d", 0);
+  TEST ("%d", 1);
+  TEST ("%d", INT_MAX);
+  TEST ("%d", -1);
+  TEST ("%d", INT_MIN + 1);
+  TEST ("%d", INT_MIN);
+  TEST ("%u", 0U);
+  TEST ("%u", 1U);
+  TEST ("%u", UINT_MAX);
+  TEST ("%x", 0);
+  TEST ("%x", 1);
+  TEST ("%x", UINT_MAX);
+  TEST ("%ld", 0L);
+  TEST ("%ld", 1L);
+  TEST ("%ld", LONG_MAX);
+  TEST ("%ld", -1L);
+  TEST ("%ld", LONG_MIN + 1);
+  TEST ("%ld", LONG_MIN);
+  TEST ("%lu", 0UL);
+  TEST ("%lu", 1UL);
+  TEST ("%lu", ULONG_MAX);
+  TEST ("%lx", 0UL);
+  TEST ("%lx", 1UL);
+  TEST ("%lx", ULONG_MAX);
+  xclose (fds[0]);
+  xclose (fds[1]);
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.30.2

Re: [PATCH] elf: add test for dl-printf

[Florian Weimer]

* Roy Eldar:

> This patch checks _dl_debug_vdprintf, by passing various inputs to
> _dl_dprintf and comparing the output with invocations of snprintf.
>
> Signed-off-by: Roy Eldar <royeldar0@gmail.com>

Do you have a copyright assignment on file?  If not, Signed-off-by: is
sufficient but we should say …

> diff --git a/elf/tst-dl-printf-static.c b/elf/tst-dl-printf-static.c
> new file mode 100644
> index 0000000000..a31759a006
> --- /dev/null
> +++ b/elf/tst-dl-printf-static.c
> @@ -0,0 +1,75 @@
> +/* Check _dl_debug_vdprintf.
> +   Copyright (C) 2016-2023 Free Software Foundation, Inc.

… “Copyright The GNU Toolchain Authors.” here.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <ldsodefs.h>
> +#include <limits.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +
> +#define BUFSZ 64
> +
> +#define TEST(fmt, ...) do {                                                   \

GNU style puts { and } on their own lines.

> +    char str1[BUFSZ], str2[BUFSZ];                                            \
> +    int len1 = snprintf (str1, BUFSZ, fmt, __VA_ARGS__);                      \
> +    TEST_VERIFY_EXIT (len1 >= 0);                                             \
> +    TEST_VERIFY_EXIT (len1 < BUFSZ);                                          \
> +    _dl_dprintf (fds[1], fmt, __VA_ARGS__);                                   \
> +    ssize_t len2 = read (fds[0], str2, BUFSZ);                                \
> +    TEST_VERIFY_EXIT (len2 >= 0);                                             \
> +    TEST_VERIFY_EXIT (len2 < BUFSZ);                                          \
> +    str2[len2] = '\0';                                                        \
> +    TEST_COMPARE_STRING(str1, str2);                                          \

Missing space after TEST_COMPARE_STRING.

Rest looks good.

Thanks,
Florian

[PATCH v2] elf: add test for dl-printf

[Roy Eldar]

This patch checks _dl_debug_vdprintf, by passing various inputs to
_dl_dprintf and comparing the output with invocations of snprintf.

Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
 elf/Makefile               |  1 +
 elf/tst-dl-printf-static.c | 78 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)
 create mode 100644 elf/tst-dl-printf-static.c

diff --git a/elf/Makefile b/elf/Makefile
index 396ec51424..90ccd65db1 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -275,6 +275,7 @@ tests-static-normal := \
 
 tests-static-internal := \
   tst-dl_find_object-static \
+  tst-dl-printf-static \
   tst-ptrguard1-static \
   tst-stackguard1-static \
   tst-tls1-static \
diff --git a/elf/tst-dl-printf-static.c b/elf/tst-dl-printf-static.c
new file mode 100644
index 0000000000..da89c44183
--- /dev/null
+++ b/elf/tst-dl-printf-static.c
@@ -0,0 +1,78 @@
+/* Check _dl_debug_vdprintf.
+   Copyright The GNU Toolchain Authors.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <ldsodefs.h>
+#include <limits.h>
+#include <stdarg.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+
+#define BUFSZ 64
+
+#define TEST(fmt, ...)                                                        \
+  do                                                                          \
+    {                                                                         \
+      char str1[BUFSZ], str2[BUFSZ];                                          \
+      int len1 = snprintf (str1, BUFSZ, fmt, __VA_ARGS__);                    \
+      TEST_VERIFY_EXIT (len1 >= 0);                                           \
+      TEST_VERIFY_EXIT (len1 < BUFSZ);                                        \
+      _dl_dprintf (fds[1], fmt, __VA_ARGS__);                                 \
+      ssize_t len2 = read (fds[0], str2, BUFSZ);                              \
+      TEST_VERIFY_EXIT (len2 >= 0);                                           \
+      TEST_VERIFY_EXIT (len2 < BUFSZ);                                        \
+      str2[len2] = '\0';                                                      \
+      TEST_COMPARE_STRING (str1, str2);                                       \
+    }                                                                         \
+  while (0)
+
+static int
+do_test (void)
+{
+  int fds[2];
+  xpipe (fds);
+  TEST ("%d", 0);
+  TEST ("%d", 1);
+  TEST ("%d", INT_MAX);
+  TEST ("%d", -1);
+  TEST ("%d", INT_MIN + 1);
+  TEST ("%d", INT_MIN);
+  TEST ("%u", 0U);
+  TEST ("%u", 1U);
+  TEST ("%u", UINT_MAX);
+  TEST ("%x", 0);
+  TEST ("%x", 1);
+  TEST ("%x", UINT_MAX);
+  TEST ("%ld", 0L);
+  TEST ("%ld", 1L);
+  TEST ("%ld", LONG_MAX);
+  TEST ("%ld", -1L);
+  TEST ("%ld", LONG_MIN + 1);
+  TEST ("%ld", LONG_MIN);
+  TEST ("%lu", 0UL);
+  TEST ("%lu", 1UL);
+  TEST ("%lu", ULONG_MAX);
+  TEST ("%lx", 0UL);
+  TEST ("%lx", 1UL);
+  TEST ("%lx", ULONG_MAX);
+  xclose (fds[0]);
+  xclose (fds[1]);
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.30.2

Re: [PATCH v2] elf: add test for dl-printf

[Florian Weimer]

* Roy Eldar:

> This patch checks _dl_debug_vdprintf, by passing various inputs to
> _dl_dprintf and comparing the output with invocations of snprintf.
>
> Signed-off-by: Roy Eldar <royeldar0@gmail.com>

Reviewed-by: Florian Weimer <fweimer@redhat.com>

And pushed.

Thanks,
Florian

Re: [PATCH] elf: fix handling of negative numbers in dl-printf

[Florian Weimer]

* Roy Eldar via Libc-alpha:

> _dl_debug_vdprintf is a bare-bones printf implementation; currently
> printing a signed integer (using "%d" format specifier) behaves
> incorrectly when the number is negative, as it just prints the
> corresponding unsigned integer, preceeded by a minus sign.
>
> For example, _dl_printf("%d", -1) would print '-4294967295'.
>
> Signed-off-by: Roy Eldar <royeldar0@gmail.com>
> ---
>  elf/dl-printf.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/elf/dl-printf.c b/elf/dl-printf.c
> index e8b9900370..977ac330b6 100644
> --- a/elf/dl-printf.c
> +++ b/elf/dl-printf.c

Please update the copyright staement with

  Copyright The GNU Toolchain Authors.

Then I'm going to apply it together with the test.

Thanks,
Florian

[PATCH v2] elf: fix handling of negative numbers in dl-printf

[Roy Eldar]

_dl_debug_vdprintf is a bare-bones printf implementation; currently
printing a signed integer (using "%d" format specifier) behaves
incorrectly when the number is negative, as it just prints the
corresponding unsigned integer, preceeded by a minus sign.

For example, _dl_printf("%d", -1) would print '-4294967295'.

Signed-off-by: Roy Eldar <royeldar0@gmail.com>
---
 elf/dl-printf.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/elf/dl-printf.c b/elf/dl-printf.c
index e8b9900370..6efb4c019a 100644
--- a/elf/dl-printf.c
+++ b/elf/dl-printf.c
@@ -1,5 +1,6 @@
 /* printf implementation for the dynamic loader.
    Copyright (C) 1997-2023 Free Software Foundation, Inc.
+   Copyright The GNU Toolchain Authors.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -150,19 +151,25 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 		    if (long_mod)
 		      {
 			if ((long int) num < 0)
-			  negative = true;
+			  {
+			    num = -num;
+			    negative = true;
+			  }
 		      }
 		    else
 		      {
 			if ((int) num < 0)
 			  {
-			    num = (unsigned int) num;
+			    num = -(unsigned int) num;
 			    negative = true;
 			  }
 		      }
 #else
 		    if ((int) num < 0)
-		      negative = true;
+		      {
+			num = -num;
+			negative = true;
+		      }
 #endif
 		  }
 
-- 
2.30.2

Re: [PATCH v2] elf: fix handling of negative numbers in dl-printf

[Florian Weimer]

* Roy Eldar:

> _dl_debug_vdprintf is a bare-bones printf implementation; currently
> printing a signed integer (using "%d" format specifier) behaves
> incorrectly when the number is negative, as it just prints the
> corresponding unsigned integer, preceeded by a minus sign.
>
> For example, _dl_printf("%d", -1) would print '-4294967295'.
>
> Signed-off-by: Roy Eldar <royeldar0@gmail.com>

Reviewed-by: Florian Weimer <fweimer@redhat.com>

And pushed.

Thanks,
Florian