Re: [PATCH] yes: Remove memory leak on buf memory

[Kaz Kylheku]

On 2022-11-17 14:21, Alireza Arzehgar wrote:
> From a4edebe5bafb8bf789cf5e9c807375c9bcdfab4e Mon Sep 17 00:00:00 2001
> From: alireza <alirezaarzehgar82@gmail.com>
> Date: Fri, 18 Nov 2022 01:33:10 +0330
> Subject: [PATCH] yes: Remove memory leak on buf memory

I don't agree with the patch. There is no real leak here; there is one
call to malloc in main() which is not freed when the function returns.

It's an "academic leak": the failure to tidy up all singleton objects
on program exit.

Doing so is actually a waste of machine cycles in production runs of the
program.

A useful practice is to have an option for freeing everything.
You use the option when you are debugging for memory leaks.
It could be a run-time option that is available in production
builds of the program, or else a compile-time option which
makes that available in debug builds.

> Using `xmalloc` on an infinite loop due memory leak report on valgrind
> after termination.

The xmalloc call is not enclosed in any loop, though.

> Replacing memory allocation from heap to using stack
> improves simplicity and correctness of the program.

These are debatable points. It isn't unconditionally incorrect
for a program to allocate something with malloc, and then
terminate without deallocating it. In fact, there may be
a requirement in place specifically not to do that.

> Signed-off-by: alireza <alirezaarzehgar82@gmail.com>
> ---
>  src/yes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/yes.c b/src/yes.c
> index 13b990e..28d129a 100644
> --- a/src/yes.c
> +++ b/src/yes.c
> @@ -99,7 +99,7 @@ main (int argc, char **argv)
> 
>    /* Fill the buffer with one copy of the output.  If possible, reuse
>       the operands strings; this wins when the buffer would be large.  */
> -  char *buf = reuse_operand_strings ? *operands : xmalloc (bufalloc);
> +  char mem[bufalloc], *buf = reuse_operand_strings ? *operands : mem;

Firstly, the xmalloc is conditional; in the happy case when
reuse_operand_strings is true (because the arguments happen to
be consecutively allocated in memory) the malloc doesn't happen.

The newly introduced VLA is unconditionally allocated.

The VLA causes the program to use stack space proportional to the
number of bytes in the argument space. Though nowadays the default
stack size is typically quite large, and larger than the limit on
the size of argument material, there could be situations in
which that will crash, like "ulimit -s" being used to set a reduced
stack size.

You generally want to avoid stack allocations proportional to the
sizes of external inputs, without some extraordinary justification.

A good fix for the program would be:

  #if CONFIG_LEAK_DEBUG
    if (! reuse_operand_strings)
      free (buf);
  #endif
  main_exit (EXIT_FAILURE);

(I'm not saying that CoreUtils has a CONFIG_LEAK_DEBUG flag;
if it has no such convention, it could be proposed.)