[syslog-ng] Duplicate configuration items causing memory leak

[anthony lineham]

Hi Bazsi,

Thanks for the follow up. It makes sense to be trapping the config error as soon as possible, and as you said, users should be encouraged to have sensible configurations.

Regards
Anthony

>>> On 11/1/2011 at 04:37 AM, in message <1320075450.22972.127.camel at bzorp>, Balazs
Scheidler <bazsi at balabit.hu> wrote: 
> Hi Anthony,
> 
> Sorry for not answering any sooner, this email has gone the same fate as
> Jakub's, therefore I didn't read it when it was posted.
> 
> Thanks for noticing this problem, tracking it down and posting a patch.
> I decided to use a different implementation, which uses the
> configuration parser framework to report problems and committed this
> patch:
> 
> commit b061328b3e0d3b466ac29ef4e1b3ee384b7130b0
> Author: Balazs Scheidler <bazsi at balabit.hu>
> Date:   Mon Oct 31 16:33:32 2011 +0100
> 
>     configuration: report duplicate configuration elements
>     
>     Previously if a configuration element name was used twice, the 2nd would
>     be used and some memory was leaked on every reload.
>     
>     This patch changes that by reporting duplicate IDs as configuration 
> errors.
>     
>     A patch was posted by Anthony, but a different implementation
>     was used instead.
>     
>     Reported-By: anthony lineham <anthony.lineham at alliedtelesis.co.nz>
>     Signed-off-by: Balazs Scheidler <bazsi at balabit.hu>
> 
> It reports duplicate config elements like this:
> 
> /------------
> |Error parsing config, duplicate destination in etc/syslog-ng.conf at line 
> 29, column 13:
> |
> |destination d_file { 
> |            ^^^^^^
> |
> |syslog-ng documentation: 
> http://www.balabit.com/support/documentation/?product=syslog-ng
> |mailing list: https://lists.balabit.hu/mailman/listinfo/syslog-ng
> \- -----------
> 
> This also means, that such problems will prevent syslog-ng to start up, but 
> I'd 
> say anyone using duplicate config elements should take notice. :)
> 
> The leak should also be fixed by my patch too.
> 
> Thanks again.
> 
> On Thu, 2011-10-13 at 09:47 +1300, anthony lineham wrote:
>> Hi Everyone,
>> 
>> I've recently found a memory leak in version 3.0.4 that occurs if the 
> configuration file contains any duplicate entries for the following items: 
> sources, destinations, filters, rewrites, parsers. The leak occurs when the 
> configuration is loaded or reloaded due to a SIGHUP.
>> The config items are stored in a hash table (hash is based on the name of 
> the filter). It seems that in the hashing utility if a request is made to add 
> an item with the same key as a previous item the old item is replaced by the 
> new item (libglib2-2.24.1/glib/ghash.c, g_hash_table_insert()). This is where 
> the memory gets lost. Syslog-ng is not notified that the item has been 
> discarded. 
>> 
>> I considered several possibilities to fix this issue. There are other modes 
> for the hash utility where destroy functions can be setup to deal with the 
> discarded items, but these are orientated around discarding the old item. I 
> thought this was risky as the item may already be referenced by other items. 
> I'm not sure if this case??? In the end I decided to check for the presence 
> of a duplicate in the table before adding the new item. If an existing item 
> is found I discard the new item.
>> 
>> While I realise 3.0.4 is quite old now and perhaps not supported, it looks 
> like the same basic mechanism is still used in OSE 3.3.1, so I'm submitting 
> my code changes for consideration. I haven't tried reproducing the issue on 
> 3.3.1.
>> 
>> Any comments?