ping//Re: [PATCH v2] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]

[Adhemerval Zanella]

On 24/08/2020 05:31, Xiaoming Ni wrote:
> On 2020/8/23 2:09, Paul Eggert wrote:
>> On 8/21/20 8:27 PM, Xiaoming Ni wrote:
>>> How do I determine whether data.maxdir is too large for alloca?
>>
>> __libc_use_alloca. Also see include/scratch_buffer.h, which is designed for this sort of situation.
>>
>> .
> 
> is that ?
> 
> --- a/io/ftw.c
> +++ b/io/ftw.c
> @@ -645,6 +645,13 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
>      }
> 
>    data.maxdir = descriptors < 1 ? 1 : descriptors;
> +  if ((__glibc_unlikely (data.maxdir > SIZE_MAX / sizeof (struct dir_data *)))
> +      || (! __libc_use_alloca (data.maxdir * sizeof (struct dir_data *))))
> +    {
> +      __set_errno (EINVAL);
> +      return -1;
> +    }
> +
>    data.actdir = 0;
>    data.dirstreams = (struct dir_data **) alloca (data.maxdir
>                                                  * sizeof (struct dir_data *));
> 

I would prefer to just remove alloca altogether and either move to dynamic 
allocation or use hybrid approach and use a scratch_buffer or a dynarray.  
In this specific case, it already does a unconditional dynamic memory 
allocation for the 'data.dirbuf', so why not just allocate the 'dirstream' 
buffer on the same block as:

  data.maxdir = descriptors < 1 ? 1 : descriptors;
  data.actdir = 0;
  data.dirbufsize = MAX (2 * strlen (dir), PATH_MAX);

  data.dirstreams = malloc (data.maxdir * sizeof (struct dir_data *)
			    + data.dirbufsize);
  if (data.dirstreams == NULL)
    return -1;

  memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *));

  data.dirbuf = (char *) data.dirstreams
		+ data.maxdir * sizeof (struct dir_data *);

It would require adjust the size of 'process_entry' for the realloc (to take
in consideration the 'dirstreams' size).

> 
> Whether to use malloc or return an error message when the input is too large?
> 
> I still don't understand why libc uses alloca so much.
> Is it for performance?

Most of the code that uses alloca is pre-C99 that provides VLA or are code where
there is no prior way to know how much stack would be allocated (that's why the
__libc_use_alloca which tries to put a high bound in stack usage).  However, 
dynamic stack allocation either with alloca or VLA tend to produce bad code gen
(as the Linux kernel developers has observed) and increases the possibility of
multiple issues (unbounded stack allocation, performance issues, etc.). There
were multiple issues with alloca usage over the years, so recently we were 
moving the code to either scratch_buffer or dynarray.