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

[Kamil Dudka]

On Friday, November 18, 2022 9:45:23 AM CET Kaz Kylheku wrote:
> 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.)

I think coreutils already uses the `lint` define for this at multiple places.
Fedora builds coreutils with this flag enabled:

    https://src.fedoraproject.org/rpms/coreutils/c/1f6e0df2

Kamil