Page MenuHomePhabricator

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

Authored by mstorsjo on Nov 20 2020, 1:45 AM.

Details

Summary

Alternatively SANITIZER_LIMIT_FRAME_SIZE could be disabled altogether when building for windows.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 20 2020, 1:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2020, 1:45 AM
Herald added subscribers: Restricted Project, dberris. · View Herald Transcript
mstorsjo requested review of this revision.Nov 20 2020, 1:45 AM

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)
  1. Does symbolizing happen on multiple threads? If not, the buffer could be static, which would keep the frame size small.
mstorsjo added inline comments.Dec 1 2020, 12:11 AM
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.
  1. I guess it can happen on multiple threads, for sanitizer error reporting at runtime in cases where error don't terminate the process.
amccarth accepted this revision.Fri, Jan 8, 12:11 PM

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.

This revision is now accepted and ready to land.Fri, Jan 8, 12:11 PM
mstorsjo added inline comments.Fri, Jan 8, 12:46 PM
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!