public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree
@ 2018-07-23 16:48 Paul Pluzhnikov
  2018-07-30 20:13 ` Adhemerval Zanella
  0 siblings, 1 reply; 7+ messages in thread
From: Paul Pluzhnikov @ 2018-07-23 16:48 UTC (permalink / raw)
  To: GLIBC Devel

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]

Greetings,

Attached patch fixes BZ 23400, by using standard support for creating
temporary directories.

2018-07-23  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #23400]
        * stdlib/test-bz22786.c (do_test): Use support_create_temp_directory.

-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz23400-20180721.txt --]
[-- Type: text/plain, Size: 1794 bytes --]

diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
index e7837f98c1..f7635c785f 100644
--- a/stdlib/test-bz22786.c
+++ b/stdlib/test-bz22786.c
@@ -26,22 +26,21 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <support/support.h>
+#include <support/temp_file.h>
 #include <support/test-driver.h>
 #include <libc-diag.h>
 
 static int
 do_test (void)
 {
-  const char dir[] = "bz22786";
-  const char lnk[] = "bz22786/symlink";
+  const char *dir = support_create_temp_directory ("bz22786.");
+  char *lnk = xmalloc (strlen (dir) + strlen ("/symlink") + 1);
 
-  rmdir (dir);
-  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
-    {
-      printf ("mkdir %s: %m\n", dir);
-      return EXIT_FAILURE;
-    }
-  if (symlink (".", lnk) != 0 && errno != EEXIST)
+  strcpy (lnk, dir);
+  strcat (lnk, "/symlink");
+
+  if (symlink (".", lnk) != 0)
     {
       printf ("symlink (%s, %s): %m\n", dir, lnk);
       return EXIT_FAILURE;
@@ -55,17 +54,11 @@ do_test (void)
      allocation to succeed for the test to work.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  char *path = malloc (path_len);
+  char *path = xmalloc (path_len);
   DIAG_POP_NEEDS_COMMENT;
 
-  if (path == NULL)
-    {
-      printf ("malloc (%zu): %m\n", path_len);
-      return EXIT_UNSUPPORTED;
-    }
-
-  /* Construct very long path = "bz22786/symlink/aaaa....."  */
-  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
+  /* Construct very long path = "/tmp/bz22786.XXXX/symlink/aaaa....."  */
+  char *p = mempcpy (path, lnk, strlen (lnk));
   *(p++) = '/';
   memset (p, 'a', path_len - (path - p) - 2);
   p[path_len - (path - p) - 1] = '\0';
@@ -81,7 +74,6 @@ do_test (void)
 
   /* Cleanup.  */
   unlink (lnk);
-  rmdir (dir);
 
   return 0;
 }

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree
  2018-07-23 16:48 [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree Paul Pluzhnikov
@ 2018-07-30 20:13 ` Adhemerval Zanella
  2018-08-06 15:12   ` Paul Pluzhnikov
  0 siblings, 1 reply; 7+ messages in thread
From: Adhemerval Zanella @ 2018-07-30 20:13 UTC (permalink / raw)
  To: libc-alpha



On 23/07/2018 13:48, Paul Pluzhnikov wrote:
> Greetings,
> 
> Attached patch fixes BZ 23400, by using standard support for creating
> temporary directories.
> 
> 2018-07-23  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         [BZ #23400]
>         * stdlib/test-bz22786.c (do_test): Use support_create_temp_directory.
> 
> -- Paul Pluzhnikov
> 
> 
> glibc-bz23400-20180721.txt
> 
> 
> diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
> index e7837f98c1..f7635c785f 100644
> --- a/stdlib/test-bz22786.c
> +++ b/stdlib/test-bz22786.c
> @@ -26,22 +26,21 @@
>  #include <unistd.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
>  #include <support/test-driver.h>
>  #include <libc-diag.h>
>  
>  static int
>  do_test (void)
>  {
> -  const char dir[] = "bz22786";
> -  const char lnk[] = "bz22786/symlink";
> +  const char *dir = support_create_temp_directory ("bz22786.");
> +  char *lnk = xmalloc (strlen (dir) + strlen ("/symlink") + 1);
>  
> -  rmdir (dir);
> -  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
> -    {
> -      printf ("mkdir %s: %m\n", dir);
> -      return EXIT_FAILURE;
> -    }
> -  if (symlink (".", lnk) != 0 && errno != EEXIST)
> +  strcpy (lnk, dir);
> +  strcat (lnk, "/symlink");

Maybe just 'char *lnk = xasprintf ("%s/symlink", dir);' instead?

> +
> +  if (symlink (".", lnk) != 0)
>      {
>        printf ("symlink (%s, %s): %m\n", dir, lnk);
>        return EXIT_FAILURE;

Use FAIL_EXIT1 or just TEST_VERIFY_EXIT.


> @@ -55,17 +54,11 @@ do_test (void)
>       allocation to succeed for the test to work.  */
>    DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
>  #endif
> -  char *path = malloc (path_len);
> +  char *path = xmalloc (path_len);
>    DIAG_POP_NEEDS_COMMENT;
>  
> -  if (path == NULL)
> -    {
> -      printf ("malloc (%zu): %m\n", path_len);
> -      return EXIT_UNSUPPORTED;
> -    }
> -
> -  /* Construct very long path = "bz22786/symlink/aaaa....."  */
> -  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
> +  /* Construct very long path = "/tmp/bz22786.XXXX/symlink/aaaa....."  */
> +  char *p = mempcpy (path, lnk, strlen (lnk));
>    *(p++) = '/';
>    memset (p, 'a', path_len - (path - p) - 2);
>    p[path_len - (path - p) - 1] = '\0';

Shouldn't it 'p - path' instead? The subtraction is clearly issuing a
overflow and I think it is not what the test meant here.

> @@ -81,7 +74,6 @@ do_test (void)
>  
>    /* Cleanup.  */
>    unlink (lnk);
> -  rmdir (dir);
>  
>    return 0;
>  }
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree
  2018-07-30 20:13 ` Adhemerval Zanella
