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