Page MenuHomePhabricator

Fix memory allocating inside signal handler (MI)
ClosedPublic

Authored by ki.stfu on Mar 11 2015, 8:27 AM.

Details

Summary

This patch fixes a memory allocating inside signal handler.

This bug was found by @vharron:

Hi all,

I noticed these thread sanitizer warnings while running lldb-mi tests on
Linux.

WARNING: ThreadSanitizer: signal-unsafe call inside of a signal (pid=39721)
#0 operator new(unsigned long) <null>:0 (lldb-mi-3.7.0+0x000000092b9d)

#1 std::string::_Rep::_S_create(unsigned long, unsigned long,

std::allocator<char> const&) <null>:0 (libstdc++.so.6+0x0000000ba3b8)

#2 CMICmnResources::GetStringFromResource(unsigned int, CMIUtilString&)

const
/usr/local/google/home/vharron/ll/tot/llvm/tools/lldb/tools/lldb-mi/MICmnResources.cpp:434
(lldb-mi-3.7.0+0x00000014ddd8)

#3 CMICmnResources::GetString(unsigned int) const

/usr/local/google/home/vharron/ll/tot/llvm/tools/lldb/tools/lldb-mi/MICmnResources.cpp:371
(lldb-mi-3.7.0+0x00000014db81)

#4 sigwinch_handler(int)

/usr/local/google/home/vharron/ll/tot/llvm/tools/lldb/tools/lldb-mi/MIDriverMain.cpp:99
(lldb-mi-3.7.0+0x0000001589ff)

#5 __tsan::CallUserSignalHandler(__tsan::ThreadState*, bool, bool, int,

my_siginfo_t*, void*) tsan_interceptors.o:0 (lldb-mi-3.7.0+0x00000009f25f)

#6 void std::this_thread::sleep_for<long, std::ratio<1l, 1000l>

(std::chrono::duration<long, std::ratio<1l, 1000l> > const&)

/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/thread:279
(lldb-mi-3.7.0+0x00000013243e)

#7 CMIDriver::ReadStdinLineQueue()

/usr/local/google/home/vharron/ll/tot/llvm/tools/lldb/tools/lldb-mi/MIDriver.cpp:710
(lldb-mi-3.7.0+0x000000155e62)

#8 CMIDriver::DoMainLoop()

/usr/local/google/home/vharron/ll/tot/llvm/tools/lldb/tools/lldb-mi/MIDriver.cpp:631
(lldb-mi-3.7.0+0x000000155d37)

#9 non-virtual thunk to CMIDriver::DoMainLoop()

/usr/local/google/home/vharron/ll/tot/llvm/tools/lldb/tools/lldb-mi/MIDriver.cpp:648
(lldb-mi-3.7.0+0x000000155fbd)

#10 CMIDriverMgr::DriverMainLoop()

/usr/local/google/home/vharron/ll/tot/llvm/tools/lldb/tools/lldb-mi/MIDriverMgr.cpp:340
(lldb-mi-3.7.0+0x000000159ee6)

#11 main

/usr/local/google/home/vharron/ll/tot/llvm/tools/lldb/tools/lldb-mi/MIDriverMain.cpp:364
(lldb-mi-3.7.0+0x000000158f60)

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 21728.Mar 11 2015, 8:27 AM
ki.stfu retitled this revision from to Fix memory allocating inside signal handler (MI).
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: vharron, abidh.
ki.stfu added subscribers: vharron, abidh, Unknown Object (MLST).
abidh edited edge metadata.Mar 11 2015, 10:36 AM

I think best will be to remove that logging from the signal handler. The signal handler in the lldb does not log anything too. Moving forward, I think lldb-mi should only handle the MI mode. I dont see the reason why it also tries to be a normal driver. If someone wants command line driver, he can just call lldb. When that happens, we can just remove handler for sigwinch as it is not really useful for lldb-mi.

Let's wait for Vince's review.

vharron edited edge metadata.Mar 12 2015, 7:56 AM

If you can reproduce the error without the patch and then it goes away with Thread Sanitizer, then I approve.

Here is how I build with Thread Sanitizer

cmake -GNinja \
-DCMAKE_C_COMPILER=clang-3.5 \
-DCMAKE_CXX_COMPILER=clang++-3.5 \
-DCMAKE_BUILD_TYPE=Debug ../../llvm \
-DLLVM_USE_SANITIZER=Thread && \
ninja

(run your test)

ki.stfu planned changes to this revision.Mar 17 2015, 10:54 AM
ki.stfu updated this revision to Diff 22161.Mar 18 2015, 12:40 AM
ki.stfu edited edge metadata.

Revert CMICmnResources; Remove logging from signal handlers

ki.stfu updated this revision to Diff 22163.Mar 18 2015, 1:04 AM

Remove unused IDS_PROCESS_SIGNAL_RECEIVED from CMICmnResources

Please, take a look again.

abidh accepted this revision.Mar 19 2015, 6:37 AM
abidh edited edge metadata.

Looks ok.

This revision is now accepted and ready to land.Mar 19 2015, 6:37 AM
ki.stfu closed this revision.Mar 19 2015, 10:19 AM