[PATCH] D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`

[Brian Gesiak via Phabricator via cfe-commits]

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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D75542.247949.patch
Type: text/x-patch
Size: 813 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200303/c2191703/attachment.bin>