@ 2018-08-06 15:12   ` Paul Pluzhnikov
  2018-08-07 11:20     ` Adhemerval Zanella
  2018-08-29 13:25     ` Stefan Liebler
  0 siblings, 2 replies; 7+ messages in thread
From: Paul Pluzhnikov @ 2018-08-06 15:12 UTC (permalink / raw)
  To: Adhemerval Zanella; +Cc: GLIBC Devel

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

Thanks for review!

On Mon, Jul 30, 2018 at 1:13 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:

> > +  strcpy (lnk, dir);
> > +  strcat (lnk, "/symlink");
>
> Maybe just 'char *lnk = xasprintf ("%s/symlink", dir);' instead?

Done.

> > +  if (symlink (".", lnk) != 0)
> >      {
> >        printf ("symlink (%s, %s): %m\n", dir, lnk);
> >        return EXIT_FAILURE;
>
> Use FAIL_EXIT1 or just TEST_VERIFY_EXIT.

Done.

> >    memset (p, 'a', path_len - (path - p) - 2);
> >    p[path_len - (path - p) - 1] = '\0';
>
> Shouldn't it 'p - path' instead? The subtraction is clearly issuing a
> overflow and I think it is not what the test meant here.

Good catch. Turns out that this was a buffer overflow in the original
test. Fixed.

Thanks,

2018-08-06  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #23400]
        * stdlib/test-bz22786.c (do_test): Fix undefined behavior.

-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz23400-20180805.txt --]
[-- Type: text/plain, Size: 2144 bytes --]

diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
index e7837f98c1..879d61dafa 100644
--- a/stdlib/test-bz22786.c
+++ b/stdlib/test-bz22786.c
@@ -26,28 +26,20 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
 #include <support/test-driver.h>
 #include <libc-diag.h>
 
 static int
 do_test (void)
 {
-  const char dir[] = "bz22786";
-  const char lnk[] = "bz22786/symlink";
+  const char *dir = support_create_temp_directory ("bz22786.");
+  const char *lnk = xasprintf ("%s/symlink", dir);
+  const size_t path_len = (size_t) INT_MAX + strlen (lnk) + 1;
 
-  rmdir (dir);
-  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
-    {
-      printf ("mkdir %s: %m\n", dir);
-      return EXIT_FAILURE;
-    }
-  if (symlink (".", lnk) != 0 && errno != EEXIST)
-    {
-      printf ("symlink (%s, %s): %m\n", dir, lnk);
-      return EXIT_FAILURE;
-    }
-
-  const size_t path_len = (size_t) INT_MAX + 1;
+  TEST_VERIFY_EXIT (symlink (".", lnk) == 0);
 
   DIAG_PUSH_NEEDS_COMMENT;
 #if __GNUC_PREREQ (7, 0)
@@ -55,20 +47,14 @@ do_test (void)
      allocation to succeed for the test to work.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  char *path = malloc (path_len);
+  char *path = xmalloc (path_len);
   DIAG_POP_NEEDS_COMMENT;
 
-  if (path == NULL)
-    {
-      printf ("malloc (%zu): %m\n", path_len);
-      return EXIT_UNSUPPORTED;
-    }
-
-  /* Construct very long path = "bz22786/symlink/aaaa....."  */
-  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
+  /* Construct very long path = "/tmp/bz22786.XXXX/symlink/aaaa....."  */
+  char *p = mempcpy (path, lnk, strlen (lnk));
   *(p++) = '/';
-  memset (p, 'a', path_len - (path - p) - 2);
-  p[path_len - (path - p) - 1] = '\0';
+  memset (p, 'a', path_len - (p - path) - 2);
+  p[path_len - (p - path) - 1] = '\0';
 
   /* This call crashes before the fix for bz22786 on 32-bit platforms.  */
   p = realpath (path, NULL);
@@ -81,7 +67,6 @@ do_test (void)
 
   /* Cleanup.  */
   unlink (lnk);
-  rmdir (dir);
 
   return 0;
 }

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree
  2018-08-06 15:12   ` Paul Pluzhnikov
