[compiler-rt] [sanitizer] Silence -Wframe-larger-than= for a few windows functions with large stack buffers

[mstorsjo]

Summary

Alternatively SANITIZER_LIMIT_FRAME_SIZE could be disabled altogether when building for windows.

[amccarth]

I prefer the relatively localized disabling of the warning with the #pragmas as you've done as opposed to building the sanitizer builds with different settings.

I'm concerned about what looks to be an existing possible buffer overrun.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

144

1. The sample code used sizeof(TCHAR) not CHAR. So I'm guessing this is a potential stack buffer overrun bug for "Unicode" builds. (Windows defines CHAR to char as indicated here: https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types)

2. Does symbolizing happen on multiple threads? If not, the buffer could be static, which would keep the frame size small.

[mstorsjo]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

144

1. In https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/ns-dbghelp-symbol_info it looks like the SYMBOL_INFO struct explicitly uses CHAR and isn't available in an unicode form, so this code seems to be correct in that aspect.

2. I guess it can happen on multiple threads, for sanitizer error reporting at runtime in cases where error don't terminate the process.

[mstorsjo]

Ping @amccarth

[amccarth]

LGTM, but please take the suggested edit to update a documentation URL.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

143

URL update in case the redirect goes bad in the future.

144

1. Well, there is a wide version called SYMBOL_INFOW for use with SymFromAddrW, see https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/ns-dbghelp-symbol_infow

Unlike other Win32 APIs, the "wide" versions seem to be enabled by DBGHELP_TRANSLATE_TCHAR rather than UNICODE. LLVM doesn't seem to use that, so I guess this works for now. Using sizeof(TCHAR) now might ease a future transition, but I'll leave it up to you.

[mstorsjo]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

143

Sure, will add that change.

144

I'd hold off of such further refactorings as part of this commit, and just stick to silencing warnings, but thanks for the pointers!

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp

#undef DBGHELP_IMPORT
   CHECK_NE(last_bslash, 0);
   *last_bslash = L'\0';
   if (!SymSetSearchPathW(GetCurrentProcess(), path_buffer)) {
     Report("*** WARNING: Failed to SymSetSearchPathW\n");
     return;
   }
 }
 
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wframe-larger-than="
+#endif
 bool WinSymbolizerTool::SymbolizePC(uptr addr, SymbolizedStack *frame) {
   InitializeDbgHelpIfNeeded();
 
-  // See http://msdn.microsoft.com/en-us/library/ms680578(VS.85).aspx
+  // See https://docs.microsoft.com/en-us/windows/win32/debug/retrieving-symbol-information-by-address
   char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(CHAR)];
   PSYMBOL_INFO symbol = (PSYMBOL_INFO)buffer;
   symbol->SizeOfStruct = sizeof(SYMBOL_INFO);
   symbol->MaxNameLen = MAX_SYM_NAME;
   DWORD64 offset = 0;
   BOOL got_objname = SymFromAddr(GetCurrentProcess(),
                                  (DWORD64)addr, &offset, symbol);
   if (!got_objname)
     return false;
 
   DWORD unused;
   IMAGEHLP_LINE64 line_info;
   line_info.SizeOfStruct = sizeof(IMAGEHLP_LINE64);
   BOOL got_fileline = SymGetLineFromAddr64(GetCurrentProcess(), (DWORD64)addr,
                                            &unused, &line_info);
   frame->info.function = internal_strdup(symbol->Name);
   frame->info.function_offset = (uptr)offset;
   if (got_fileline) {
     frame->info.file = internal_strdup(line_info.FileName);
     frame->info.line = line_info.LineNumber;
   }
   // Only consider this a successful symbolization attempt if we got file info.
   // Otherwise, try llvm-symbolizer.
   return got_fileline;
 }
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
 
 const char *WinSymbolizerTool::Demangle(const char *name) {
   CHECK(is_dbghelp_initialized);
   static char demangle_buffer[1000];
   if (name[0] == '\01' &&
       UnDecorateSymbolName(name + 1, demangle_buffer, sizeof(demangle_buffer),
                            UNDNAME_NAME_ONLY))
     return demangle_buffer;

compiler-rt/lib/sanitizer_common/sanitizer_unwind_win.cpp

void BufferedStackTrace::UnwindSlow(uptr pc, u32 max_depth) {
   uptr pc_location = LocatePcInTrace(pc);
   PopStackFrames(pc_location);
 
   // Replace the first frame with the PC because the frame in the
   // stacktrace might be incorrect.
   trace_buffer[0] = pc;
 }
 
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wframe-larger-than="
+#endif
 void BufferedStackTrace::UnwindSlow(uptr pc, void *context, u32 max_depth) {
   CHECK(context);
   CHECK_GE(max_depth, 2);
   CONTEXT ctx = *(CONTEXT *)context;
   STACKFRAME64 stack_frame;
   memset(&stack_frame, 0, sizeof(stack_frame));
 
   InitializeDbgHelpIfNeeded();
#endif
   stack_frame.AddrStack.Mode = AddrModeFlat;
   while (StackWalk64(machine_type, GetCurrentProcess(), GetCurrentThread(),
     &stack_frame, &ctx, NULL, SymFunctionTableAccess64,
     SymGetModuleBase64, NULL) &&
     size < Min(max_depth, kStackTraceMax)) {
     trace_buffer[size++] = (uptr)stack_frame.AddrPC.Offset;
   }
 }
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
 #endif  // #if !SANITIZER_GO
 
 #endif  // SANITIZER_WINDOWS

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

static uptr GetPreferredBase(const char *modname) {
   // Check for more magic in the PE header.
   if (pe_header->Magic != IMAGE_NT_OPTIONAL_HDR_MAGIC)
     return 0;
 
   // Finally, return the ImageBase.
   return (uptr)pe_header->ImageBase;
 }
 
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wframe-larger-than="
+#endif
 void ListOfModules::init() {
   clearOrInit();
   HANDLE cur_process = GetCurrentProcess();
 
   // Query the list of modules.  Start by assuming there are no more than 256
   // modules and retry if that's not sufficient.
   HMODULE *hmodules = 0;
   uptr modules_buffer_size = sizeof(HMODULE) * 256;
for (size_t i = 0; i < num_modules; ++i) {
     cur_module.set(module_name, adjusted_base);
     // We add the whole module as one single address range.
     cur_module.addAddressRange(base_address, end_address, /*executable*/ true,
                                /*writable*/ true);
     modules_.push_back(cur_module);
   }
   UnmapOrDie(hmodules, modules_buffer_size);
 }
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
 
 void ListOfModules::fallbackInit() { clear(); }
 
 // We can't use atexit() directly at __asan_init time as the CRT is not fully
 // initialized at this point.  Place the functions into a vector and use
 // atexit() as soon as it is ready for use (i.e. after .CRT$XIC initializers).
 InternalMmapVectorNoCtor<void (*)(void)> atexit_functions;