modocache created this revision.
modocache added reviewers: rsmith, RKSimon, aaron.ballman, wenlei.
Herald added a project: clang.
A closed-source C++ codebase I help maintain began hitting a Clang ICE
with a stack trace that referenced
`clang::OverloadCandidate::getNumParams`:
https://gist.github.com/modocache/84eac9c519796644139471dd06ef4628
Specifically, Clang was querying `getNumParams`, the implementation of
which checks the `IsSurrogate` member:
unsigned getNumParams() const {
if (IsSurrogate) {
QualType STy = Surrogate->getConversionType();
// ...
}
// ...
}
However, the `OverloadCandidate` instance had not initialized that member
with a value, triggering UB. In the case of the stack trace, `IsSurrogate`
was evaluated as if it were `true`, and the access into uninitialized
`Surrogate` caused the crash.
Every other use of `clang::OverloadCandidate` initializes `IsSurrogate`
with a value, so I do so in this patch in order to avoid triggering UB
in the case of my closed-source codebase. I believe the potential for UB
was introduced in this commit from January 9:
https://github.com/llvm/llvm-project/commit/25195541349b1d6dfc03bf7511483110bda69b29
Alternatively, I considered modifying the `clang::OverloadCandidate`
constructor to initialize `IsSurrogate` with a `false` value. I feel doing
so would be safer, but I stuck with what appears to be the convention:
initializing `OverloadCandidate` members are the site of use.
Unfortunately I don't have a small example I can share of how
`getNumParams` can be called in an invalid state. The closed-source C++
code that triggers the UB looks something like this and only occurs when
using GCC's libc++:
class MyClass {
std::pair<std::string, int> myPair;
// ...
void myMemberFunctionoFoo(std::pair<std::string, int> p) {}
void myMemberFunctionBar() {
std::bind(&MyClass::myMemberFunctionFoo, this, myPair);
}
};
I tried reverse-engineering a test for this patch by commenting out the
if statements added in the commit that I think introduced the UB,
https://github.com/llvm/llvm-project/commit/25195541349b1d6dfc03bf7511483110bda69b29.
But in fact I found that removing these if statements entirely did not
cause any tests in check-clang to fail, so I'm concerned the code may be
untested at the moment.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D75542
Files:
clang/lib/Sema/SemaOverload.cpp
Index: clang/lib/Sema/SemaOverload.cpp
===================================================================
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -6978,6 +6978,7 @@
OverloadCandidate &Candidate = CandidateSet.addCandidate();
Candidate.FoundDecl = FoundDecl;
Candidate.Function = FunctionTemplate->getTemplatedDecl();
+ Candidate.IsSurrogate = false;
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_explicit;
return;
@@ -7363,6 +7364,7 @@
OverloadCandidate &Candidate = CandidateSet.addCandidate();
Candidate.FoundDecl = FoundDecl;
Candidate.Function = FunctionTemplate->getTemplatedDecl();
+ Candidate.IsSurrogate = false;
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_explicit;
return;
Index: clang/lib/Sema/SemaOverload.cpp
===================================================================
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -6978,6 +6978,7 @@
OverloadCandidate &Candidate = CandidateSet.addCandidate();
Candidate.FoundDecl = FoundDecl;
Candidate.Function = FunctionTemplate->getTemplatedDecl();
+ Candidate.IsSurrogate = false;
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_explicit;
return;
@@ -7363,6 +7364,7 @@
OverloadCandidate &Candidate = CandidateSet.addCandidate();
Candidate.FoundDecl = FoundDecl;
Candidate.Function = FunctionTemplate->getTemplatedDecl();
+ Candidate.IsSurrogate = false;
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_explicit;
return;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
