Re: [patch] Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree

[Carlos O'Donell <carlos at redhat dot com>]

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.