memleak hook: find leaks in plugins #4737
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
rustyrussell
force-pushed
the
guilt/memleak-hook
branch
from
b939602
to
57ac8f7
Compare
|
Trivial fix for non-DEVELOPER unit test builds, and rebase to fix trivial conflict on master. |
rustyrussell
force-pushed
the
guilt/memleak-hook
branch
from
57ac8f7
to
15b6f66
Compare
|
Just a minor comment about the |
cdecker
reviewed
Aug 30, 2021
rustyrussell
force-pushed
the
guilt/memleak-hook
branch
from
25fed32
to
5ee26e7
Compare
rustyrussell
force-pushed
the
guilt/memleak-hook
branch
from
5ee26e7
to
c8f4d31
Compare
cdecker
approved these changes
Sep 4, 2021
| @@ -1182,8 +1182,7 @@ static void PRINTF_FMT(1,2) log_memleak(const char *fmt, ...) | |||
| va_list ap; | |||
|
|
|||
| va_start(ap, fmt); | |||
| /* FIXME: This is LOG_DEBUG until we fix leaks! */ | |||
| plugin_logv(memleak_plugin, LOG_DEBUG, fmt, ap); | |||
| plugin_logv(memleak_plugin, LOG_BROKEN, fmt, ap); | |||
There was a problem hiding this comment.
I see, this is why the shutdown isn't a hook.
cdecker
force-pushed
the
guilt/memleak-hook
branch
from
c8f4d31
to
b30560b
Compare
|
Reasing to address conflict in test_gossip.py |
|
Had a minor fixup to apply: |
|
Unused variable in |
cdecker
force-pushed
the
guilt/memleak-hook
branch
from
46016b3
to
9dcc1a2
Compare
This is useful for plugins which can't send junk to stdout. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will let plugins use it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Otherwise the NULL parents look like a memleak. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…pics. Useful for plugins which dynamically generate them. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/memleak-hook
branch
from
9dcc1a2
to
bc6474a
Compare
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> mar
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And free the err string when policy turns out not to be a u64. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
They get grafted into clone, so have them parented there. Otherwise we get a small leak every time we RBF. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Otherwise it looks like a leak. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This lets memleak track them, but makes sure they don't leak; using notleak could cover up a leak here. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And fix utx leak in the withdraw case. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Another tiny leak, which happened in keysend which uses these routines. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…mmer. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Fixed: Plugins: C plugins would could leak memory on every command (esp. seen when hammering topology's listchannels).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we've fixed them, this makes sure CI notices if new leaks appear. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/memleak-hook
branch
2 times, most recently
from
f830bca
to
f4f0527
Compare
Otherwise we get very upset when the plugins go away. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/memleak-hook
branch
from
f4f0527
to
9315cee
Compare
|
ACK 9315cee |
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.
And indeed, it found some. None as major as the one in #4721 but that seems to be when the plugin doesn't read the entire JSON stream at once, and leaks the command struct, which doesn't happen in our tests (at least here).
This is based on #4754 thanks to @cdecker suggesting a generic shutdown notification!