[UBSan] Allow UBSan location to store frames returned by symbolizer.

[samsonov]

Summary

__ubsan::getFunctionLocation() used to issue a call to symbolizer, and
convert the result (SymbolizedStack) to one of UBSan structures:
SourceLocation, ModuleLocation or MemoryLocation. This:
(1) is inefficient: we do an extra allocation/deallocation to copy data,
while we can instead can just pass SymbolizedStack around (which
contains all the necessary data).
(2) leaks memory: strings stored in SourceLocation/MemoryLocation are
never deallocated, and Filipe Cabecinhas suggests this causes crashes
of UBSan-ified programs in the wild.

Instead, let Location store a pointer to SymbolizedStack object, and
make sure it's properly deallocated when UBSan handler exits.

ModuleLocation is made obsolete by this change, and is deleted.

[filcab]

LGTM with the call to InternalFree.

lib/ubsan/ubsan_diag.h

29

Aren't we still leaking the SymbolizedStack object, here? (as well as in the destructor and reset() (which call clear()))
I guess we're missing a call to InternalFree()

[samsonov]

lib/ubsan/ubsan_diag.h

29

SymbolizedStack::ClearAll() calls InternalFree(this), so it shouldn't be necessary.

[filcab]

Ah, yes. Thanks.

[filcab]

Although, wouldn't calling free() on the this pointer be undefined behavior? After we (conceptually) return from InternalFree, we have an invalid this pointer. Wouldn't this be UB? We're not calling methods on it, but we end up inside a member function (however briefly) with an invalid this pointer.

I couldn't find it in the standard, but I'm sure @rsmith knows the answer :-)

[samsonov]

"delete this" is a common pattern and I believe we're fine here provided we're careful enough and don't access the object after InternalFree() statement.

lib/ubsan/ubsan_diag.h

 //
 //===----------------------------------------------------------------------===//
 #ifndef UBSAN_DIAG_H
 #define UBSAN_DIAG_H
 
 #include "ubsan_value.h"
 #include "sanitizer_common/sanitizer_stacktrace.h"
 #include "sanitizer_common/sanitizer_suppressions.h"
