[clang-tidy] Add recursion protection in ExceptionSpecAnalyzer

[PiotrZSL]

Normally endless recursion should not happen in ExceptionSpecAnalyzer, but if AST would be malformed (missing include), this could cause crash.

I run into this issue when due to missing include constructor argument were parsed as FieldDecl.
As checking for recursion cost nothing, why not to do this in check just in case.

Fixes #111436

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Changes

Normally endless recursion should not happen in ExceptionSpecAnalyzer, but if AST would be malformed (missing include), this could cause crash.

I run into this issue when due to missing include constructor argument were parsed as FieldDecl.
As checking for recursion cost nothing, why not to do this in check just in case.

---

Full diff: https://github.com/llvm/llvm-project/pull/66810.diff

1 Files Affected:

• (modified) clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp (+7-6)

diff --git a/clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp b/clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp index 4dd4a95f880aca0..5ffa4a92f574e0b 100644 --- a/clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp +++ b/clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp @@ -14,13 +14,14 @@ namespace clang::tidy::utils { ExceptionSpecAnalyzer::State ExceptionSpecAnalyzer::analyze(const FunctionDecl *FuncDecl) { - // Check if the function has already been analyzed and reuse that result. - const auto CacheEntry = FunctionCache.find(FuncDecl); - if (CacheEntry == FunctionCache.end()) { + // Check if function exist in cache or add temporary value to cache to protect + // against endless recursion. + const auto [CacheEntry, NotFound] = + FunctionCache.try_emplace(FuncDecl, State::NotThrowing); + if (NotFound) { ExceptionSpecAnalyzer::State State = analyzeImpl(FuncDecl); - - // Cache the result of the analysis. - FunctionCache.try_emplace(FuncDecl, State); + // Update result with calculated value + FunctionCache[FuncDecl] = State; return State; } 

[carlosgalvezp]

Being totally unfamiliar with the code, I have a feeling that we shouldn't be fixing in the analyze function. analyze has 1 single responsibility: wrap analyzeImpl in order to add cacheability for performance reasons, it does so simply and well, and it does exactly what one would expect - no more no less.

However, the real problem is that analyzeImpl ultimately might end up calling itself, via analyzeUnresolvedOrDefaulted -> analyzeRecord -> analyze -> analyzeImpl (IIUC). This is not obvious due to how the code is written, and this patch would make it even less obvious IMO.

In that sense I believe it would make sense to ensure no recursion happens somewhere in the chain I described above, not in analyze. WDYT?

[PiotrZSL]

I put it into analyze because when running this locally I seen that analyze show up repeatedly in stacktrace. Even in chain that you showed we still got analyze in this chain. And adding protection there is cheap, as we can easily utilize cached map to do that, instead of adding more maps. Also this issue isn't something that should happen often, I run into it only because I got malformed AST in first place. This is other reason why I would prefer to fix it in current way.

[carlosgalvezp]

To clarify, I'm not concerned about performance, but with code readability and maintainability.

I realize the chain that I wrote above should not lead to recursion due to SkipMethods::Yes in the relevant call. I'd like to take a closer look at the problem, would you be able to share some of the stacktrace that shows how the recursion is happening?

[AMS21]

I think this is a very reasonable change and something I didn't think about when implementing it originally.
LGTM 👍

[5chmidti]

There is a minimal repro in #111436 available

[layus]

@PiotrZSL What are the chances of landing this as-is ? Does this require the changes mentioned by @carlosgalvezp ?

[PiotrZSL]

Actually only thing that were missing were example.
Added test. I'm not adding release notes as this is crash on invalid anyway, and I don't want to add multiple entrys, as this impact multiple checks.

[5chmidti]

Being totally unfamiliar with the code, I have a feeling that we shouldn't be fixing in the analyze function. analyze has 1 single responsibility: wrap analyzeImpl in order to add cacheability for performance reasons, it does so simply and well, and it does exactly what one would expect - no more no less.

This class could maintain a stack-like SmallVector to check for an infinite recursion., so instead of checking the function cache (which might be large), only a few functions have to be compared this way.

[PiotrZSL]

This class could maintain a stack-like SmallVector to check for an infinite recursion., so instead of checking the function cache (which might be large), only a few functions have to be compared this way.

Actually this change does not impact performance. Before we had 2 lookups, and now we still got 2 lookups.

[5chmidti]

LGTM, but please add a release note about fixing the infinite recursion

[5chmidti]

And maybe @carlosgalvezp wants to comment

[carlosgalvezp]

LGTM, had a small comment. Not sure it's worth mentioning this in the releases notes, it's just a crash fix. But it doesn't hurt I guess.