Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Fix segmentation fault in lldb_private::Symbols::LocateExecutableSymbolFile()

Authored by enlight on Sep 27 2015, 10:46 PM.



When module_spec.GetFileSpec().GetDirectory().AsCString() returned a nullptr this line caused a segmentation fault:

std::string module_directory = module_spec.GetFileSpec().GetDirectory().AsCString()

Some context:
I was remote debugging an executable built with Clang in an Ubuntu VM on my Windows machine using lldb-mi. I copied the executable and nothing else from the Ubuntu VM to the Windows machine.

Then started lldb-server in the Ubuntu VM:

./bin/lldb-server gdbserver *:8888 -- /home/enlight/Projects/dbgmits/build/Debug/data_tests_target

And ran lldb-mi --interpreter on Windows with the following commands:

-file-exec-and-symbols C:\Projects\data_tests_target
-target-select remote

After which the segmentation fault occurred at the aforementioned line. Inside this method module_spec.GetFileSpec() returns an empty FileSpec (no dir, no filename), while module_spec.GetSymbolFileSpec().GetFilename() returns "".

Diff Detail


Event Timeline

enlight updated this revision to Diff 35832.Sep 27 2015, 10:46 PM
enlight retitled this revision from to Fix segmentation fault in lldb_private::Symbols::LocateExecutableSymbolFile().
enlight updated this object.
enlight added reviewers: brucem, clayborg, zturner.
enlight set the repository for this revision to rL LLVM.
brucem edited edge metadata.Sep 27 2015, 11:26 PM

Talking to Vadim (enlight) offline, this happened when debugging an inferior on a Linux target from a Windows host. He had only copied the executable back to the Windows host, so the shared libraries involved weren't present on his system.

I'm not sure if this is the right place for this fix or if it should be somewhere else prior to this point.

zturner requested changes to this revision.Sep 27 2015, 11:26 PM
zturner edited edge metadata.

I know you are just following the existing pattern, but this whole function appears broken in the presence of file systems that use backslash instead of forward slash. Could you try to fix that? I've provided some suggestions inline.

I also don't see any tests for this. We've been pretty lax about this historically, but I want to start pushing back against CLs with no tests. If you need help writing a test, let me know and we can figure something out. For starters, what is the scenario you encountered this in?

238–250 ↗(On Diff #35832)

Can you do all of this only if the target is not Windows?

263–270 ↗(On Diff #35832)

Don't use literal slashes anywhere. I know you're just copying the existing code, which was already broken, but it seems easy enough to fix all this by calling llvm::sys::path::append, which will do the right thing dependign on the Host platform.

Since this code is in Host, it's ok to do this, since we're ultimately going to pass this path directly through to the filesystem anyway, we already need to be able to assume that we're dealing with a path that has the appropriate separator for the platform anyway.

This revision now requires changes to proceed.Sep 27 2015, 11:26 PM
enlight updated this object.Sep 27 2015, 11:50 PM
enlight edited edge metadata.

I've updated the summary with the scenario. I don't know how one would go about writing a test for this, mixed platform remote debugging is a bit finicky.

238–250 ↗(On Diff #35832)

You mean if the host is not Windows? Skipping /usr/lib/debug on Windows makes sense, but it seems like the uuid_str stuff could still apply if the symbol files are copied to or shared with Windows.

263–270 ↗(On Diff #35832)

I'll submit a separate patch for the requested path separator changes. I'd prefer to do it after this patch though.

As for the test, this could be a good candidate for a unit test. It's not advertised very well so there's definitely some work for us to do on that front, but basically you can run ninja check-lldb-unit. Seems like you could just create a ModuleSpec on the stack, construct it the way you want, and pass it to this function, and verify the output in a couple of different scenarios. At a minimum the one you ran into this segfault with, but if you can think of a couple of other useful scenarios to test it wouldn't hurt to have a couple go in at the same time.

238–250 ↗(On Diff #35832)

You're right, the host. Also seems reasonable about the uuid_str stuff.

enlight updated this revision to Diff 36468.Oct 4 2015, 8:34 AM
enlight edited edge metadata.

Added a couple of unit tests, the Windows-specific cleanups will be submitted in a separate patch.

zturner accepted this revision.Oct 4 2015, 2:11 PM
zturner edited edge metadata.

Looks great, thanks

This revision is now accepted and ready to land.Oct 4 2015, 2:11 PM
clayborg accepted this revision.Oct 5 2015, 1:11 PM
clayborg edited edge metadata.

Back from vacation, sorry for the delay. Looks good.

This revision was automatically updated to reflect the committed changes.