fix: check correct variable after strdup in _bf_parse_tcp_flags

[Nepomuk5665]

Summary

Fix a null check bug in _bf_parse_tcp_flags() where the wrong variable is checked after strdup().

The Bug

In src/libbpfilter/matcher.c at line 617, after calling strdup(raw_payload) and storing the result in _raw_payload, the code incorrectly checks raw_payload (the input parameter) instead of _raw_payload (the strdup result):

_raw_payload = strdup(raw_payload);
if (!raw_payload)   // BUG: checks input instead of strdup result
    goto err;

The Fix

Check _raw_payload (the result of strdup) instead:

_raw_payload = strdup(raw_payload);
if (!_raw_payload)  // FIXED: check the strdup result
    goto err;

Impact

If strdup() fails due to memory allocation failure, it returns NULL. Without this fix:

• The null check passes (because raw_payload input is non-null)
• _raw_payload is used on line 621: tmp = _raw_payload;
• This leads to null pointer dereference in the subsequent strtok_r() call

This fix ensures proper error handling when memory allocation fails.

[qdeslandes]

LGTM, except for the commit title: fix: check correct variable after strdup in _bf_parse_tcp_flags -> lib: matcher: fix strdup return value check in _bf_parse_tcp_flags as per https://bpfilter.io/developers/style.html.

Did you use any tool to detect this issue? If so, which one? This is definitely something we should not have missed, if there's any tool that can validate this for us, that would be great.

[Nepomuk5665]

Thanks for the review! I've updated the commit title to follow the style guide.

Regarding the detection: I found this through manual code review while looking for common C bug patterns (unchecked return values, null pointer issues, etc.).

For automated detection, tools like:

• Clang Static Analyzer with -analyzer-checker=core.NullDereference
• Coverity
• cppcheck with --enable=nullPointer
• PVS-Studio

could potentially catch this pattern. The specific issue is that the strdup result is assigned to _raw_payload but the null check is against raw_payload (the input parameter). Static analyzers that track value flow should flag this.

[qdeslandes]

Regarding the detection: I found this through manual code review while looking for common C bug patterns (unchecked return values, null pointer issues, etc.).

For automated detection, tools like:

• Clang Static Analyzer with -analyzer-checker=core.NullDereference
• Coverity
• cppcheck with --enable=nullPointer
• PVS-Studio

Thanks for the explanation. We use clang-tidy, so I expected it to raise this issue, but clang-analyzer-unix.Malloc is disabled due to too many false positives... Enabling it back raise the issues, but also multiple false positives. I'll create a new issue to investigate further.

In the meantime, LGTM. Congrats on your first PR, and thanks for the fix! I'll merge this PR once the CI is green.

[qdeslandes]

LGTM, except for the commit title: fix: check correct variable after strdup in _bf_parse_tcp_flags -> lib: matcher: fix strdup return value check in _bf_parse_tcp_flags as per https://bpfilter.io/developers/style.html.

[Nepomuk5665]

Thank you for the kind words and for merging this! 🎉

I saw you created issue #394 for investigating the static analysis detection. I'd be happy to look into that and see if I can help find a configuration that catches these issues without too many false positives. I'll take a look at the clang-tidy options and report back on that issue.

[qdeslandes]

Thank you for the kind words and for merging this! 🎉

I saw you created issue #394 for investigating the static analysis detection. I'd be happy to look into that and see if I can help find a configuration that catches these issues without too many false positives. I'll take a look at the clang-tidy options and report back on that issue.

Thanks, I'll take a look today :)