Assertion "PathDiagnosticSpotPiece's must have a valid location."

[steakhal]

Reproducer: https://godbolt.org/z/3M1v5Y37s

clang --analyze -Xclang -analyzer-checker=core,alpha.security.ReturnPtrRange preprocessed.cpp

namespace std {
template <typename T> T&& move(T &&) noexcept;
} // namespace std

char buf[1];

void top() {
  (void)std::move(*(buf + 3)); // crashes
}

Stack-trace:

clang: llvm-project/clang/include/clang/Analysis/PathDiagnostic.h:519: clang::ento::PathDiagnosticSpotPiece::PathDiagnosticSpotPiece(const clang::ento::PathDiagnosticLocation &, llvm::StringRef, PathDiagnosticPiece::Kind, bool): Assertion `Pos.isValid() && Pos.hasValidLocation() && "PathDiagnosticSpotPiece's must have a valid location."' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: llvm-project/build/debug/bin/clang --analyze -Xclang -analyzer-checker=core,alpha.security.ReturnPtrRange preprocessed.cpp
1.	<eof> parser at end of file
2.	While analyzing stack: 
	#0 Calling std::move(char &) at line 8
	#1 Calling top(int)
3.	Error evaluating statement
4.	Error evaluating statement
 #0 0x00007f563011921a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) llvm-project/llvm/lib/Support/Unix/Signals.inc:565:11
 #1 0x00007f56301193cb PrintStackTraceSignalHandler(void*) llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
 #2 0x00007f5630117a66 llvm::sys::RunSignalHandlers() llvm-project/llvm/lib/Support/Signals.cpp:103:5
 #3 0x00007f5630118b0e llvm::sys::CleanupOnSignal(unsigned long) llvm-project/llvm/lib/Support/Unix/Signals.inc:362:1
 #4 0x00007f562ff6db8e (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:0:7
 #5 0x00007f562ff6df73 CrashRecoverySignalHandler(int) llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp:390:1
 #6 0x00007f5637d4f980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
 #7 0x00007f562ef42e87 raise /build/glibc-uZu3wS/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #8 0x00007f562ef447f1 abort /build/glibc-uZu3wS/glibc-2.27/stdlib/abort.c:81:0
 #9 0x00007f562ef343fa __assert_fail_base /build/glibc-uZu3wS/glibc-2.27/assert/assert.c:89:0
#10 0x00007f562ef34472 (/lib/x86_64-linux-gnu/libc.so.6+0x30472)
#11 0x00007f56250c1cd4 clang::ento::PathDiagnosticSpotPiece::PathDiagnosticSpotPiece(clang::ento::PathDiagnosticLocation const&, llvm::StringRef, clang::ento::PathDiagnosticPiece::Kind, bool) llvm-project/clang/include/clang/Analysis/PathDiagnostic.h:520:21
#12 0x00007f56250c1bb8 clang::ento::PathDiagnosticNotePiece::PathDiagnosticNotePiece(clang::ento::PathDiagnosticLocation const&, llvm::StringRef, bool) llvm-project/clang/include/clang/Analysis/PathDiagnostic.h:731:9
#13 0x00007f56250c1b50 void __gnu_cxx::new_allocator<clang::ento::PathDiagnosticNotePiece>::construct<clang::ento::PathDiagnosticNotePiece, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&>(clang::ento::PathDiagnosticNotePiece*, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/ext/new_allocator.h:156:60
#14 0x00007f56250c1942 void std::allocator_traits<std::allocator<clang::ento::PathDiagnosticNotePiece> >::construct<clang::ento::PathDiagnosticNotePiece, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&>(std::allocator<clang::ento::PathDiagnosticNotePiece>&, clang::ento::PathDiagnosticNotePiece*, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/alloc_traits.h:516:2
#15 0x00007f56250c16e9 std::_Sp_counted_ptr_inplace<clang::ento::PathDiagnosticNotePiece, std::allocator<clang::ento::PathDiagnosticNotePiece>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<clang::ento::PathDiagnosticLocation const&, llvm::StringRef&>(std::allocator<clang::ento::PathDiagnosticNotePiece>, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:521:2
#16 0x00007f56250c152a std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<clang::ento::PathDiagnosticNotePiece, std::allocator<clang::ento::PathDiagnosticNotePiece>, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&>(clang::ento::PathDiagnosticNotePiece*&, std::_Sp_alloc_shared_tag<std::allocator<clang::ento::PathDiagnosticNotePiece> >, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:650:16
#17 0x00007f56250c1485 std::__shared_ptr<clang::ento::PathDiagnosticNotePiece, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<clang::ento::PathDiagnosticNotePiece>, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&>(std::_Sp_alloc_shared_tag<std::allocator<clang::ento::PathDiagnosticNotePiece> >, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:1337:14
#18 0x00007f56250c1412 std::shared_ptr<clang::ento::PathDiagnosticNotePiece>::shared_ptr<std::allocator<clang::ento::PathDiagnosticNotePiece>, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&>(std::_Sp_alloc_shared_tag<std::allocator<clang::ento::PathDiagnosticNotePiece> >, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:410:4
#19 0x00007f56250c1365 std::shared_ptr<clang::ento::PathDiagnosticNotePiece> std::allocate_shared<clang::ento::PathDiagnosticNotePiece, std::allocator<clang::ento::PathDiagnosticNotePiece>, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&>(std::allocator<clang::ento::PathDiagnosticNotePiece> const&, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:860:14
#20 0x00007f56250c11b2 std::shared_ptr<clang::ento::PathDiagnosticNotePiece> std::make_shared<clang::ento::PathDiagnosticNotePiece, clang::ento::PathDiagnosticLocation const&, llvm::StringRef&>(clang::ento::PathDiagnosticLocation const&, llvm::StringRef&) /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:876:7
#21 0x00007f56250bf44c clang::ento::BugReport::addNote(llvm::StringRef, clang::ento::PathDiagnosticLocation const&, llvm::ArrayRef<clang::SourceRange>) llvm-project/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:205:26
#22 0x00007f562567c860 (anonymous namespace)::ReturnPointerRangeChecker::checkPreStmt(clang::ReturnStmt const*, clang::ento::CheckerContext&) const llvm-project/clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp:116:5

Likely related to invalid source locations produced by BodyFarm #40680 (https://reviews.llvm.org/D60808).

PS: It's not entirely clear what the semantics/invariants are in the PathDiagnosticLocation class. When and what can be invalid etc, thus I'm requesting some feedback on this by @haoNoQ.

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

[haoNoQ]

Well, you obviously can't put a note on something that's not present in the source code.

In this case it's std::move implementation synthesized by Sema - no, this doesn't seem to have anything to do with our body farms, Sema does this for us (though I had no idea this could happen).

I'm curious how does your checker prefer the note to look in this case. Why is this a non-path note in the first place?

[steakhal]

In this case it's std::move implementation synthesized by Sema - no, this doesn't seem to have anything to do with our body farms, Sema does this for us (though I had no idea this could happen).

It seems like b27430f introduced this crash. It's a fairly recent change.

I'm curious how does your checker prefer the note to look in this case. Why is this a non-path note in the first place?

Could you elaborate on this part?

[haoNoQ]

I mean, instead of coming up with contracts for PathDiagnosticPieces with invalid source locations out of thin air, we should infer them from practical use cases. This issue is an example of a practical use case. What can we learn from it?

You're trying to put a non-path note (PathDiagnosticNotePiece produced by BugReport::addNote(), as opposed to PathDiagnosticEventPiece typically produced by visitors). What information does the user infer from that note? Is there a different place in the source code where it's appropriate to display this information? If so, maybe the note should be rewritten to reflect the location change? What happens if we omit the note entirely? - does the report become incomprehensible?

[steakhal]

You're trying to put a non-path note (PathDiagnosticNotePiece produced by BugReport::addNote(), as opposed to PathDiagnosticEventPiece typically produced by visitors).

To be precise, I just filed this issue. I tried to factually gather all information I had, triage, and pinpoint possible causes.
I'm doing the same with the rest of the CSA bugreports. This way I feel I gain experience, and also save time for you.
I feel a bit uncomfortable by reading "You are trying" and a question flood. Anyway.

You're trying to put a non-path note (PathDiagnosticNotePiece produced by BugReport::addNote(), as opposed to PathDiagnosticEventPiece typically produced by visitors).

If I understand you correctly, what you are stating is that no path-sensitive checker should use the BugReport::addNote(), since it wouldn't have the expected effect. Rather, one should use PathDiagnosticEventPieces.

What information does the user infer from that note? Is there a different place in the source code where it's appropriate to display this information? If so, maybe the note should be rewritten to reflect the location change? What happens if we omit the note entirely? - does the report become incomprehensible?

What do you think about how and where should we put a note that would describe how big the underlying buffer is?
This information would be critical for understanding why the return pointer points to an out-of-bounds element.

[steakhal]

Possible fix at https://reviews.llvm.org/D138713.