[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

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

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

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

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

119

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

124

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–160

nullptr

sanitizer_symbolizer.h

123

Please use a named constant.

127

const char *

128

Why is this not a const char * ?

[timurrrr]

Addressed the review comments

[timurrrr]

PTAL

sanitizer_symbolizer.cc

153–160

Done

sanitizer_symbolizer.h

90

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

119

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

123

Done

124

Done

127

yeah, done

128

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

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.

sanitizer_coverage_libcdep.cc

 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;
   if (module_name_vec.empty() ||
-      internal_strcmp(module_name_vec.back().copied_module_name, module_name))
-    module_name_vec.push_back(
-        {internal_strdup(module_name), range_beg, range_end});
+      module_name_vec.back().copied_module_name != module_name)
+    module_name_vec.push_back({module_name, 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
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->GetModuleNameAndOffsetForPC(pc, nullptr, &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]);
 

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);
+
+  // The module names Symbolizer returns are stable and unique for every given
+  // module.  It is safe to store and compare them as pointers.
   bool GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
                                    uptr *module_address);
   const char *GetModuleNameForPc(uptr pc) {
-    const char *module_name = 0;
+    const char *module_name = nullptr;
     uptr unused;
     if (GetModuleNameAndOffsetForPC(pc, &module_name, &unused))
       return module_name;
     return nullptr;
   }
+
   // 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
   // to distinguish system library calls made in user code from calls made
   // during in-process symbolization.
   typedef void (*StartSymbolizationHook)();
   typedef void (*EndSymbolizationHook)();
   // May be called at most once.
   void AddHooks(StartSymbolizationHook start_hook,
                 EndSymbolizationHook end_hook);
 
  private:
+  // GetModuleNameAndOffsetForPC has to return a string to the caller.
+  // Since the corresponding module might get unloaded later, we should create
+  // our owned copies of the strings that we can safely return.
+  // ModuleNameOwner does not provide any synchronization, thus calls to
+  // its method should be protected by |mu_|.
+  class ModuleNameOwner {
+   public:
+    ModuleNameOwner() : storage_(kInitialCapacity), last_match_(nullptr) {}
+    const char *GetOwnedCopy(const char *str);
+
+   private:
+    static const uptr kInitialCapacity = 1000;
+    InternalMmapVector<const char*> storage_;
+    const char *last_match_;
+  } module_names_;
+
   /// Platform-specific function for creating a Symbolizer object.
   static Symbolizer *PlatformInit();
 
   virtual bool PlatformFindModuleNameAndOffsetForAddress(
       uptr address, const char **module_name, uptr *module_offset) {
     UNIMPLEMENTED();
   }
   // Platform-specific default demangler, must not return nullptr.

sanitizer_symbolizer.cc

 
 void Symbolizer::AddHooks(Symbolizer::StartSymbolizationHook start_hook,
                           Symbolizer::EndSymbolizationHook end_hook) {
   CHECK(start_hook_ == 0 && end_hook_ == 0);
   start_hook_ = start_hook;
   end_hook_ = end_hook;
 }
 
+const char *Symbolizer::ModuleNameOwner::GetOwnedCopy(const char *str) {
+  // 'str' will be the same string multiple times in a row, optimize this case.
+  if (last_match_ && !internal_strcmp(last_match_, str))
+    return last_match_;
+
+  // FIXME: this is linear search.
+  // We should optimize this further if this turns out to be a bottleneck later.
+  for (uptr i = 0; i < storage_.size(); ++i) {
+    if (!internal_strcmp(storage_[i], str)) {
+      last_match_ = storage_[i];
+      return last_match_;
+    }
+  }
+  last_match_ = internal_strdup(str);
+  storage_.push_back(last_match_);
+  return last_match_;
+}
+
 Symbolizer::Symbolizer(IntrusiveList<SymbolizerTool> tools)
     : tools_(tools), start_hook_(0), end_hook_(0) {}
 
 Symbolizer::SymbolizerScope::SymbolizerScope(const Symbolizer *sym)
     : sym_(sym) {
   if (sym_->start_hook_)
     sym_->start_hook_();
 }
for (auto iter = Iterator(&tools_); iter.hasNext();) {
     if (tool->SymbolizeData(addr, info)) {
       return true;
     }
   }
   return true;
 }
 
 bool Symbolizer::GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
                                              uptr *module_address) {
   BlockingMutexLock l(&mu_);
-  return PlatformFindModuleNameAndOffsetForAddress(pc, module_name,
-                                                   module_address);
+  const char *internal_module_name = nullptr;
+  if (!PlatformFindModuleNameAndOffsetForAddress(pc, &internal_module_name,
+                                                 module_address))
+    return false;
+
+  if (module_name)
+    *module_name = module_names_.GetOwnedCopy(internal_module_name);
+  return true;
 }
 
 void Symbolizer::Flush() {
   BlockingMutexLock l(&mu_);
   for (auto iter = Iterator(&tools_); iter.hasNext();) {
     auto *tool = iter.next();
     SymbolizerScope sym_scope(this);
     tool->Flush();