Skip to content

Fix #14990: FP memleak with function call through function pointer - #8813

Open
aadanen wants to merge 11 commits into
cppcheck-opensource:mainfrom
aadanen:fptr_leak
Open

aadanen wants to merge 11 commits into
cppcheck-opensource:mainfrom
aadanen:fptr_leak

Conversation

@aadanen

@aadanen aadanen commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Modify checkleakautovar to consider calls to lambdas and function pointers as function calls that can allow resources to escape the current scope.

@aadanen

aadanen commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor Author

I am suprised to see all these segmentation faults. In my testing everything seemed to work nicely but I guess it was just crashing silently and working properly. I will need to modify functionCall() more heavily to not send nullptr to the library functions. This kinda undermines my idea of reusing functionCall for anonymous functions but I still think it is better than duplicating the function.

@aadanen

aadanen commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor Author

I switched from the makefile to cmake and ran the CI workflows locally so I think next time they run, they should pass. Sorry about that. Also I noticed that git was using my work account, so I sorted that out too.

@aadanen
aadanen force-pushed the fptr_leak branch 2 times, most recently from 4bc4b04 to 2621d76 Compare August 28, 2026 00:09

@danmar danmar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why but there was significant slowdown in CI (selfchecking) after this?

Comment thread lib/checkleakautovar.cpp Outdated
@aadanen

aadanen commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I will look into it more on monday.

@aadanen

aadanen commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

I found the issue with the selfcheck slowdown. isAnonymousFunctionCall would sometimes link() backwards causing the checker to loop forever.

void f(char ch16) {
    ch = static_cast<unsigned char>(((ch16 >= 0x80) ? 0xff : ch16));
}

@aadanen

aadanen commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

the sanitizers/build check failed because of some network thing:
Connecting to apt.llvm.org (apt.llvm.org)|2a04:4e42:8a::561|:443... failed: Network is unreachable.
I think it probably just needs to be re-run.

Thanks!

Comment thread lib/checkleakautovar.cpp
@danmar

danmar commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

@aadanen I can't retrigger it now. maybe it's too old. can you please make a new commit to retrigger the CI

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

New problem: a lost deallocuse warning. On the merge-base, passing a freed pointer to a call through a function pointer (free(p); (*cb)(p);) or to the result of a call (free(p); get()(p);) gives deallocuse. With this PR the warning is gone. The new branches set tok = lpar->link() (and return anonOpeningPar->link()), so the arguments never reach the dealloc‑use check in checkTokenInsideExpression. Calls through plain names (foo(p), cb(p)) do not warn on the merge-base either. So this warning was specific to these call forms, but it was a real use-after-free detection and the PR drops it silently.

The FP is only partly fixed. It is fixed for (*cb)(p), (s->fp)(p), (*s.fp)(p), (*fps[0])(p), (c ? a : b)(p) and get()(p). It remains for function-pointer calls through a subscript, such as fps[0](p) and s->fps[1](p) (dispatch tables), and for chained calls like get2()()(p).

I found no crash, hang or slowdown. I also found no other result changes on the cfg test files.

Testing

  • I built cppcheck and testrunner in pr/, and cppcheck in base/.
  • ./testrunner TestLeakAutoVar TestLeakAutoVarStrcpy TestLeakAutoVarRecursiveCountLimit TestLeakAutoVarWindows TestLeakAutoVarPosix ran 161 tests with 0 failures.
  • I ran base and PR on test/cfg/{posix,std,gnu}.c, test/cfg/{std,windows}.cpp and lib/checkleakautovar.cpp. The output was identical, and the run time was the same (215s vs 214s).
  • I tried about 40 small cases: function pointers, lambdas (including immediately invoked ones), functors, casts, sizeof, conditions, assignments, and nested or chained calls. The intended cases improve. There were no new false positives besides the one already raised in comments.md.

Regression: deallocuse is lost (c1.c)

