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

[enlight]

Summary

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 192.168.56.101:8888
-exec-continue

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 "libc-2.19.so".

[brucem]

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]

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?

source/Host/common/Symbols.cpp

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.

[enlight]

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.

source/Host/common/Symbols.cpp

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.

[zturner]

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.

source/Host/common/Symbols.cpp

238–250 ↗

(On Diff #35832)

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

[enlight]

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

[zturner]

Looks great, thanks

[clayborg]

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

lldb/trunk/source/Host/common/Symbols.cpp

if (symbol_filename && symbol_filename[0])
         {
             // Some debug files are stored in the .build-id directory like this:
             //   /usr/lib/debug/.build-id/ff/e7fe727889ad82bb153de2ad065b2189693315.debug
             uuid_str = module_uuid.GetAsString("");
             uuid_str.insert (2, 1, '/');
             uuid_str = uuid_str + ".debug";
         }
 
-        // Get directory of our module. Needed to check debug files like this:
-        //   /usr/lib/debug/usr/lib/library.so.debug
-        std::string module_directory = module_spec.GetFileSpec().GetDirectory().AsCString();
-
         size_t num_directories = debug_file_search_paths.GetSize();
         for (size_t idx = 0; idx < num_directories; ++idx)
         {
             FileSpec dirspec = debug_file_search_paths.GetFileSpecAtIndex (idx);
             dirspec.ResolvePath();
             if (!dirspec.Exists() || !dirspec.IsDirectory())
                 continue;
 
             std::vector<std::string> files;
             std::string dirname = dirspec.GetPath();
 
             files.push_back (dirname + "/" + symbol_filename);
             files.push_back (dirname + "/.debug/" + symbol_filename);
             files.push_back (dirname + "/.build-id/" + uuid_str);
-            files.push_back (dirname + module_directory + "/" + symbol_filename);
+
+            // Some debug files may stored in the module directory like this:
+            //   /usr/lib/debug/usr/lib/library.so.debug
+            if (!file_dir.IsEmpty())
+                files.push_back (dirname + file_dir.AsCString() + "/" + symbol_filename);
 
             const uint32_t num_files = files.size();
             for (size_t idx_file = 0; idx_file < num_files; ++idx_file)
             {
                 const std::string &filename = files[idx_file];
                 FileSpec file_spec (filename.c_str(), true);
 
                 if (llvm::sys::fs::equivalent (file_spec.GetPath(), module_spec.GetFileSpec().GetPath()))

lldb/trunk/unittests/Host/CMakeLists.txt

 add_lldb_unittest(HostTests
   SocketAddressTest.cpp
   SocketTest.cpp
+  SymbolsTest.cpp
   )

lldb/trunk/unittests/Host/SymbolsTest.cpp

+//===-- SymbolsTest.cpp -----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+#include "lldb/Host/Symbols.h"
+#include "lldb/Core/ModuleSpec.h"
+
+using namespace lldb_private;
+
+TEST(SymbolsTest, LocateExecutableSymbolFileForUnknownExecutableAndUnknownSymbolFile)
+{
+    ModuleSpec module_spec;
+    FileSpec symbol_file_spec = Symbols::LocateExecutableSymbolFile(module_spec);
+    EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
+}
+
+TEST(SymbolsTest, LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile)
+{
+    ModuleSpec module_spec;
+    // using a GUID here because the symbol file shouldn't actually exist on disk
+    module_spec.GetSymbolFileSpec().SetFile("4A524676-B24B-4F4E-968A-551D465EBAF1.so", false);
+    FileSpec symbol_file_spec = Symbols::LocateExecutableSymbolFile(module_spec);
+    EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
+}