@ 2018-08-07 11:20     ` Adhemerval Zanella
  2018-08-29 13:25     ` Stefan Liebler
  1 sibling, 0 replies; 7+ messages in thread
From: Adhemerval Zanella @ 2018-08-07 11:20 UTC (permalink / raw)
  To: Paul Pluzhnikov; +Cc: GLIBC Devel



On 06/08/2018 12:12, Paul Pluzhnikov wrote:
> Thanks for review!
> 
> On Mon, Jul 30, 2018 at 1:13 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
> 
>>> +  strcpy (lnk, dir);
>>> +  strcat (lnk, "/symlink");
>> Maybe just 'char *lnk = xasprintf ("%s/symlink", dir);' instead?
> Done.
> 
>>> +  if (symlink (".", lnk) != 0)
>>>      {
>>>        printf ("symlink (%s, %s): %m\n", dir, lnk);
>>>        return EXIT_FAILURE;
>> Use FAIL_EXIT1 or just TEST_VERIFY_EXIT.
> Done.
> 
>>>    memset (p, 'a', path_len - (path - p) - 2);
>>>    p[path_len - (path - p) - 1] = '\0';
>> Shouldn't it 'p - path' instead? The subtraction is clearly issuing a
>> overflow and I think it is not what the test meant here.
> Good catch. Turns out that this was a buffer overflow in the original
> test. Fixed.
> 
> Thanks,
> 
> 2018-08-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         [BZ #23400]
>         * stdlib/test-bz22786.c (do_test): Fix undefined behavior.

Add that it fix the temporary file creation in glibc source tree as
well. LGTM, thanks.

> 
> -- Paul Pluzhnikov
> 
> 
> glibc-bz23400-20180805.txt
> 
> 
> diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
> index e7837f98c1..879d61dafa 100644
> --- a/stdlib/test-bz22786.c
> +++ b/stdlib/test-bz22786.c
> @@ -26,28 +26,20 @@
>  #include <unistd.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
>  #include <support/test-driver.h>
>  #include <libc-diag.h>
>  
>  static int
>  do_test (void)
>  {
> -  const char dir[] = "bz22786";
> -  const char lnk[] = "bz22786/symlink";
> +  const char *dir = support_create_temp_directory ("bz22786.");
> +  const char *lnk = xasprintf ("%s/symlink", dir);
> +  const size_t path_len = (size_t) INT_MAX + strlen (lnk) + 1;
>  
> -  rmdir (dir);
> -  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
> -    {
> -      printf ("mkdir %s: %m\n", dir);
> -      return EXIT_FAILURE;
> -    }
> -  if (symlink (".", lnk) != 0 && errno != EEXIST)
> -    {
> -      printf ("symlink (%s, %s): %m\n", dir, lnk);
> -      return EXIT_FAILURE;
> -    }
> -
> -  const size_t path_len = (size_t) INT_MAX + 1;
> +  TEST_VERIFY_EXIT (symlink (".", lnk) == 0);
>  
>    DIAG_PUSH_NEEDS_COMMENT;
>  #if __GNUC_PREREQ (7, 0)
> @@ -55,20 +47,14 @@ do_test (void)
>       allocation to succeed for the test to work.  */
>    DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
>  #endif
> -  char *path = malloc (path_len);
> +  char *path = xmalloc (path_len);
>    DIAG_POP_NEEDS_COMMENT;
>  
> -  if (path == NULL)
> -    {
> -      printf ("malloc (%zu): %m\n", path_len);
> -      return EXIT_UNSUPPORTED;
> -    }
> -
> -  /* Construct very long path = "bz22786/symlink/aaaa....."  */
> -  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
> +  /* Construct very long path = "/tmp/bz22786.XXXX/symlink/aaaa....."  */
> +  char *p = mempcpy (path, lnk, strlen (lnk));
>    *(p++) = '/';
> -  memset (p, 'a', path_len - (path - p) - 2);
> -  p[path_len - (path - p) - 1] = '\0';
> +  memset (p, 'a', path_len - (p - path) - 2);
> +  p[path_len - (p - path) - 1] = '\0';
>  
>    /* This call crashes before the fix for bz22786 on 32-bit platforms.  */
>    p = realpath (path, NULL);
> @@ -81,7 +67,6 @@ do_test (void)
>  
>    /* Cleanup.  */
>    unlink (lnk);
> -  rmdir (dir);
>  
>    return 0;
>  }
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree
  2018-08-06 15:12   ` Paul Pluzhnikov
  2018-08-07 11:20     ` Adhemerval Zanella
@ 2018-08-29 13:25     ` Stefan Liebler
  2018-08-29 14:05       ` Carlos O'Donell
  1 sibling, 1 reply; 7+ messages in thread
From: Stefan Liebler @ 2018-08-29 13:25 UTC (permalink / raw)
  To: libc-alpha

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

Hi Paul,

If I run the test on s390 (31bit), the test fails with:
malloc: unable to allocate 2147483675 bytes: Cannot allocate memory
warning: could not remove temporary file: /tmp/bz22786.iaoLYS: Directory 
not empty

This test tries to allocate a little bit too much bytes for 31bit. Thus 
the former implementation returned EXIT_UNSUPPORTED. Now the test is 
failing due to xmalloc.

As the symlink was created within the temporary directory 
/tmp/bz22786.XXXXXX, but was not unlinked by the test, 
support_delete_temp_files() fails to remove it.

Can we just return EXIT_UNSUPPORTED on 31bit as done with the attached 
patch?

Bye
Stefan

On 08/06/2018 05:12 PM, Paul Pluzhnikov wrote:
> Thanks for review!
> 
> On Mon, Jul 30, 2018 at 1:13 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
> 
>>> +  strcpy (lnk, dir);
>>> +  strcat (lnk, "/symlink");
>>
>> Maybe just 'char *lnk = xasprintf ("%s/symlink", dir);' instead?
> 
> Done.
> 
>>> +  if (symlink (".", lnk) != 0)
>>>       {
>>>         printf ("symlink (%s, %s): %m\n", dir, lnk);
>>>         return EXIT_FAILURE;
>>
>> Use FAIL_EXIT1 or just TEST_VERIFY_EXIT.
> 
> Done.
> 
>>>     memset (p, 'a', path_len - (path - p) - 2);
>>>     p[path_len - (path - p) - 1] = '\0';
>>
>> Shouldn't it 'p - path' instead? The subtraction is clearly issuing a
>> overflow and I think it is not what the test meant here.
> 
> Good catch. Turns out that this was a buffer overflow in the original
> test. Fixed.
> 
> Thanks,
> 
> 2018-08-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>          [BZ #23400]
>          * stdlib/test-bz22786.c (do_test): Fix undefined behavior.
> 


[-- Attachment #2: 20180829_test-bz22786.patch --]
[-- Type: text/x-patch, Size: 1802 bytes --]

commit 5ada1975be8f1b30b8f33d1d25cb5575690066e1
Author: Stefan Liebler <stli@linux.ibm.com>
Date:   Wed Aug 29 15:20:51 2018 +0200

    Test stdlib/test-bz22786 exits now with unsupported if malloc fails.
    
    The test tries to allocate more than 2^31 bytes which will always fail on s390
    as it has maximum 31bit of memory.
    Before commit 6c3a8a9d868a8deddf0d6dcc785b6d120de90523, this test returned
    unsupported if malloc fails.  This patch re enables this behaviour.
    
    Furthermore support_delete_temp_files() failed to remove the temp directory
    in this case as it is not empty due to the created symlink.
    Thus the creation of the symlink is moved behind malloc.
    
    ChangeLog
    
            * stdlib/test-bz22786.c (do_test): Return EXIT_UNSUPPORTED
            if malloc fails.

diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
index d1aa69106c..44ec631a96 100644
--- a/stdlib/test-bz22786.c
+++ b/stdlib/test-bz22786.c
@@ -39,16 +39,21 @@ do_test (void)
   const char *lnk = xasprintf ("%s/symlink", dir);
   const size_t path_len = (size_t) INT_MAX + strlen (lnk) + 1;
 
-  TEST_VERIFY_EXIT (symlink (".", lnk) == 0);
-
   DIAG_PUSH_NEEDS_COMMENT;
 #if __GNUC_PREREQ (7, 0)
   /* GCC 7 warns about too-large allocations; here we need such
      allocation to succeed for the test to work.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  char *path = xmalloc (path_len);
+  char *path = malloc (path_len);
   DIAG_POP_NEEDS_COMMENT;
+  if (path == NULL)
+    {
+      printf ("malloc (%zu): %m\n", path_len);
+      return EXIT_UNSUPPORTED;
+    }
+
+  TEST_VERIFY_EXIT (symlink (".", lnk) == 0);
 
   /* Construct very long path = "/tmp/bz22786.XXXX/symlink/aaaa....."  */
   char *p = mempcpy (path, lnk, strlen (lnk));

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree
  2018-08-29 13:25     ` Stefan Liebler
@ 2018-08-29 14:05       ` Carlos O'Donell
  2018-08-30  6:49         ` Stefan Liebler
  0 siblings, 1 reply; 7+ messages in thread
From: Carlos O'Donell @ 2018-08-29 14:05 UTC (permalink / raw)
  To: Stefan Liebler, libc-alpha

On 08/29/2018 09:24 AM, Stefan Liebler wrote:

Thanks for posting a git-am'able patch :-) I like being able to review
everything that will go into git.

> commit 5ada1975be8f1b30b8f33d1d25cb5575690066e1
> Author: Stefan Liebler <stli@linux.ibm.com>
> Date:   Wed Aug 29 15:20:51 2018 +0200
> 
>     Test stdlib/test-bz22786 exits now with unsupported if malloc fails.
>     
>     The test tries to allocate more than 2^31 bytes which will always fail on s390
>     as it has maximum 31bit of memory.
>     Before commit 6c3a8a9d868a8deddf0d6dcc785b6d120de90523, this test returned
>     unsupported if malloc fails.  This patch re enables this behaviour.
>     
>     Furthermore support_delete_temp_files() failed to remove the temp directory
>     in this case as it is not empty due to the created symlink.
>     Thus the creation of the symlink is moved behind malloc.
>     
>     ChangeLog
>     
>             * stdlib/test-bz22786.c (do_test): Return EXIT_UNSUPPORTED
>             if malloc fails.
> 

OK for master with the additional comment below.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
> index d1aa69106c..44ec631a96 100644
> --- a/stdlib/test-bz22786.c
> +++ b/stdlib/test-bz22786.c
> @@ -39,16 +39,21 @@ do_test (void)
>    const char *lnk = xasprintf ("%s/symlink", dir);
>    const size_t path_len = (size_t) INT_MAX + strlen (lnk) + 1;
>  
> -  TEST_VERIFY_EXIT (symlink (".", lnk) == 0);
> -

OK.

>    DIAG_PUSH_NEEDS_COMMENT;
>  #if __GNUC_PREREQ (7, 0)
>    /* GCC 7 warns about too-large allocations; here we need such
>       allocation to succeed for the test to work.  */
>    DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
>  #endif
> -  char *path = xmalloc (path_len);
> +  char *path = malloc (path_len);

Needs a comment explaining why we are not using xmalloc.

Suggestion:

/* On 31-bit s390 the malloc might fail, and we want to mark 
   the test unsupported.  Likewise on systems with little
   physical memory the test will fail and should be unsupported.  */

>    DIAG_POP_NEEDS_COMMENT;
> +  if (path == NULL)
> +    {
> +      printf ("malloc (%zu): %m\n", path_len);
> +      return EXIT_UNSUPPORTED;
> +    }
> +
> +  TEST_VERIFY_EXIT (symlink (".", lnk) == 0);

OK.

>  
>    /* Construct very long path = "/tmp/bz22786.XXXX/symlink/aaaa....."  */
>    char *p = mempcpy (path, lnk, strlen (lnk));


-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree
  2018-08-29 14:05       ` Carlos O'Donell
@ 2018-08-30  6:49         ` Stefan Liebler
  0 siblings, 0 replies; 7+ messages in thread
From: Stefan Liebler @ 2018-08-30  6:49 UTC (permalink / raw)
  To: Carlos O'Donell, libc-alpha

Committed with the comment.
Thanks for reviewing.
Stefan

On 08/29/2018 04:05 PM, Carlos O'Donell wrote:
> On 08/29/2018 09:24 AM, Stefan Liebler wrote:
> 
> Thanks for posting a git-am'able patch :-) I like being able to review
> everything that will go into git.
> 
>> commit 5ada1975be8f1b30b8f33d1d25cb5575690066e1
>> Author: Stefan Liebler <stli@linux.ibm.com>
>> Date:   Wed Aug 29 15:20:51 2018 +0200
>>
>>      Test stdlib/test-bz22786 exits now with unsupported if malloc fails.
>>      
>>      The test tries to allocate more than 2^31 bytes which will always fail on s390
>>      as it has maximum 31bit of memory.
>>      Before commit 6c3a8a9d868a8deddf0d6dcc785b6d120de90523, this test returned
>>      unsupported if malloc fails.  This patch re enables this behaviour.
>>      
>>      Furthermore support_delete_temp_files() failed to remove the temp directory
>>      in this case as it is not empty due to the created symlink.
>>      Thus the creation of the symlink is moved behind malloc.
>>      
>>      ChangeLog
>>      
>>              * stdlib/test-bz22786.c (do_test): Return EXIT_UNSUPPORTED
>>              if malloc fails.
>>
> 
> OK for master with the additional comment below.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
>> diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
>> index d1aa69106c..44ec631a96 100644
>> --- a/stdlib/test-bz22786.c
>> +++ b/stdlib/test-bz22786.c
>> @@ -39,16 +39,21 @@ do_test (void)
>>     const char *lnk = xasprintf ("%s/symlink", dir);
>>     const size_t path_len = (size_t) INT_MAX + strlen (lnk) + 1;
>>   
>> -  TEST_VERIFY_EXIT (symlink (".", lnk) == 0);
>> -
> 
> OK.
> 
>>     DIAG_PUSH_NEEDS_COMMENT;
>>   #if __GNUC_PREREQ (7, 0)
>>     /* GCC 7 warns about too-large allocations; here we need such
>>        allocation to succeed for the test to work.  */
>>     DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
>>   #endif
>> -  char *path = xmalloc (path_len);
>> +  char *path = malloc (path_len);
> 
> Needs a comment explaining why we are not using xmalloc.
> 
> Suggestion:
> 
> /* On 31-bit s390 the malloc might fail, and we want to mark
>     the test unsupported.  Likewise on systems with little
>     physical memory the test will fail and should be unsupported.  */
> 
>>     DIAG_POP_NEEDS_COMMENT;
>> +  if (path == NULL)
>> +    {
>> +      printf ("malloc (%zu): %m\n", path_len);
>> +      return EXIT_UNSUPPORTED;
>> +    }
>> +
>> +  TEST_VERIFY_EXIT (symlink (".", lnk) == 0);
> 
> OK.
> 
>>   
>>     /* Construct very long path = "/tmp/bz22786.XXXX/symlink/aaaa....."  */
>>     char *p = mempcpy (path, lnk, strlen (lnk));
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-08-30  6:49 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-23 16:48 [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree Paul Pluzhnikov
2018-07-30 20:13 ` Adhemerval Zanella
2018-08-06 15:12   ` Paul Pluzhnikov
2018-08-07 11:20     ` Adhemerval Zanella
2018-08-29 13:25     ` Stefan Liebler
2018-08-29 14:05       ` Carlos O'Donell
2018-08-30  6:49         ` Stefan Liebler

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).