Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runmodes: patch identified leak on runmode single #5293

Closed
wants to merge 1 commit into from

Conversation

JoshuaLumb
Copy link
Contributor

@JoshuaLumb JoshuaLumb commented Aug 13, 2020

Make sure these boxes are signed before submitting your Pull Request -- thank you.

==148857==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 176 byte(s) in 1 object(s) allocated from:#0 0x7f2d59623dc6 in calloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10ddc6)#1 0x561af4dcf19b in SCCallocFunc /home/user/suricata/src/util-mem.c:57#2 0x561af4c63df7 in ParseAFPConfig /home/user/suricata/src/runmode-af-packet.c:131#3 0x561af4e13479 in RunModeSetLiveCaptureSingle /home/user/suricata/src/util-runmodes.c:382#4 0x561af4c683f8 in RunModeIdsAFPSingle /home/user/suricata/src/runmode-af-packet.c:860#5 0x561af4c75f56 in RunModeDispatch /home/user/suricata/src/runmodes.c:374#6 0x561af4d6179d in SuricataMain /home/user/suricata/src/suricata.c:2780#7 0x561af489f72c in main /home/user/suricata/src/main.c:22#8 0x7f2d58e700b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: 176 byte(s) leaked in 1 allocation(s).

An explanation:

During startup suricata parses config settings for af-packet threads in the function ParseAFPConfig and stores those settings in a memory-allocated AFPIfaceConfig struct. For efficiency this is only done once - but since the thread initialization function will need the AFPIfaceConfig once for each receiving thread the AFPIfaceConfig tracks the number of threads which will need to refer to it in AFPIfaceConfig->ref.

AFPIfaceConfig->ref is set based on the config file’s specified number of threads, which itself is usually determined here:

if (StringParseInt32(&aconf->threads, 10, 0, (const char *)threadsstr) < 0)

Which eventually ends up setting aconf->ref here:

SC_ATOMIC_RESET(aconf->ref);
(void) SC_ATOMIC_ADD(aconf->ref, aconf->threads);

If all is going well AFPIfaceConfig aconf is freed by the thread initialization function ReceiveAFPThreadInit's sub-function AFPDerefConfig using the following mechanism:

if (SC_ATOMIC_SUB(pfp->ref, 1) == 1) {
SCFree(pfp);

This means that as each thread finishes initialization it will decrement the counter until the last thread finally decrements it to 0, at which time AFPIfaceConfig aconf is freed.

The problem comes into play with --runmode single which overrides the number of threads to be created. There is no consideration for this currently in the assignment of aconf->ref so you can end up with only one thread calling AFPDerefConfig with aconf->ref > 1, meaning it is never freed.

@JoshuaLumb JoshuaLumb requested a review from a team as a code owner August 13, 2020 14:21
@JoshuaLumb JoshuaLumb closed this Aug 13, 2020
@JoshuaLumb JoshuaLumb deleted the runmode-single-asan-patch branch August 13, 2020 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant