* [PATCH] elf: fix handling of negative numbers in dl-printf @ 2023-05-03 21:16 Roy Eldar 2023-05-08 13:42 ` Florian Weimer 2023-05-25 7:46 ` [PATCH] elf: fix handling of negative numbers in dl-printf Florian Weimer 0 siblings, 2 replies; 11+ messages in thread From: Roy Eldar @ 2023-05-03 21:16 UTC (permalink / raw) To: libc-alpha; +Cc: Roy Eldar, carlos _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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] elf: fix handling of negative numbers in dl-printf 2023-05-03 21:16 [PATCH] elf: fix handling of negative numbers in dl-printf Roy Eldar @ 2023-05-08 13:42 ` Florian Weimer 2023-05-08 18:08 ` Roy E 2023-05-15 20:29 ` [PATCH] elf: add test for dl-printf Roy Eldar 2023-05-25 7:46 ` [PATCH] elf: fix handling of negative numbers in dl-printf Florian Weimer 1 sibling, 2 replies; 11+ messages in thread From: Florian Weimer @ 2023-05-08 13:42 UTC (permalink / raw) To: Roy Eldar via Libc-alpha; +Cc: Roy Eldar, carlos * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] elf: fix handling of negative numbers in dl-printf 2023-05-08 13:42 ` Florian Weimer @ 2023-05-08 18:08 ` Roy E 2023-05-24 17:41 ` Roy E 2023-05-15 20:29 ` [PATCH] elf: add test for dl-printf Roy Eldar 1 sibling, 1 reply; 11+ messages in thread From: Roy E @ 2023-05-08 18:08 UTC (permalink / raw) To: Florian Weimer; +Cc: carlos, libc-alpha 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] elf: fix handling of negative numbers in dl-printf 2023-05-08 18:08 ` Roy E @ 2023-05-24 17:41 ` Roy E 0 siblings, 0 replies; 11+ messages in thread From: Roy E @ 2023-05-24 17:41 UTC (permalink / raw) To: Florian Weimer; +Cc: carlos, libc-alpha 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] elf: add test for dl-printf 2023-05-08 13:42 ` Florian Weimer 2023-05-08 18:08 ` Roy E @ 2023-05-15 20:29 ` Roy Eldar 2023-05-25 7:44 ` Florian Weimer 1 sibling, 1 reply; 11+ messages in thread From: Roy Eldar @ 2023-05-15 20:29 UTC (permalink / raw) To: fweimer; +Cc: Roy Eldar, libc-alpha, carlos 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] elf: add test for dl-printf 2023-05-15 20:29 ` [PATCH] elf: add test for dl-printf Roy Eldar @ 2023-05-25 7:44 ` Florian Weimer 2023-05-25 14:41 ` [PATCH v2] " Roy Eldar 0 siblings, 1 reply; 11+ messages in thread From: Florian Weimer @ 2023-05-25 7:44 UTC (permalink / raw) To: Roy Eldar; +Cc: libc-alpha, carlos * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] elf: add test for dl-printf 2023-05-25 7:44 ` Florian Weimer @ 2023-05-25 14:41 ` Roy Eldar 2023-05-25 17:10 ` Florian Weimer 0 siblings, 1 reply; 11+ messages in thread From: Roy Eldar @ 2023-05-25 14:41 UTC (permalink / raw) To: fweimer; +Cc: Roy Eldar, libc-alpha, carlos 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] elf: add test for dl-printf 2023-05-25 14:41 ` [PATCH v2] " Roy Eldar @ 2023-05-25 17:10 ` Florian Weimer 0 siblings, 0 replies; 11+ messages in thread From: Florian Weimer @ 2023-05-25 17:10 UTC (permalink / raw) To: Roy Eldar; +Cc: libc-alpha, carlos * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] elf: fix handling of negative numbers in dl-printf 2023-05-03 21:16 [PATCH] elf: fix handling of negative numbers in dl-printf Roy Eldar 2023-05-08 13:42 ` Florian Weimer @ 2023-05-25 7:46 ` Florian Weimer 2023-05-25 14:41 ` [PATCH v2] " Roy Eldar 1 sibling, 1 reply; 11+ messages in thread From: Florian Weimer @ 2023-05-25 7:46 UTC (permalink / raw) To: Roy Eldar via Libc-alpha; +Cc: Roy Eldar, carlos * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] elf: fix handling of negative numbers in dl-printf 2023-05-25 7:46 ` [PATCH] elf: fix handling of negative numbers in dl-printf Florian Weimer @ 2023-05-25 14:41 ` Roy Eldar 2023-05-25 17:10 ` Florian Weimer 0 siblings, 1 reply; 11+ messages in thread From: Roy Eldar @ 2023-05-25 14:41 UTC (permalink / raw) To: fweimer; +Cc: Roy Eldar, libc-alpha, carlos _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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] elf: fix handling of negative numbers in dl-printf 2023-05-25 14:41 ` [PATCH v2] " Roy Eldar @ 2023-05-25 17:10 ` Florian Weimer 0 siblings, 0 replies; 11+ messages in thread From: Florian Weimer @ 2023-05-25 17:10 UTC (permalink / raw) To: Roy Eldar; +Cc: libc-alpha, carlos * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-05-25 17:10 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-03 21:16 [PATCH] elf: fix handling of negative numbers in dl-printf Roy Eldar 2023-05-08 13:42 ` Florian Weimer 2023-05-08 18:08 ` Roy E 2023-05-24 17:41 ` Roy E 2023-05-15 20:29 ` [PATCH] elf: add test for dl-printf Roy Eldar 2023-05-25 7:44 ` Florian Weimer 2023-05-25 14:41 ` [PATCH v2] " Roy Eldar 2023-05-25 17:10 ` Florian Weimer 2023-05-25 7:46 ` [PATCH] elf: fix handling of negative numbers in dl-printf Florian Weimer 2023-05-25 14:41 ` [PATCH v2] " Roy Eldar 2023-05-25 17:10 ` Florian Weimer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).