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