RE: PATCH: Memory leak in Clickatell + Brunet HTTP SMSC

Hi Stipe

I must disagree with you (unfortunately).
If you look closely, you'll see that:
word = gwlist_extract_first(words);
and
value = gwlist_extract_first(words);
is used on the list, (i.e. 2 elements removed from the list), and when gwlist_destroy is called, 'word' and 'value' is not destroyed.
There is an octstr_destroy(word), but there is no octstr_destroy(value), and because there is no callback for the 'param' dictionary, 'value' never will be destroyed, resulting in a memory leak.

The patch you provided creates a copy of 'value' which will be destroyed via the callback when dict_destroy is called, but the original 'value' will still reside in memory indefinitely.  So I would go for the patch provided by Mr. Jackson, or you should add a octstr_destroy(value) to your patch.

Regards



--

    Werner Coetzee
*   C Developer: Messaging Engine

*   T +27 21 910 7700
*   F +27 21 910 7701

    [email protected]
    www.clickatell.com




-----Original Message-----
From: Stipe Tolj [mailto:[email protected]]
Sent: 14 July 2008 17:13
Cc: [email protected]
Subject: Re: PATCH: Memory leak in Clickatell + Brunet HTTP SMSC

Donald Jackson schrieb:
> Hi everyone,
>
> Attached is a diff for memory leak found in smsc_http.c for Clickatell
> and Brunet specific implementations. The message ID Octstr's never get
> destroyed when the dictionary gets destroyed.

-1 on this patch. Reason: we do a double free() on the Octstr pointers and cause
a segfault.

The issue is:

   - octstr_split_words() malloc()s the new items in the list.
   - then we inject to Dict the pointer refrence, that's the "original", not a copy.
   - then we gwlist_destroy() and hence all items of the list are freed, also
the one we put into the dict.
   - later on we dict_destroy() _WITH_ the octstr_destroy() callback and hence
we'll try to double free. Bang.

Attached is a revised patch, that takes care we duplicate the items that we put
into the Dict hash, so we have a clean destruction later, as the
gwlist_destroy() took care of the original ones.

Please review and vote.

Stipe

-------------------------------------------------------------------
Kölner Landstrasse 419
40589 Düsseldorf, NRW, Germany

tolj.org system architecture      Kannel Software Foundation (KSF)
http://www.tolj.org/              http://www.kannel.org/

mailto:st_{at}_tolj.org           mailto:stolj_{at}_kannel.org
-------------------------------------------------------------------