[Sanitizer RT] Put the module name string ownership in Symbolizer in order

[timurrrr]

Summary

What do you think about this approach?
I haven't benchmarked it yet.

[timurrrr]

I don't see any significant difference between runtimes of pdfium_test from Chromium with this patch and coverage enabled vs disabled on a 4 MB PDF.
Should I benchmark further?

[kcc]

What is the rationale? I see added complexity but don't see the benefit

sanitizer_symbolizer.cc

78 ↗

(On Diff #22805)

Why do you need a DTOR?
Are you ever going to call it except for when the process is dying?

[timurrrr]

Rationale: we have a possible UAF in the RTL.
We either leak [see r233257] or UAF or manage the strings somehow.
A possible alternative is to replace char **module_name kind of returning a string to "pass us a fixed-size buffer" and char *module_name.

sanitizer_symbolizer.cc

78 ↗

(On Diff #22805)

Correct. Should I remove it?

[kcc]

In D8666#147984, @timurrrr wrote:

Rationale: we have a possible UAF in the RTL.
We either leak [see r233257] or UAF or manage the strings somehow.
A possible alternative is to replace char **module_name kind of returning a string to "pass us a fixed-size buffer" and char *module_name.

"pass us a fixed-size buffer" (i.e. the caller owns the buffer, we copy data there is better here, imho

sanitizer_symbolizer.cc

78 ↗

(On Diff #22805)

Yes, if we decide to proceed with this, but I'd prefer we don't

[timurrrr]

How does the caller pick the buffer size? Also, this way we'll be copying
a lot of strings without the need to do so...

[kcc]

In D8666#147989, @timurrrr wrote:

How does the caller pick the buffer size?

isn't this kMaxPathLength ?

Also, this way we'll be copying
a lot of strings without the need to do so...

How bad is that?
Are we getting the module name in any perf critical place?

[timurrrr]

How does the caller pick the buffer size?

isn't this kMaxPathLength ?

Yes, probably :)

Also, this way we'll be copying a lot of strings without the need to do so...

How bad is that?
Are we getting the module name in any perf critical place?

I haven't measured that yet, but I think it's basically once for each
TU in the default coverage mode.
Possibly more for edge coverage?

[timurrrr]

Like this?

Please note we have a warning if a function takes more than 512 bytes on stack, so I used InternalScopedString instead. This implies mmap+munmap on each TU on process startup, plus a few times on shutdown. I've tried the patch on pdfium_test and chrome and haven't observed much slowdown...

[TODO: update subject before comitting]

[timurrrr]

+glider

[kcc]

In D8666#148920, @timurrrr wrote:

Like this?

Yea.. This clearly sucks too. Probably more than you initial variant.
Let's get back to it and polish it.

Please note we have a warning if a function takes more than 512 bytes on stack, so I used InternalScopedString instead. This implies mmap+munmap on each TU on process startup, plus a few times on shutdown. I've tried the patch on pdfium_test and chrome and haven't observed much slowdown...

[TODO: update subject before comitting]

[timurrrr]

Reverted to the original version, sans destructor

[kcc]

Ok, let's go this way...

sanitizer_symbolizer.h

90 ↗

(On Diff #22900)

Can you modify the comments (and logic) by removing "as long as .. "
These names should be just live forever, independent of anything.

120 ↗

(On Diff #22900)

Why race conditions?
This may happen w/o any threads, right?

125 ↗

(On Diff #22900)

add a comment explaining thread-(un) safety and which lock should be held.

[samsonov]

I think this change is reasonable. Timur, please make sure that (after this goes in) you audit the calls to Symbolizer::GetModuleNameAndOffsetForPC() and Symbolizer::GetModuleNameForPc() and remove internal_strdup() that could encounter at call sites: now we don't have to take ownership of these strings, as they are always owned by the Symbolizer.

sanitizer_symbolizer.cc

153 ↗

(On Diff #22900)

nullptr

sanitizer_symbolizer.h

124 ↗

(On Diff #22900)

Please use a named constant.

128 ↗

(On Diff #22900)

const char *

129 ↗

(On Diff #22900)

Why is this not a const char * ?

[timurrrr]

Addressed the review comments

[timurrrr]

PTAL

sanitizer_symbolizer.cc

153 ↗

(On Diff #22900)

Done

sanitizer_symbolizer.h

90 ↗

(On Diff #22900)

Comment – updated. What logic do I need to change now that the dtor is deleted?

120 ↗

(On Diff #22900)

Theoretically – yes, but not with the current code.
Updated the comment.

124 ↗

(On Diff #22900)

Done

125 ↗

(On Diff #22900)

Done

128 ↗

(On Diff #22900)

yeah, done

129 ↗

(On Diff #22900)

Growth rings... Fixed, thanks for catching!

[timurrrr]

Timur, please make sure that (after this goes in) you audit the calls to Symbolizer::GetModuleNameAndOffsetForPC() and Symbolizer::GetModuleNameForPc() and remove internal_strdup() that could encounter at call sites: now we don't have to take ownership of these strings, as they are always owned by the Symbolizer.

I think I've already done it, but will double-check before committing when everything else is OK.

[samsonov]

I'm OK with the change, but have a suggestion. Feel free to address it if you find it reasonable.

sanitizer_symbolizer.h

125 ↗

(On Diff #22908)

Suggestion: replace the ModuleNameOwner class with just a private method of Symbolizer class. Then you could self-documented code there:

mu_.CheckLocked();

and probably hide last_match_ in the implementation by making a function-static variable storing the cached const char *. It will be thread-safe (protected by mu_), and in practice we don't have many Symbolizer objects anyway.

[timurrrr]

I think ModuleNameOwner might eventually turn into something bigger (e.g. ModulesHistory?) and/or it can be reused in other parts of the RTL (e.g. StringsCache/StringsOwner?), so I think it makes sense to keep it as a separate self-contained type.
I liked the idea of CheckLocked though, so I've decided to just pass a pointer to the Symbolizer::mu_ mutex to the ModuleNameOwner ctor.

Please take a look at r233687.

lib/asan/asan_suppressions.cc

bool IsStackTraceSuppressed(const StackTrace *stack) {
   CHECK(suppression_ctx);
   Symbolizer *symbolizer = Symbolizer::GetOrInit();
   Suppression *s;
   for (uptr i = 0; i < stack->size && stack->trace[i]; i++) {
     uptr addr = stack->trace[i];
 
     if (suppression_ctx->HasSuppressionType(kInterceptorViaLibrary)) {
       // Match "interceptor_via_lib" suppressions.
-      if (const char *module_name = symbolizer->GetModuleNameForPc(addr))
-        if (suppression_ctx->Match(module_name, kInterceptorViaLibrary, &s))
-          return true;
+      InternalScopedString mod_name(kMaxPathLength);
+      if (symbolizer->GetModuleNameForPc(addr, mod_name.data()) &&
+          suppression_ctx->Match(mod_name.data(), kInterceptorViaLibrary, &s))
     }
 
     if (suppression_ctx->HasSuppressionType(kInterceptorViaFunction)) {
       SymbolizedStack *frames = symbolizer->SymbolizePC(addr);
       for (SymbolizedStack *cur = frames; cur; cur = cur->next) {
         const char *function_name = cur->info.function;
         if (!function_name) {
           continue;

lib/lsan/lsan_common.cc

void DoLeakCheck() {
   }
 }
 
 static Suppression *GetSuppressionForAddr(uptr addr) {
   Suppression *s = nullptr;
 
   // Suppress by module name.
   SuppressionContext *suppressions = GetSuppressionContext();
-  if (const char *module_name =
-          Symbolizer::GetOrInit()->GetModuleNameForPc(addr))
-    if (suppressions->Match(module_name, kSuppressionLeak, &s))
-      return s;
+  InternalScopedString module_name(kMaxPathLength);
+  if (Symbolizer::GetOrInit()->GetModuleNameForPc(addr, module_name.data()) &&
+      suppressions->Match(module_name.data(), kSuppressionLeak, &s))
 
   // Suppress by file or function name.
   SymbolizedStack *frames = Symbolizer::GetOrInit()->SymbolizePC(addr);
   for (SymbolizedStack *cur = frames; cur; cur = cur->next) {
     if (suppressions->Match(cur->info.function, kSuppressionLeak, &s) ||
         suppressions->Match(cur->info.file, kSuppressionLeak, &s)) {
       break;
     }

lib/sanitizer_common/sanitizer_coverage_libcdep.cc

void CoverageData::InitializeCounters(u8 *counters, uptr n) {
   num_8bit_counters += n;
 }
 
 void CoverageData::UpdateModuleNameVec(uptr caller_pc, uptr range_beg,
                                        uptr range_end) {
   auto sym = Symbolizer::GetOrInit();
   if (!sym)
     return;
-  const char *module_name = sym->GetModuleNameForPc(caller_pc);
-  if (!module_name) return;
+  InternalScopedString module_name(kMaxPathLength);
+  if (!sym->GetModuleNameForPc(caller_pc, module_name.data()))
+    return;
   if (module_name_vec.empty() ||
-      internal_strcmp(module_name_vec.back().copied_module_name, module_name))
+      internal_strcmp(module_name_vec.back().copied_module_name,
+                      module_name.data()))
      module_name_vec.push_back(
-        {internal_strdup(module_name), range_beg, range_end});
+        { internal_strdup(module_name.data()), range_beg, range_end });
   else
     module_name_vec.back().end = range_end;
 }
 
 void CoverageData::InitializeGuards(s32 *guards, uptr n,
                                     const char *comp_unit_name,
                                     uptr caller_pc) {
   // The array 'guards' has n+1 elements, we use the element zero

 // Dump trace PCs and trace events into two separate files.
 void CoverageData::DumpTrace() {
   uptr max_idx = tr_event_pointer - tr_event_array;
   if (!max_idx) return;
   auto sym = Symbolizer::GetOrInit();
   if (!sym)
     return;
   InternalScopedString out(32 << 20);
+  InternalScopedString module_name(kMaxPathLength);
   for (uptr i = 0, n = size(); i < n; i++) {
-    const char *module_name = "<unknown>";
-    uptr module_address = 0;
-    sym->GetModuleNameAndOffsetForPC(UnbundlePc(pc_array[i]), &module_name,
-                                     &module_address);
-    out.append("%s 0x%zx\n", module_name, module_address);
+    module_name.clear();
+    module_name.append("<unknown>");
+    uptr offset = 0;
+    sym->GetModuleNameAndOffsetForPC(UnbundlePc(pc_array[i]),
+                                     module_name.data(), &offset);
+    out.append("%s 0x%zx\n", module_name.data(), offset);
   }
   InternalScopedString path(kMaxPathLength);
   int fd = CovOpenFile(&path, false, "trace-points");
   if (fd < 0) return;
   internal_write(fd, out.data(), out.length());
   internal_close(fd);
 
   fd = CovOpenFile(&path, false, "trace-compunits");

 void CoverageData::DumpCallerCalleePairs() {
   uptr max_idx = atomic_load(&cc_array_index, memory_order_relaxed);
   if (!max_idx) return;
   auto sym = Symbolizer::GetOrInit();
   if (!sym)
     return;
   InternalScopedString out(32 << 20);
   uptr total = 0;
+  InternalScopedString caller_module_name(kMaxPathLength),
+                       callee_module_name(kMaxPathLength);
   for (uptr i = 0; i < max_idx; i++) {
     uptr *cc_cache = cc_array[i];
     CHECK(cc_cache);
     uptr caller = cc_cache[0];
     uptr n_callees = cc_cache[1];
-    const char *caller_module_name = "<unknown>";
-    uptr caller_module_address = 0;
-    sym->GetModuleNameAndOffsetForPC(caller, &caller_module_name,
-                                     &caller_module_address);
+    caller_module_name.clear();
+    caller_module_name.append("<unknown>");
+    uptr caller_module_offset = 0;
+    sym->GetModuleNameAndOffsetForPC(caller, caller_module_name.data(),
+                                     &caller_module_offset);
     for (uptr j = 2; j < n_callees; j++) {
       uptr callee = cc_cache[j];
       if (!callee) break;
       total++;
-      const char *callee_module_name = "<unknown>";
-      uptr callee_module_address = 0;
-      sym->GetModuleNameAndOffsetForPC(callee, &callee_module_name,
-                                       &callee_module_address);
-      out.append("%s 0x%zx\n%s 0x%zx\n", caller_module_name,
-                 caller_module_address, callee_module_name,
-                 callee_module_address);
+      callee_module_name.clear();
+      callee_module_name.append("<unknown>");
+      uptr callee_module_offset = 0;
+      sym->GetModuleNameAndOffsetForPC(callee, callee_module_name.data(),
+                                       &callee_module_offset);
+      out.append("%s 0x%zx\n%s 0x%zx\n", caller_module_name.data(),
+                 caller_module_offset, callee_module_name.data(),
+                 callee_module_offset);
     }
   }
   InternalScopedString path(kMaxPathLength);
   int fd = CovOpenFile(&path, false, "caller-callee");
   if (fd < 0) return;
   internal_write(fd, out.data(), out.length());
   internal_close(fd);
   VReport(1, " CovDump: %zd caller-callee pairs written\n", total);
for (uptr m = 0; m < module_name_vec.size(); m++) {
     auto r = module_name_vec[m];
     CHECK(r.copied_module_name);
     CHECK_LE(r.beg, r.end);
     CHECK_LE(r.end, size());
     for (uptr i = r.beg; i < r.end; i++) {
       uptr pc = UnbundlePc(pc_array[i]);
       uptr counter = UnbundleCounter(pc_array[i]);
       if (!pc) continue; // Not visited.
-      const char *unused;
       uptr offset = 0;
-      sym->GetModuleNameAndOffsetForPC(pc, &unused, &offset);
+      sym->GetModuleOffsetForPc(pc, &offset);
       offsets.push_back(BundlePcAndCounter(offset, counter));
     }
 
     CHECK_GE(offsets.size(), num_words_for_magic);
     SortArray(offsets.data(), offsets.size());
     for (uptr i = 0; i < offsets.size(); i++)
       offsets[i] = UnbundlePc(offsets[i]);
 

lib/sanitizer_common/sanitizer_symbolizer.h

  public:
   /// Initialize and return platform-specific implementation of symbolizer
   /// (if it wasn't already initialized).
   static Symbolizer *GetOrInit();
   // Returns a list of symbolized frames for a given address (containing
   // all inlined functions, if necessary).
   SymbolizedStack *SymbolizePC(uptr address);
   bool SymbolizeData(uptr address, DataInfo *info);
-  bool GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
-                                   uptr *module_address);
-  const char *GetModuleNameForPc(uptr pc) {
-    const char *module_name = 0;
-    uptr unused;
-    if (GetModuleNameAndOffsetForPC(pc, &module_name, &unused))
-      return module_name;
-    return nullptr;
+
+  // |name| should contain at least kMaxPathLength bytes.
+  bool GetModuleNameAndOffsetForPC(uptr pc, char *name, uptr *offset);
+  bool GetModuleNameForPc(uptr pc, char *name) {
+    return GetModuleNameAndOffsetForPC(pc, name, nullptr);
+  }
+  bool GetModuleOffsetForPc(uptr pc, uptr *offset) {
+    return GetModuleNameAndOffsetForPC(pc, nullptr, offset);
   }
+
   // Release internal caches (if any).
   void Flush();
   // Attempts to demangle the provided C++ mangled name.
   const char *Demangle(const char *name);
   void PrepareForSandboxing();
 
   // Allow user to install hooks that would be called before/after Symbolizer
   // does the actual file/line info fetching. Specific sanitizers may need this

lib/sanitizer_common/sanitizer_symbolizer.cc

for (auto iter = Iterator(&tools_); iter.hasNext();) {
     SymbolizerScope sym_scope(this);
     if (tool->SymbolizeData(addr, info)) {
       return true;
     }
   }
   return true;
 }
 
-bool Symbolizer::GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
-                                 uptr *module_address) {
+bool Symbolizer::GetModuleNameAndOffsetForPC(uptr pc, char *name,
+                                             uptr *offset) {
   BlockingMutexLock l(&mu_);
-  return PlatformFindModuleNameAndOffsetForAddress(pc, module_name,
-                                                   module_address);
+  const char *internal_name;
+  bool ret = PlatformFindModuleNameAndOffsetForAddress(pc, &internal_name,
+                                                       offset);
+  if (ret && name) {
+    internal_strncpy(name, internal_name, kMaxPathLength - 1);
+    name[kMaxPathLength - 1] = '\0';
+  }
+  return ret;
 }
 
 void Symbolizer::Flush() {
   BlockingMutexLock l(&mu_);
   for (auto iter = Iterator(&tools_); iter.hasNext();) {
     auto *tool = iter.next();
     SymbolizerScope sym_scope(this);
     tool->Flush();

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

#endif
   }
 
   bool PlatformFindModuleNameAndOffsetForAddress(uptr address,
                                                  const char **module_name,
                                                  uptr *module_offset) override {
     LoadedModule *module = FindModuleForAddress(address);
     if (module == 0)
       return false;
+    if (module_name)
       *module_name = module->full_name();
+    if (module_offset)
       *module_offset = address - module->base_address();
     return true;
   }
 
   // 16K loaded modules should be enough for everyone.
   static const uptr kMaxNumberOfModuleContexts = 1 << 14;
   LoadedModule modules_[kMaxNumberOfModuleContexts];
   uptr n_modules_;
   // If stale, need to reload the modules before looking up addresses.