+#include "sanitizer_common/sanitizer_symbolizer.h"
 
 namespace __ubsan {
 
-/// \brief A location within a loaded module in the program. These are used when
-/// the location can't be resolved to a SourceLocation.
-class ModuleLocation {
-  const char *ModuleName;
-  uptr Offset;
+class SymbolizedStackHolder {
+  SymbolizedStack *Stack;
+
+  void clear() {
+    if (Stack)
+      Stack->ClearAll();
+  }
 
 public:
-  ModuleLocation() : ModuleName(0), Offset(0) {}
-  ModuleLocation(const char *ModuleName, uptr Offset)
-    : ModuleName(ModuleName), Offset(Offset) {}
-  const char *getModuleName() const { return ModuleName; }
-  uptr getOffset() const { return Offset; }
+  explicit SymbolizedStackHolder(SymbolizedStack *Stack = nullptr)
+      : Stack(Stack) {}
+  ~SymbolizedStackHolder() { clear(); }
+  void reset(SymbolizedStack *S) {
+    if (Stack != S)
+      clear();
+    Stack = S;
+  }
+  const SymbolizedStack *get() const { return Stack; }
 };
 
+SymbolizedStack *getSymbolizedLocation(uptr PC);
+
+inline SymbolizedStack *getCallerLocation(uptr CallerPC) {
+  CHECK(CallerPC);
+  uptr PC = StackTrace::GetPreviousInstructionPc(CallerPC);
+  return getSymbolizedLocation(PC);
+}
+
 /// A location of some data within the program's address space.
 typedef uptr MemoryLocation;
 
 /// \brief Location at which a diagnostic can be emitted. Either a
-/// SourceLocation, a ModuleLocation, or a MemoryLocation.
+/// SourceLocation, a MemoryLocation, or a SymbolizedStack.
 class Location {
 public:
-  enum LocationKind { LK_Null, LK_Source, LK_Module, LK_Memory };
+  enum LocationKind { LK_Null, LK_Source, LK_Memory, LK_Symbolized };
 
 private:
   LocationKind Kind;
   // FIXME: In C++11, wrap these in an anonymous union.
   SourceLocation SourceLoc;
-  ModuleLocation ModuleLoc;
   MemoryLocation MemoryLoc;
+  const SymbolizedStack *SymbolizedLoc;  // Not owned.
 
 public:
   Location() : Kind(LK_Null) {}
   Location(SourceLocation Loc) :
     Kind(LK_Source), SourceLoc(Loc) {}
-  Location(ModuleLocation Loc) :
-    Kind(LK_Module), ModuleLoc(Loc) {}
   Location(MemoryLocation Loc) :
     Kind(LK_Memory), MemoryLoc(Loc) {}
+  // SymbolizedStackHolder must outlive Location object.
+  Location(const SymbolizedStackHolder &Stack) :
+    Kind(LK_Symbolized), SymbolizedLoc(Stack.get()) {}
 
   LocationKind getKind() const { return Kind; }
 
   bool isSourceLocation() const { return Kind == LK_Source; }
-  bool isModuleLocation() const { return Kind == LK_Module; }
   bool isMemoryLocation() const { return Kind == LK_Memory; }
+  bool isSymbolizedStack() const { return Kind == LK_Symbolized; }
 
   SourceLocation getSourceLocation() const {
     CHECK(isSourceLocation());
     return SourceLoc;
   }
-  ModuleLocation getModuleLocation() const {
-    CHECK(isModuleLocation());
-    return ModuleLoc;
-  }
   MemoryLocation getMemoryLocation() const {
     CHECK(isMemoryLocation());
     return MemoryLoc;
   }
+  const SymbolizedStack *getSymbolizedStack() const {
+    CHECK(isSymbolizedStack());
+    return SymbolizedLoc;
+  }
 };
 
-/// Try to obtain a location for the caller. This might fail, and produce either
-/// an invalid location or a module location for the caller.
-Location getCallerLocation(uptr CallerPC);
-
-/// Try to obtain a location for the given function pointer. This might fail,
-/// and produce either an invalid location or a module location for the caller.
-/// If FName is non-null and the name of the function is known, set *FName to
-/// the function name, otherwise *FName is unchanged.
-Location getFunctionLocation(uptr PC, const char **FName);
-
 /// A diagnostic severity level.
 enum DiagLevel {
   DL_Error, ///< An error.
   DL_Note   ///< A note, attached to a prior diagnostic.
 };
 
 /// \brief Annotation for a range of locations in a diagnostic.
 class Range {

lib/ubsan/ubsan_diag.cc

public:
   Decorator() : SanitizerCommonDecorator() {}
   const char *Highlight() const { return Green(); }
   const char *EndHighlight() const { return Default(); }
   const char *Note() const { return Black(); }
   const char *EndNote() const { return Default(); }
 };
 }
 
-Location __ubsan::getCallerLocation(uptr CallerPC) {
-  if (!CallerPC)
-    return Location();
-
-  return getFunctionLocation(StackTrace::GetPreviousInstructionPc(CallerPC), 0);
-}
-
-Location __ubsan::getFunctionLocation(uptr PC, const char **FName) {
-  if (!PC)
-    return Location();
+SymbolizedStack *__ubsan::getSymbolizedLocation(uptr PC) {
   InitIfNecessary();
-
-  SymbolizedStack *Frames = Symbolizer::GetOrInit()->SymbolizePC(PC);
-  const AddressInfo &Info = Frames->info;
-
-  if (!Info.module) {
-    Frames->ClearAll();
-    return Location(PC);
-  }
-
-  if (FName && Info.function)
-    *FName = internal_strdup(Info.function);
-
-  if (!Info.file) {
-    ModuleLocation MLoc(internal_strdup(Info.module), Info.module_offset);
-    Frames->ClearAll();
-    return MLoc;
-  }
-
-  SourceLocation SLoc(internal_strdup(Info.file), Info.line, Info.column);
-  Frames->ClearAll();
-  return SLoc;
+  return Symbolizer::GetOrInit()->SymbolizePC(PC);
 }
 
 Diag &Diag::operator<<(const TypeDescriptor &V) {
   return AddArg(V.getTypeName());
 }
 
 Diag &Diag::operator<<(const Value &V) {
   if (V.getType().isSignedIntegerTy())
case Location::LK_Source: {
     SourceLocation SLoc = Loc.getSourceLocation();
     if (SLoc.isInvalid())
       LocBuffer.append("<unknown>");
     else
       RenderSourceLocation(&LocBuffer, SLoc.getFilename(), SLoc.getLine(),
                            SLoc.getColumn(), common_flags()->strip_path_prefix);
     break;
   }
-  case Location::LK_Module: {
-    ModuleLocation MLoc = Loc.getModuleLocation();
-    RenderModuleLocation(&LocBuffer, MLoc.getModuleName(), MLoc.getOffset(),
-                         common_flags()->strip_path_prefix);
-    break;
-  }
   case Location::LK_Memory:
     LocBuffer.append("%p", Loc.getMemoryLocation());
     break;
+  case Location::LK_Symbolized: {
+    const AddressInfo &Info = Loc.getSymbolizedStack()->info;
+    if (Info.file) {
+      RenderSourceLocation(&LocBuffer, Info.file, Info.line, Info.column,
+                           common_flags()->strip_path_prefix);
+    } else if (Info.module) {
+      RenderModuleLocation(&LocBuffer, Info.module, Info.module_offset,
+                           common_flags()->strip_path_prefix);
+    } else {
+      LocBuffer.append("%p", Info.address);
+    }
+    break;
+  }
   case Location::LK_Null:
     LocBuffer.append("<unknown>");
     break;
   }
   Printf("%s:", LocBuffer.data());
 }
 
 static void renderText(const char *Message, const Diag::Arg *Args) {

lib/ubsan/ubsan_handlers.cc

 
 static void handleTypeMismatchImpl(TypeMismatchData *Data, ValueHandle Pointer,
                                    ReportOptions Opts) {
   Location Loc = Data->Loc.acquire();
   // Use the SourceLocation from Data to track deduplication, even if 'invalid'
   if (ignoreReport(Loc.getSourceLocation(), Opts))
     return;
 
-  if (Data->Loc.isInvalid())
-    Loc = getCallerLocation(Opts.pc);
+  SymbolizedStackHolder FallbackLoc;
+  if (Data->Loc.isInvalid()) {
+    FallbackLoc.reset(getCallerLocation(Opts.pc));
+    Loc = FallbackLoc;
+  }
 
   ScopedReport R(Opts, Loc);
 
   if (!Pointer)
     Diag(Loc, DL_Error, "%0 null pointer of type %1")
       << TypeCheckKinds[Data->TypeCheckKind] << Data->Type;
   else if (Data->Alignment && (Pointer & (Data->Alignment - 1)))
     Diag(Loc, DL_Error, "%0 misaligned address %1 for type %3, "
void __ubsan::__ubsan_handle_vla_bound_not_positive_abort(VLABoundData *Data,
   GET_REPORT_OPTIONS(true);
   handleVLABoundNotPositive(Data, Bound, Opts);
   Die();
 }
 
 static void handleFloatCastOverflow(FloatCastOverflowData *Data,
                                     ValueHandle From, ReportOptions Opts) {
   // TODO: Add deduplication once a SourceLocation is generated for this check.
-  Location Loc = getCallerLocation(Opts.pc);
+  SymbolizedStackHolder CallerLoc(getCallerLocation(Opts.pc));
+  Location Loc = CallerLoc;
   ScopedReport R(Opts, Loc);
 
   Diag(Loc, DL_Error,
        "value %0 is outside the range of representable values of type %2")
       << Value(Data->FromType, From) << Data->FromType << Data->ToType;
 }
 
 void __ubsan::__ubsan_handle_float_cast_overflow(FloatCastOverflowData *Data,
static void handleFunctionTypeMismatch(FunctionTypeMismatchData *Data,
                                        ValueHandle Function,
                                        ReportOptions Opts) {
   SourceLocation CallLoc = Data->Loc.acquire();
   if (ignoreReport(CallLoc, Opts))
     return;
 
   ScopedReport R(Opts, CallLoc);
 
-  const char *FName = "(unknown)";
-  Location FLoc = getFunctionLocation(Function, &FName);
+  SymbolizedStackHolder FLoc(getSymbolizedLocation(Function));
+  const char *FName = FLoc.get()->info.function;
+  if (!FName)
+    FName = "(unknown)";
 
   Diag(CallLoc, DL_Error,
        "call to function %0 through pointer to incorrect function type %1")
       << FName << Data->Type;
   Diag(FLoc, DL_Note, "%0 defined here") << FName;
 }
 
 void