Thank you guys for investigating it!

In D69726#2671526, @vsavchenko wrote:

@Charusso
It looks like this patch introduced a some weird false positive on PostgreSQL

report-guc.c-ParseLongOption-13-1.html1 MBDownload

I'll try to look at it myself and minimize it, but maybe you can get an idea from a full report.

Debug facility NFC: https://reviews.llvm.org/rG89d210fe1a7a1c6cbf926df0595b6f107bc491d5
size -> extent conversion: https://reviews.llvm.org/rG9b3df78b4c2ab7a7063e532165492e1ffa38d401

In D97699#2601804, @NoQ wrote:

Why are you even tracking reg_$0<envp>? It's obvious from the structure of the symbol that it's an environment pointer, there's no need to keep it in maps.

Great idea!

@NoQ, could you upstream it and move this idea forward please?

Hey, I am back.

• Fix everything.

Hey! We are somewhat slow in reviews, please understand that.

I do not get why my solution -analyzer-config silence-checkers=core.NullDereference -analyzer-config silence-checkers=core.DivideZero was insufficient, whereas -analyzer-config silence-checkers=core.NullDereference;core.DivideZero is working. One funny thing to note here: LLDB cannot parse "a;b", so that it stops to parse the compiler flags at ;... It is mostly LLDB's fault, but that is why I wanted to avoid the character ;. If I remember right I have picked ; over ,, because the Clang's flag-parser cannot parse ,, but may , is working now. Could you try that and overwrite these silly behaviors, please?

Thanks for the reviews!

• Resolve most of the review comments.
• We really need to specify the design of future checkers.

Okay! Thanks for the review.

• Using getNameAsString().

In D81062#2070823, @NoQ wrote:

This change in the state adds no new information to the state while also burdening the Store with more data to carry and making debugging harder (not only dumps will now have a bunch of explicit bindings, but also these bindings will be much bulkier being derived symbols).

I do not see any debug burden because there were no information to begin the debug. The *new* information occurs at the appropriate place now.

Story: I was looking at the va_list test files and I was very curious why I cannot catch its binding as a struct. So I have checked out an implementation:

typedef struct {
   unsigned int gp_offset;
   unsigned int fp_offset;
   void *overflow_arg_area;
   void *reg_save_area;
} va_list[1];

The answer was: bindArray(). I believe it is convenient to see immediately what we model.

May this is somewhat hand-waving, but this double-binding does not seem useful.

• Refactor.

• Refactor.

In D71155#1854908, @NoQ wrote:

Let's separate CStringChecker improvements into a separate patch and have a separate set of tests for it.

• Refactor.
• State out explicitly whether the Analyzer models the dynamic size.

Way more sophisticated matching: https://reviews.llvm.org/D77745

• Refactor.

