Memory leak related to cloning pipes #1647
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
presidento
reviewed
Aug 21, 2017
lib/cfg-tree.c
Outdated
| @@ -476,8 +478,8 @@ cfg_tree_compile_single(CfgTree *self, LogExprNode *node, | |||
| goto error; | |||
| } | |||
| pipe->flags |= PIF_INLINED; | |||
| g_ptr_array_add(self->initialized_pipes, pipe); | |||
There was a problem hiding this comment.
Why don't we need to increase the reference counter?
There was a problem hiding this comment.
We talked it IRL, I let the g_ptr_array_add in the original place, and call only log_pipe_ref(pipe) on line 468.
lib/cfg-tree.h
Outdated
| @@ -40,20 +40,20 @@ const gchar *log_expr_node_get_content_name(gint content); | |||
| enum | |||
| { | |||
| /* expr node content type */ | |||
| ENC_SOURCE, | |||
There was a problem hiding this comment.
Just a side note: I would create two different commit: one for the fix (the .c file), and one for the refactor (.h file).
furiel
force-pushed
the
reload-memleak-during-clone
branch
2 times, most recently
from
38c72fe
to
09c1779
Compare
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
1 similar comment
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
furiel
force-pushed
the
reload-memleak-during-clone
branch
from
09c1779
to
f5cb8f1
Compare
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
bazsi
approved these changes
Sep 5, 2017
lib/cfg-tree.h
Outdated
| ENL_SINGLE, /* 7 */ | ||
| ENL_REFERENCE, /* 8 */ | ||
| ENL_SEQUENCE, /* 9 */ | ||
| ENL_JUNCTION, /* 10 */ | ||
| }; |
There was a problem hiding this comment.
Why is this "numbering" useful? as I see it only adds maintenance burden, should we need to add new enums.
furiel
force-pushed
the
reload-memleak-during-clone
branch
from
f5cb8f1
to
18edd8d
Compare
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
Lets consider the following config snippet:
filter f_warn { level(warn, err, crit) };
log {
filter(f_warn);
filter(f_warn);
};
There is a named filter and two filter references here. These filters
are instantiated with in different phases. The named filters are
instantiated during config parse, and added to CfgTree->objects
hashtable which has a general destructor.
The instantiation of the referenced filters happen during config
compile. In the cfg_tree_compile_single, when we hit the first filter
reference, we reuse the pipe that is instantiated during config parse:
the pipe element stored in CfgTree->objects. We utilize a flag to do
this only once. Now when we compile the the second filter reference,
we see that the pipe in CfgTree->objects is already used, so we clone
the pipe instead. Here a completely new unname pipe is created. So
that these could be freed they are added to a pointer array:
CfgTree->initialized_pipes, and during free, we call log_pipe_unref
for all these elements. The conflict comes from that both the pipes above
are added to the initialized_pipes array, but for the first one we
have two destructors: one for the CfgTree->objects hashtable, and one
for the CfgTree->initialized_pipes, but only one for the cloned pipe.
To avoid double free, the original implementation unconditionally
log_pipe_ref-fed all pipes that are added to CfgTree->initialized_pipes.
But this resulted in the cloned pipes had reference count 2 but only
one destructor, leaking memory.
As the config graph is recreated in every start, these leaks accumulate
in consecutive reloads.
This patch considers that initialized_pipes is intended to be used for
destructing all pipe objects. This means log_pipe_ref is missing for
pipes in objects in CfgTree->objects. So this patch branches, and
calls an extra log_pipe_ref for those, but not for the cloned pipes.
Reported in: syslog-ng#1317
Signed-off-by: Antal Nemes <antal.nemes@balabit.com>
The clone function accidentally refs self->replacement template. But it is also ref-fed inside the constructor. So this extra ref is not needed, and also causes memory leak. As the clone is part of configuration load, this leak accumulates during reloads. Signed-off-by: Antal Nemes <antal.nemes@balabit.com>
furiel
force-pushed
the
reload-memleak-during-clone
branch
from
18edd8d
to
3c8d58d
Compare
|
Build SUCCESS, the tests were executed on test branch: master and test suite: functions |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are two kind of memory leaks in syslog-ng related to cloning pipes. One of them is general: all clone invocations are affected. The other one is rewrite rule specific.
Please see the commit messages for detailed explanation of the issues and the patches.
These memory leaks can be reproduced if one defines a named filter/rewrite rule, and refer them twice in the configuration. For example:
These leaks are accumulated during reload.
As a workaround: configuration can be changed to use every reference only once.
Reported in: #1317