In D60281#1630337, @Szelethus wrote:

I'll gladly add the finishing touches :)

In D59279#1438514, @mgrang wrote:

Yes, the reason we limit the checks only to unordered containers is to reduce the false positive rate. Although, as you rightly pointed out that ordered sets of pointers are as non-deterministic as unordered ones. Once our checks have the capability to detect what happens inside the loop maybe we can add ordered sets too. I will add this to the TODO. Thanks.

In D59279#1427017, @mgrang wrote:

Following are the assumptions/limitations of this patch:

1. The assumption is that iteration of ordered containers of pointers is not non-deterministic.

In D53856#1280408, @Szelethus wrote:

In D53856#1279887, @NoQ wrote:

This might be also covered by @rnkovacs's string buffer escape checker - either already or eventually, it'll become just yet another string view API that the checker supports.

I thought about that too, adding some StringRef specific information to that checker makes more sense then essentially duplicating the logic. However, what @MTC mentioned about ArrayRef<T> would be a neat addition too, and maybe it isn't worth making InnerPointerChecker that general.

@rnkovacs, any thoughts?

Return value of dyn_cast_or_null should be checked before use. Otherwise we may put a null pointer into the map as a key and eventually crash in checkDeadSymbols.

In D48027#1203944, @MTC wrote:

However this approach has limit. Given the code below, we cannot distinguish whether the basic_string is user-defined struct or namespace. That's means when the user provide {"std", "basic_string", "append"}, we can only know the qualified name of the call sequentially contains std, basic_string, append. We don't know if these names come from RecordDecl or NamespaceDecl.

namespace  std {
  namespace basic_string {
    struct A {
      void append() {}
    };
  }
}

void foo() {
  std::basic_string::A a;
  a.append(); // Match
}

@rnkovacs What do you think? Can this approach meet InnerPointerChecker's needs?

Address comments & rebase.

In D50211#1190146, @NoQ wrote:

Welcome to the club!

Committed in r339067, I just messed up the revision-closing line in the commit message.

Replace empty Optionals with Nones.

In D50211#1186630, @NoQ wrote:

I see, so that's how it's done!

I also noticed that checker name was weird in exploded graph dumps, i.e. it was showing regular new/delete stuff as if it was done by InnerPointer checker. I'll check if this is fixed tomorrow.

Add helper function to be used in both callbacks.

In D49811#1175726, @NoQ wrote:

I guess you could write a test with debug.AnalysisOrder (by making its checkEndFunction callback (that you'll have to define) print different things depending on the return statement), not sure if it's worth it; you can also merge this commit with D49361 instead.

Rebase.

De-duplicate & add comment.

Updated to use the extended checkEndFunction() callback (committed in rL337215 - I forgot to add it as a dependency).

I'm not sure how to test this.
I'll need it in D49361 when I update it to use the changed checkEndFunction() callback, and that will kind of test this too.

Tiny bit more re-structuring.

Fix note for function pointers & handle argument counting in member operator calls.
I also refactored the code a little, because after moving things from checkPreCall to checkPostCall, the structure was a bit confusing.

Addressed comments & added two test cases for function pointers.

Two more reports on Ceph that seem to be true positives (no other reports from this checker):

1. Here (or if it does not work, the bug is on L130 here).
2. Here (or L363 and L373 here).

Note messages updated.

In D49360#1163113, @NoQ wrote:

Also we rarely commit to adding a test for every single supported API function; bonus points for that, but usually 2-3 functions from a series of similar functions is enough :)

Added standard quote, marking the section about non-member functions that may also invalidate the buffer as a TODO.
Also changed the note message to that suggested by @NoQ (thanks!). All tests pass now.

In D49058#1159533, @george.karpenkov wrote:

@rnkovacs Do you have evaluation statistics handy for this checker? How many bugs it finds, on which projects? How many of those are real bugs?

Fix test run line.

Thanks very much for your review!

Addressed comments.

No crashes on Harfbuzz, ICU, Bitcoin, and LLVM. I'll commit.

Fixed the constness of c_str() in the test file.

Fixed variable name inside the visitor.
I also clang-formatted the file, sorry for any line number shifting.

Thanks! Addressed comments.

Thanks for the comments!
I'll run this on some projects and see if any assertions fail.

Um, sorry, I totally forgot about that. Added your case to the tests.

Addressed comments.

Fixed naming and added an extra pass for regions left behind by incomplete destructors.

In D45517#1117898, @mikhail.ramalho wrote:

Just want to comment here and give thanks again for the first version of
the refutation code. It's being really helpful to develop the approach this
code as a base; things would definitely be slower if I had to start it from
scratch.

Added a check for UnknownVal and two FIXMEs (one for the OriginExpr and one for the new CheckKind).

Address (most) comments.

• All basic_string types are now supported.
• Mock tests added.
• New AllocationFamily AF_InternalBuffer introduced.
• NewDeleteChecker dependency added.

Thanks for your comments!

Adding a preliminary test file.

Expression chaining is fixed. The visitor now collects constraints that are about to disappear along the bug path and checks them once in the end.

In D45517#1074057, @NoQ wrote:

The visitor currently checks states appearing as block edges in the exploded graph. The first idea was to filter states based on the shape of the exploded graph, by checking the number of successors of the parent node, but surprisingly, both succ_size() and pred_size() seemed to return 1 for each node in the graph (except for the root), even if there clearly were branchings in the code (and on the .dot picture). To my understanding, the exploded graph is fully constructed at the stage where visitors are run, so I must be missing something.

Aha, yep, that's probably because visitors are operating on the "trimmed" exploded graph. You can paint it via the -trim-egraph flag or by calling ViewGraph(1) in the debugger.

Fixed logical operator in the Z3ConstraintManager::checkRangedStateConstraints() function.

I extended the warning message to include more information. What do you think?

In D41816#970845, @xazax.hun wrote:

Overall looks good to me, one comment inline. I think it is good to have these checks to prevent the analyzer executing undefined behavior. Maybe this would make it more feasible to run the analyzer with ubsan :)
In the future, it would be great to also look for these cases symbolically, but I believe it is perfectly fine to have that in a separate patch.

In D35796#878200, @dcoughlin wrote:

This looks good to me! Do you have commit access, or do you need someone to commit it for you?

• Accidentally left-in comment removed.
• Checker file clang-formatted.

Sorry for the late reply. I did run it on a few open-source projects as well as LLVM/Clang and honestly it didn't find anything. As the test cases seem to work fine it might already be in a state ready to bring out of alpha.

Thanks for the comments. I improved the docs and truncated the messages in the test file.