This is an archive of the discontinued LLVM Phabricator instance.

[Darwin] Fix a bug where the symbolizer would examine the wrong process.
ClosedPublic

Authored by delcypher on Apr 6 2020, 7:23 PM.

Details

Summary

Previously AtosSymbolizer would set the PID to examine in the
constructor which is called early on during sanitizer init. This can
lead to incorrect behaviour in the case of a fork() because if the
symbolizer is launched in the child it will be told examine the parent
process rather than the child.

To fix this the PID is determined just before the symbolizer is
launched.

A test case is included that triggers the buggy behaviour that existed
prior to this patch. The test observes the PID that atos was called
on. It also examines the symbolized stacktrace. Prior to this patch
atos failed to symbolize the stacktrace giving output that looked
like...

#0 0x100fc3bb5 in __sanitizer_print_stack_trace asan_stack.cpp:86
#1 0x10490dd36 in PrintStack+0x56 (/path/to/print-stack-trace-in-code-loaded-after-fork.cpp.tmp_shared_lib:x86_64+0xd36)
#2 0x100f6f986 in main+0x4a6 (path/to/print-stack-trace-in-code-loaded-after-fork.cpp.tmp_loader:x86_64+0x100001986)
#3 0x7fff714f1cc8 in start+0x0 (/usr/lib/system/libdyld.dylib:x86_64+0x1acc8)

After this patch stackframes #1 and #2 are fully symbolized.

This patch is also a pre-requisite refactor for rdar://problem/58789439.

Diff Detail

Event Timeline

delcypher created this revision.Apr 6 2020, 7:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 7:23 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
kubamracek accepted this revision.Apr 6 2020, 8:43 PM
This revision is now accepted and ready to land.Apr 6 2020, 8:43 PM
yln added inline comments.Apr 7 2020, 11:03 AM
compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp
45

Can you explain why the separately loaded lib/dlopen is required to trigger the error condition? From the commit message I thought that a fork should be enough? Other than that: LGTM

delcypher marked an inline comment as done.Apr 7 2020, 12:24 PM
delcypher added inline comments.
compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp
45

I'm deliberately loading code into the child process that doesn't exist in the parent process. If atos is told examine the parent process then it will be told to examine a PC in PrintStack which isn't loaded in that process and so will fail.

I'll add a comment to the test case to explain the reasoning here.

delcypher updated this revision to Diff 255864.Apr 7 2020, 5:10 PM

Add comment explaining loading of dylib.

delcypher marked an inline comment as done.Apr 7 2020, 5:15 PM

@yln Is the comment I added clear enough?

yln accepted this revision.Apr 7 2020, 5:40 PM
yln marked an inline comment as done.
yln added inline comments.
compiler-rt/test/sanitizer_common/TestCases/Darwin/print-stack-trace-in-code-loaded-after-fork.cpp
45

So before, we were always calling atos with the wrong pid (parent pid), but it only matters in cases where we try to symbolize code that isn't actually present in the parent. Got it, thanks!

This revision was automatically updated to reflect the committed changes.