Checks to make sure that stdlib's (std::)free is being appropriately
used. Presently checks for the following misuses:
- free(&stack_object)
- free(stack_array)
Differential D89988
adds basic -Wfree-nonheap-object functionality cjdb on Oct 22 2020, 3:01 PM. Authored by
Details Checks to make sure that stdlib's (std::)free is being appropriately
Diff Detail Event TimelineComment Actions thanks for this! i like the idea of this warning; seems like a cheap way to catch ugly bugs early. +rtrieu and aaron.ballman, as this is a new diagnostic most of my comments are just poking at style.
along the same lines, have you considered also adding a check for: void foo(); void baz() { free(foo); } ...which... sadly... i've seen in actual C code. :(
Comment Actions LGTM, though please wait for a review from someone with more expertise with clang's warnings (maybe Aaron or Richard?) to land. Thanks again! Comment Actions In order to avoid false negatives, I think this needs to be flow-sensitive to track values. e.g., I would love for this to catch code like: void func(bool b) { int i = 12; int *ip = b ? &i : (int *)malloc(sizeof(int)); ... free(ip); // Should we warn or not? } While this example is incredibly contrived, you'll fine real-world code does this sort of path sensitive stuff relatively often (a common example is a function that sometimes returns a value that's been strdup()'ed except for one bad function that returns a string literal).
Comment Actions Yep, this is what I'm considering "phase 2" or "phase 3". Here's another one of note: char* p = static_cast<char*>(std::malloc(8)); char& r = *p; std::free(&p); // should warn std::free(&r); // shouldn't warn A key intention of D89988 is to get minimal coverage and then expand from there. Comment Actions Awesome! However, the part that concerns me relating to D89988 is that doing control-flow sensitive checks is something that's generally done in the Analysis part of the frontend rather than Sema, because generating a CFG and then traversing it to reason about the code can be expensive. So the usual approach is to have a warning flag (like what you've done) that enables the checking, akin to: https://github.com/llvm/llvm-project/blob/52bcd691cb1992187d022161e95977a9db371f51/clang/lib/Sema/AnalysisBasedWarnings.cpp#L2208 which suggests that some of the work here should perhaps be restructured in that way to cut down on the churn when you get to later phases. It also suggests that perhaps adding this check to -Wall is not desired due to expected compilation performance concerns (which aren't here for the initial version of the patch). Comment Actions If we want _full_ analysis, clang-analyzer-unix.Malloc is what originally flagged the free(function_pointer) case that I referenced above; its complaint was:
I think that whatever we do with this warning is going to be less powerful than what the analyzer does, so the goal is surfacing an appropriate subset of these complaints in clang, potentially with less room for false-positives, etc etc. Doing fun tricks with the CFG sounds like a reasonable next step here, and doing so will likely be way more powerful (+ CPU-consuming, and invite potentially more false-positives). I agree that that probably shouldn't be under -Wall, but I wonder if these trivial cases aren't worth putting there on their own? It's not a perfect parallel, but our split between -Wuninitialized + -Wsometimes-uninitialized comes to mind. Comment Actions Yep, I can live with that. -Wfree-nonheap-object to do the bare minimum (so it can be turned on by -Wall) and perhaps -Wdeep-free-nonheap-object to do the CFG analysis? Comment Actions I think that approach would work well -- then we get the safety of the trivial-to-check cases without the performance impacts unless the user opts in to those. That would also address my concern about churn for the future changes.
Comment Actions LGTM aside from a few request for comments in the test cases.
|
why not include this in -Wall? IMO freeing something that's obviously not from the heap is broken enough to be in -Wall