[PATCH] tst-fmon/tst-numeric: switch malloc to static stack space [BZ #19671]

[PATCH] tst-fmon/tst-numeric: switch malloc to static stack space [BZ #19671]

[Mike Frysinger]

The current test code doesn't check the return value of malloc.
This should rarely (if ever) cause a problem, but rather than add
some return value checks, just statically allocate the buffer on
the stack.  This will never fail (or if it does, we've got much
bigger problems that don't matter to the test).

Checked that the tests still pass on x86_64-linux-gnu.
---
 localedata/tst-fmon.c    | 2 +-
 localedata/tst-numeric.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/localedata/tst-fmon.c b/localedata/tst-fmon.c
index 995cf90..1359775 100644
--- a/localedata/tst-fmon.c
+++ b/localedata/tst-fmon.c
@@ -40,7 +40,7 @@
 int
 main (int argc, char *argv[])
 {
-  char *s = malloc (201);
+  char s[201];
 
   if (setlocale (LC_MONETARY, argv[1]) == NULL)
     {
diff --git a/localedata/tst-numeric.c b/localedata/tst-numeric.c
index 46a6b48..ac06965 100644
--- a/localedata/tst-numeric.c
+++ b/localedata/tst-numeric.c
@@ -41,7 +41,7 @@
 int
 main (int argc, char *argv[])
 {
-  char *s = malloc (201);
+  char s[201];
   double val;
 
   /* Make sure to read the value before setting of the locale, as
-- 
2.7.4

Re: [PATCH] tst-fmon/tst-numeric: switch malloc to static stack space [BZ #19671]

[Florian Weimer]

* Mike Frysinger:

> The current test code doesn't check the return value of malloc.
> This should rarely (if ever) cause a problem, but rather than add
> some return value checks, just statically allocate the buffer on
> the stack.  This will never fail (or if it does, we've got much
> bigger problems that don't matter to the test).

This needs a ChangeLog entry.

> -  char *s = malloc (201);
> +  char s[201];

Please use a enum constant of 200, and also pass it to strfmon.
K think the current 200/201 choice is technically incorrect
(the maximum includes the terminating null byte).

Re: [PATCH] tst-fmon/tst-numeric: switch malloc to static stack space [BZ #19671]

[Mike Frysinger]

[-- Attachment #1.1: Type: text/plain, Size: 853 bytes --]

On 23 Apr 2016 14:23, Florian Weimer wrote:
> * Mike Frysinger:
> > The current test code doesn't check the return value of malloc.
> > This should rarely (if ever) cause a problem, but rather than add
> > some return value checks, just statically allocate the buffer on
> > the stack.  This will never fail (or if it does, we've got much
> > bigger problems that don't matter to the test).
> 
> This needs a ChangeLog entry.

i don't bother writing ChangeLog entries until before i push.
it helps minimize time wastage.

> > -  char *s = malloc (201);
> > +  char s[201];
> 
> Please use a enum constant of 200, and also pass it to strfmon.
> K think the current 200/201 choice is technically incorrect
> (the maximum includes the terminating null byte).

these two tests are both badly written.  i've changed it to sizeof.
-mike

[-- Attachment #1.2: 0001-tst-fmon-tst-numeric-switch-malloc-to-static-stack-s.patch --]
[-- Type: text/x-diff, Size: 1925 bytes --]

From 54e15c89dbb0806a6924b0e573fc64a2a839c2fb Mon Sep 17 00:00:00 2001
From: Mike Frysinger <vapier@gentoo.org>
Date: Fri, 22 Apr 2016 17:11:09 -0400
Subject: [PATCH] tst-fmon/tst-numeric: switch malloc to static stack space [BZ
 #19671]

The current test code doesn't check the return value of malloc.
This should rarely (if ever) cause a problem, but rather than add
some return value checks, just statically allocate the buffer on
the stack.  This will never fail (or if it does, we've got much
bigger problems that don't matter to the test).
---
 localedata/tst-fmon.c    | 4 ++--
 localedata/tst-numeric.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/localedata/tst-fmon.c b/localedata/tst-fmon.c
index 995cf90..62cd7db 100644
--- a/localedata/tst-fmon.c
+++ b/localedata/tst-fmon.c
@@ -40,7 +40,7 @@
 int
 main (int argc, char *argv[])
 {
-  char *s = malloc (201);
+  char s[200];
 
   if (setlocale (LC_MONETARY, argv[1]) == NULL)
     {
@@ -48,7 +48,7 @@ main (int argc, char *argv[])
       exit (EXIT_SETLOCALE);
     }
 
-  if (strfmon (s, 200, argv[2], (double) atof (argv[3])) == -1)
+  if (strfmon (s, sizeof (s), argv[2], (double) atof (argv[3])) == -1)
     {
       perror ("strfmon");
       exit (EXIT_STRFMON);
diff --git a/localedata/tst-numeric.c b/localedata/tst-numeric.c
index 46a6b48..64abb0a 100644
--- a/localedata/tst-numeric.c
+++ b/localedata/tst-numeric.c
@@ -41,7 +41,7 @@
 int
 main (int argc, char *argv[])
 {
-  char *s = malloc (201);
+  char s[200];
   double val;
 
   /* Make sure to read the value before setting of the locale, as
@@ -54,7 +54,7 @@ main (int argc, char *argv[])
       exit (EXIT_SETLOCALE);
     }
 
-  if (snprintf (s, 200, argv[2], val) == -1)
+  if (snprintf (s, sizeof (s), argv[2], val) == -1)
     {
       perror ("snprintf");
       exit (EXIT_SNPRINTF);
-- 
2.7.4


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] tst-fmon/tst-numeric: switch malloc to static stack space [BZ #19671]

[Florian Weimer]

* Mike Frysinger:

>> Please use a enum constant of 200, and also pass it to strfmon.
>> K think the current 200/201 choice is technically incorrect
>> (the maximum includes the terminating null byte).
>
> these two tests are both badly written.  i've changed it to sizeof.

Thanks.  It's also annoying that the file is named tst-fmon, and not
tst-strfmon, which is why I missed it when I contributed
stdlib/tst-strfmon_l.c (it also did not occur to me to look at the
localedata subdirectory).  I wonder if we can add an internal
consistency check to tst-fmon and thus get rid of tst-strfmon_l.
(Obviously, this is a separate change.)

I wonder if the malloc allocation was done so that it is possible to
detect overflows in the implementation because that's easier for heap
overflows than for stack overflows.  It's the reason why I use malloc
in some tests.

In this case, the buffer length is not even close to what is actually
used by the output, so it's not a valid concern, and the removal of
malloc is appropriate.