Fix BZ 23400 -- stdlib/test-bz22786.c creates temporary files in glibc source tree

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

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

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