I believe it is very strange on a Windows system to have multiple dots in a file. The other issue could be the wildcard /*/ in a path full of \s. The LLVM lit (https://llvm.org/docs/CommandGuide/lit.html) has tons of Windows-related shortcuts, which I have never seen being used, but could be useful.

In D76768#1959285, @ASDenysPetrov wrote:

@NoQ please, review my new patch. I've made some changes from C++ side. It regards replacing backslash to forward-slash in case of Win platform defined.

• Simplify tests.
• Remove dead code, they are far away to being used.
• Add an extra test case.

• Remove the last dead comment.

Thanks for the review, hopefully if I ping @NoQ in every round, it will be green-marked soon.

In D77066#1953280, @Charusso wrote:

getDynamicSizeWithOffset(State, MR, SVB) {
  Offset = State->getStoreManager().getStaticOffset(MR, SVB);
  ...
}

Hm, the MemRegion's offset should be great. I was thinking about if we would store SVal offsets in the Store.

Given that the secondary behavior confuse people I have removed it for now. May if someone introduce a NullTerminationChecker then we introduce such option to warn on insecure calls instant. Thanks @balazske for influencing that change. @NoQ this project had a deadline like half year ago, could you smash that green button please?

• Get rid of the secondary behavior for now.
• Fix review comments.

• Remove the last gymnastic.
• Rebase.

Please avoid to stuff in CheckerContext because this facility should be used by ExprEngine/Store as well.
Let us reword your API: getDynamicSizeWithOffset(ProgramStateRef, SVal, SValBuilder &). Of course we are trying to obtain some buffer-ish size, that is the purpose of the entire API.
I also could imagine something like getDynamicSizeMul(ProgramStateRef, const MemRegion &, const MemRegion &, SValBuilder &), as it is very common.

In D76768#1952674, @ASDenysPetrov wrote:

@Charusso Could you point me where I can find correspondent code which creates dot (graph) files?

Each class has its own dumpJson method which builds up the graph together. The actual dot creation happens in ExprEngine.cpp's struct DOTGraphTraits<ExplodedGraph*>, changed in: D62346

In D73521#1951776, @ASDenysPetrov wrote:

Will this utility affect Visual Studio builds?

• Remove the test of creating a live checker, instead copy over the live checker when the script runs.
• Simplify the script by adding the new package to the end of the file.
• In case of the checkers.rst a non-alpha package is going to be added before the alpha packages.
• According to this change simplify the tests.
• DummyChecker -> ExampleChecker.

In D73521#1923693, @NoQ wrote:

Or is each test run updating the repo? Can we simply make the script do cat DummyChecker.cpp | sed s/DummyChecker/$NAME/g > $NAME.cpp and store the checker only once?

• Fix VLASizeChecker's multi-dimensional array early return.
• So that fix the regression in test misc-ps-region-store.m.
• Fix tests that need regex.
• Add documentation about dumpExtent, dumpElementCount.

"To prevent such errors, either limit copies through truncation or, preferably, ensure that the destination is of sufficient size to hold the character data" - from the rule's page.
Most of the projects are fine truncating by hand because the write happens in somewhat well-bounded strings: IP-addresses, names, numbers... I wanted to make this as practical as possible. Until you are having a null-terminated string without being read, you are most likely fine. Feel free to try this out, probably you would already understand the WarnOnCall option very well.

Thanks for the feedback! Given that it will remain an alpha checker for a long time (~1 year), no one really should use it.

Nice catch, thanks! We have some FIXMEs about MSVC sadly and I was thinking about the same change back in the days.

In D76229#1925363, @f00kat wrote:

In D76229#1925268, @aaron.ballman wrote:

Have you considered making this a static analyzer check as opposed to a clang-tidy check? I think using dataflow analysis will really reduce the false positives because it will be able to track the allocation and alignment data across control flow.

I have never try to write static analyzer before. Okay, I will look at examples of how to work with dataflow.

In D75529#1920796, @vabridgers wrote:

I believe all comments have been addressed. Please let me know if there's anything else required. Thanks

• Try to invoke TableGen, if that fails the user need to specify the path to it.
• The script actually creates a real world (hidden) checker.
  • This checker always made with the build invocation.
  • Its test file always made with the build invocation.
  • Everything else remain as is.
• (calculated: DummyChecker.cpp (100 lines))

In D75529#1912849, @vabridgers wrote:

@Charusso, I cannot find a way to create a test for this change, since this was found using an architecture that supports 16-bit chars.

In D74467#1905416, @NoQ wrote:

In D74467#1905011, @Charusso wrote:

Could you mention how to use this feature in the Summary please?
And something is not right, I have tried it:

Use of uninitialized value $Clang in concatenation (.) or string at /llvm-project/clang/tools/scan-build/bin/scan-build line 1895.

Uh-oh. Does this still show up if you specify --use-analyzer explicitly? Because %scan_build in tests includes this flag because otherwise it has no knowledge of the clang build directory.

sh: 1: : Permission denied

This looks like a separate problem, dunno.

Could you mention how to use this feature in the Summary please?

cd reports
scan-build --generate-index-only .

• Make the tags robust and more unique.

Done, thanks! I will eventually document more. "Some day"... jk.

[Achievement unlocked] 3 green marks.

Thanks everyone! I hope the Analyzer developers start to use the wonderful features from Clang-Tidy.

• Set the size properly.
• Add new debug.ExprInspection patterns: region, size, element count.
• clang-format -i ExprInspectionChecker.cpp.
• Having no idea what is the single regression in tests.

Could you add a test please? We really need tests for every patch.

Cool, that one a lucky one! I think the SymbolRef based world also working, just at some point it could not scale because other systems are region based... For now, it is a much better solution, and this pattern to overload the callback with all the interestingness seems like the standard way of using NoteTags. Thanks!