In UnsafeBufferUsage.cpp is getEndCharLoc(...) missing an assertion

[shafik]

Static analysis points out that on this line:

llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp

Line 2348 in 12a154a

unsigned TkLen = Lexer::MeasureTokenLength(Node->getEndLoc(), SM, LangOpts);

Lexer::MeasureTokenLength can return 0 and on the subsequent line TkLen - 1 will end up as max unsigned which does not look like the code would deal with that gracefully.

This was brought in via 6341301

It looks like the assumption is that this will never return zero, if that is the case there should be an assertion, something like assert(TkLen !=0); to document this invariant.

[shafik]

CC @ziqingluo-90 @haoNoQ the author and reviewer.

[haoNoQ]

Whoa your static analysis rocks! Is it just a simple check for concrete value overflows or is it something much cleverer?

I like how there's an overflow check in getLocWithOffset() but it doesn't catch this overflow. So it just decrements the sloc by 1 in this case.

Let's just plug it with an early return std::nullopt like every other suspicious case?

[haoNoQ]

CC @jkorous-apple @dtarditi

[ziqingluo-90]

Fix: #129169

[ziqingluo-90]

Whoa your static analysis rocks! Is it just a simple check for concrete value overflows or is it something much cleverer?

I like how there's an overflow check in getLocWithOffset() but it doesn't catch this overflow. So it just decrements the sloc by 1 in this case.

Let's just plug it with an early return std::nullopt like every other suspicious case?

@haoNoQ Glad to see you active online again!

[ziqingluo-90]

@shafik. Shall we close this issue?

Thank you for reporting this bug with good explanation!