Rewriter::InsertTextAfterToken may have overflow issue

[shafik]

If we look at Rewriter::InsertTextAfterToken, we see that here:

llvm-project/clang/lib/Rewrite/Rewriter.cpp

Lines 204 to 205 in 3aab3fe

	StartOffs += getRangeSize(SourceRange(Loc, Loc), rangeOpts);
	getEditBuffer(FID).InsertText(StartOffs, Str, /*InsertAfter*/true);

getRangeSize may return a -1 which could lead to an overflow and would likely not be a good value when passed to InsertText.

This was flagged by static analysis. It looks like a valid issue but I am not sure. It seems like the function is not used anymore so maybe the correct fix is to remove it.

[shafik]

CC @jansvoboda11

[jansvoboda11]

On the question of removing the code: This is used in Apple's downstream fork and used in the libclang API. @jkorous-apple, you originally implemented this - do you have any preferences/ideas?

On the question of the static analysis warning, do you mean that StartOffs could potentially underflow if getRangeSize() returns a negative value? If that's the case, the only negative value the function returns is -1 and that happens when either:

• The beginning or the end of the source range !isRewritable(). The caller uses the same Loc for both and avoids calling getRangeSize() whenever !isRewritable(Loc), so this is unreachable.
• The beginning and the end of the source range fall into different files. This is impossible for two identical source locations, so again, unreachable.

So maybe the static analysis tool should be improved? Alternatively, we could assert that the computed source range size is not negative.

If you meant actual overflow, that's possible, but isn't that the case for every other line in LLVM/Clang?

[shafik]

So the scenario is that StartOffs has the value 0 and therefore we overflow into the max unsigned value. I just not familiar enough with the code in question to understand if that is just not possible due to a constraint I don't understand or whether the subsequent code would handle that gracefully i.e. not end up w/ a crash during further evaluation.

If you could help clarify that would be very helpful.

[jansvoboda11]

Ok, if you're talking about StartOffs being 0, getRangeSize() returning -1 and StartOffs underflowing, then I think my explanation for why that can't happen applies.

the only negative value the function returns is -1 and that happens when either:

• The beginning or the end of the source range !isRewritable(). The caller uses the same Loc for both and avoids calling getRangeSize() whenever !isRewritable(Loc), so this is unreachable.
• The beginning and the end of the source range fall into different files. This is impossible for two identical source locations, so again, unreachable.

[shafik]

@jansvoboda11 thank you for your help! So it looks like a false positive then.