#include <stdlib.h>
typedef void (*fn_t)(void*);
fn_t get(void);
void u1(void (*cb)(void*)) {
    char *p = malloc(10);
    free(p);
    (*cb)(p);
}
void u3(void) {
    char *p = malloc(10);
    free(p);
    get()(p);
}

Command: cppcheck -q --template='{line}:{column}:{id}' c1.c

  • base: 7:11:deallocuse and 12:11:deallocuse
  • PR: no output

FPs that remain (d1.c)

#include <stdlib.h>
typedef void (*fn_t)(void*);
typedef fn_t (*fn2_t)(void);
fn2_t get2(void);
struct S { fn_t fps[2]; };
void k1(fn_t *fps) { char *p = malloc(10); fps[0](p); }
void k4(struct S *s) { char *p = malloc(10); s->fps[1](p); }
void k5(void) { char *p = malloc(10); get2()()(p); }

The PR still reports memleak for all three functions, the same as base. Base and PR agree on these cases; the PR only fixes the parenthesised forms.


Automated review of cd4f032 by Claude. The pull request was built and tested locally.

Comment thread lib/checkleakautovar.cpp
Comment thread lib/checkleakautovar.cpp
@dmcppcheck

Copy link
Copy Markdown

Test results for commit cd4f032 (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8813/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: +1.8% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8813/2026-10-25_21-08-26-cd4f0320eb6d_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

@dmcppcheck

Copy link
Copy Markdown

Test results for commit a0fa6fb (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8813/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: +0.0% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8813/2026-10-26_14-48-08-a0fa6fb5bc33_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

I found no new problems in the current head (a0fa6fb). The earlier reviews already raised the problems I know of: the lost deallocuse warning, the deallocuse false positive for (*cb)(fclose(f)), and the remaining false positives for chained calls. The latest commit also handles subscript calls such as fps[0](p), v[0](p), m["k"](p) and this->fps[0](p). Things that look like these calls but are not did not lose any real leak warnings: casts to unknown types, a[i] < 0, if (a[i]), (p)[0], new S[n]() and delete [] (p).

Testing

  • Build: cppcheck and testrunner built in pr/, and cppcheck built in base/, without errors.
  • Unit tests: ./testrunner TestLeakAutoVar ran 149 tests with 0 failures (10 todos).
  • Comparison with base: I wrote small files in scratch/ and ran both versions with cppcheck -q --template='{line}:{column}:{id}:{message}'.
    • Subscript calls (t3.cpp): the PR removes the memleak false positive for v[0](p) with a std::vector<fn>, for m["k"](p) with a std::map, and for this->fps[0](p).
    • Leaks that must still be found (t3.cpp): the PR still reports memleak for a[0](NULL), (*g)(1)(p), int x = a[0] < 0 ? 1 : 2;, if (a[i]) {} and a[i] = (p)[0];. It still reports doubleFree for a[0](p); free(p); free(p);.
    • Casts and parentheses (t2.c): casts to unknown types such as (mylong_t)(p[0]), (MYVOID)(p) and (u8)(*p) + 1, and if (a) (p)[0] = 1;, give the same leaks on base and PR.
    • new, delete and functors (t1.cpp): new char[n]() and delete [] (p); p[0]=1; give the same results on base and PR. A<int>()(p) (a templated functor) no longer gives a memleak, as intended. The deallocuse warning for free(p); (g)(p); with a lambda is lost, which was already reported.
    • Conditions and returns (t4.cpp): if ((*cb)(p) < 0) return 1; no longer gives the false positive. return (*cb)(NULL); and int r = (*cb)(NULL); still report the leak.
  • cfg files: I ran base and PR on test/cfg/{qt,wxwidgets,boost,windows}.cpp and test/cfg/gtk.c, with the matching --library and --check-library --enable=information. The output was identical, with no crashes or hangs.

Automated review of a0fa6fb by Claude. The pull request was built and tested locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants