Re: OT: Possible memory leak in an exercise of a C handbook

[Franco Martelli]

On 17/12/24 at 22:09, Jeffrey Walton wrote:
On Tue, Dec 17, 2024 at 2:39 PM Franco Martelli <martelli...@gmail.com> wrote:

On 16/12/24 at 20:49, Jeffrey Walton wrote:
Here's the problem:

void dealloc()
{
      for ( const DIGIT *p = first; p->next != NULL; p = p->next )
          if ( p->prev != NULL )
              free( p->prev );
      free( last );
}

You seem to be checking backwards (p->prev) but walking the list
forwards (p = p->next) at the same time.

Try something like this. It allows you to walk the list forward.

      void dealloc()
      {
          for ( const DIGIT *next, *p = head; p->next != NULL; )
              if ( p != NULL )
                  next = p->next, free( p ), p = next;
          free( last );
      }

The use of 'next' stashes away the pointer so you can free 'p' and
still access the next pointer.

Thanks, your code works, Valgrind says 0 errors and: "All heap blocks
were freed -- no leaks are possible".
GCC gives me a warning, so I've to remove the "const" modifier in the
"for loop":

$ gcc -Wall e09-01.c
e09-01.c: In function ‘dealloc’:
e09-01.c:57:47: warning: passing argument 1 of ‘free’ discards ‘const’
qualifier from pointer target type [-Wdiscarded-qualifiers]
     57 |                         next = p->next, free( p ), p = next;
        |                                               ^
In file included from e09-01.c:5:
/usr/include/stdlib.h:568:25: note: expected ‘void *’ but argument is of
type ‘const DIGIT *’ {aka ‘const struct digit *’}
    568 | extern void free (void *__ptr) __THROW;

after done this, all works nicely. Thanks again.

There may be one logic error in the code -- if you insert one item,
then you may double free the node because you free 'p' and then you
free 'last'.

I would rewrite the cleanup code like so:

     void dealloc()
     {
         DIGIT *next, *p = head;
         while( p )
             next = p->next, free( p ), p = next;
     }

Then add three test cases instead of one. Have a test case for 0
items, 1 item, and N items (like 8).

I like the "for loop" statement because let me to define variables that 
are in use only inside the loop, so I can avoid variables that are 
visible in the full function block but are in use only in a (while(…)) loop:

void dealloc()
{
        for( DIGIT *next, *p = head; p ; )
                next = p->next, free( p ), p = next;
}

The use of comma operator for me doesn't hurt, I always read the code 
from left to right.
Thanks for this new optimization.


--
Franco Martelli