[BSDArchive] NULL check the child object file ptr before accessing its member

[kusmour]

Summary

Recently we've observed lldb crashes caused by missing object file linked to a thin archive (.a) files. The crash is due to a missing NULL check in the code when looking for child object file referred by the thin archive. Malformed archive file should not crash LLDB. Instead, it should report the error and continue.

New error message will look like the following

error: libfoo.a(__objects__/foo/barAppDelegate.mm.o) failed to load objfile for path/to/libfoo.a.
Debugging will be degraded for this module.

Test Plan:

llvm-lit test

./bin/llvm-lit -sv ../llvm-project/lldb/test/API/functionalities/archives/TestBSDArchives.py

Test without code change will error out with LLDB crash

--
Command Output (stderr):
--
PASS: LLDB (~/llvm-upstream/Debug/bin/clang-arm64) :: test (TestBSDArchives.BSDArchivesTestCase)
PASS: LLDB (~/llvm-upstream/Debug/bin/clang-arm64) :: test_frame_var_errors_when_archive_missing (TestBSDArchives.BSDArchivesTestCase)
FAIL: LLDB (~/llvm-upstream/Debug/bin/clang-arm64) :: test_frame_var_errors_when_mtime_mistmatch_for_object_in_archive (TestBSDArchives.BSDArchivesTestCase)
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      HandleCommand(command = "b a")
1.      HandleCommand(command = "breakpoint set --name 'a'")
Fatal Python error: Segmentation fault

Current thread 0x00000001f7b99e00 (most recent call first):
  File "~/llvm-upstream/Debug/bin/LLDB.framework/Resources/Python/lldb/__init__.py", line 3270 in HandleCommand
  File "~/llvm-upstream/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2070 in runCmd
  File "~/llvm-upstream/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2421 in expect
  File "~/llvm-upstream/llvm-project/lldb/test/API/functionalities/archives/TestBSDArchives.py", line 156 in test_frame_var_errors_when_thin_archive_malformed
...

[bulbazord]

This makes sense to me. I have one question and one change request though.

lldb/source/Core/Module.cpp

1288–1290

Why did you switch from the llvm::formatv style to the printf style?

lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp

543–545

This will only fail if child_data_sp is valid and the sizes aren't matching. You probably want to fail if child_data_sp is invalid every time, not just if the sizes differ.

[kusmour]

lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp

543–545

If we fail on invalid child_data_sp every time, then it won't fall back to FindPlugin below. Is it guarantee that invalid child_data_sp indicates failure finding object file?

[yinghuitan]

How hard it is to add an invalid archive to trigger the crash and test this code path?

[kusmour]

Switch back to use llvm::formatv printf and rebase

[bulbazord]

Comment aside, I think Jeffrey brings up a good point. Can you add a test to trigger this code path? I'm not sure if you can create an archive from yaml2obj, but perhaps you can use yaml2obj to create an object file and put it into an archive?

lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp

543–545

Hmm, that's a good point. Reading the implementation of ObjectFile::FindPlugin, it looks like it does have a check for child_data_sp and handles this case explicitly. My suggestion doesn't apply and what you've written is likely correct.

[kusmour]

In D156367#4536870, @yinghuitan wrote:

How hard it is to add an invalid archive to trigger the crash and test this code path?

There's API test for archive already, I will add one more test case :D

[clayborg]

lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp

543–545

We should only continue calling into ObjectFile::FindPlugin if child_data_sp is valid. So this should be:

543–545

In any ObjectContainer subclasss, like ObjectContainerBSDArchive, the idea is one files contain multiple different object files. If we fail to extract the part of the data from the current container file, then there is no reason to continue. I suggest using the code I suggested above which is exactly what Alex suggested as they is the right thing to do!

[clayborg]

In D156367#4536929, @kusmour wrote:

In D156367#4536870, @yinghuitan wrote:

How hard it is to add an invalid archive to trigger the crash and test this code path?

There's API test for archive already, I will add one more test case :D

You might see if "obj2yaml" supports BSD files. If so, you can create a simple BSD file with one .o file and then run obj2yaml on it so it can then test by running yaml2obj. The idea is you might be able to edit the yaml so that the file offsets are incorrect so we can reproduce. This might case the "yaml2obj" to fail though as there may be checks that prohibit bad BSD files from being created. Otherwise we might need to check in a .a file with a single small .o file and then do some hex editing on the file to mess up the file offset and or file size.

[clayborg]

Another possible way to make a bad .a file is to truncate the file. We would need to figure out where to truncate it, but that might be an easier way to make a bad .a file

[kusmour]

Added API test for malformed thin BSD archive
Addressed comments

[yinghuitan]

Assuming the test fails/crashes without the change then LGTM.

[bulbazord]

My concerns have been addressed. LGTM

[bulbazord]

Hey, I think this may have broken the green dragon bots. Could you take a look and fix forward or revert? Thanks

https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/3262/ (You can ignore the import-std-module tests, those have been broken some time and are being worked on AFAIK).

[kusmour]

In D156367#4539916, @bulbazord wrote:

Hey, I think this may have broken the green dragon bots. Could you take a look and fix forward or revert? Thanks

https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/3262/ (You can ignore the import-std-module tests, those have been broken some time and are being worked on AFAIK).

Fixed by commit: 4520cc066b2ffe5ac261e3aca887cba3f113b1ff

lldb/source/Core/Module.cpp

if (!m_did_load_objfile.load()) {
         if (m_objfile_sp) {
           // Once we get the object file, update our module with the object
           // file's architecture since it might differ in vendor/os if some
           // parts were unknown.  But since the matching arch might already be
           // more specific than the generic COFF architecture, only merge in
           // those values that overwrite unspecified unknown values.
           m_arch.MergeFrom(m_objfile_sp->GetArchitecture());
         } else {
-          ReportError("failed to load objfile for {0}",
+          ReportError("failed to load objfile for {0}\nDebugging will be "
+                      "degraded for this module.",
                       GetFileSpec().GetPath().c_str());
         }
       }
     }
   }
   return m_objfile_sp.get();
 }
 
 SectionList *Module::GetSectionList() {

lldb/source/Plugins/ObjectContainer/BSD-Archive/ObjectContainerBSDArchive.cpp

if (module_sp->GetObjectName() && m_archive_sp) {
           // Set file to child object file
           FileSpec child = GetChildFileSpecificationsFromThin(
               object->ar_name.GetStringRef(), m_file);
           lldb::offset_t file_offset = 0;
           lldb::offset_t file_size = object->size;
           std::shared_ptr<DataBuffer> child_data_sp =
               FileSystem::Instance().CreateDataBuffer(child, file_size,
                                                       file_offset);
-          if (child_data_sp->GetByteSize() != object->file_size)
+          if (!child_data_sp ||
+              child_data_sp->GetByteSize() != object->file_size)
             return ObjectFileSP();
           lldb::offset_t data_offset = 0;
           return ObjectFile::FindPlugin(
               module_sp, &child, m_offset + object->file_offset,
               object->file_size, child_data_sp, data_offset);
         }
         lldb::offset_t data_offset = object->file_offset;
         return ObjectFile::FindPlugin(
             module_sp, file, m_offset + object->file_offset, object->file_size,

lldb/test/API/functionalities/archives/Makefile

 C_SOURCES := main.c a.c b.c c.c
 EXE :=  # Define a.out explicitly
 MAKE_DSYM := NO
 
-all: a.out libbar.a
+all: a.out libbar.a libfoo-thin.a
 
 a.out: main.o libfoo.a
   $(LD) $(LDFLAGS) $^ -o $@
 
 libfoo.a: a.o b.o
   $(AR) $(ARFLAGS) $@ $^
 
 # This tests whether lldb can load a thin archive
 libbar.a: c.o
   $(eval LLVM_AR := $(LLVM_TOOLS_DIR)/llvm-ar)
   $(eval LLVM_ARFLAGS := -rcsDT)
   $(LLVM_AR) $(LLVM_ARFLAGS) $@ $^
 
+libfoo-thin.a: a.o b.o
+  $(eval LLVM_AR := $(LLVM_TOOLS_DIR)/llvm-ar)
+  $(eval LLVM_ARFLAGS := -rcsDT)
+  $(LLVM_AR) $(LLVM_ARFLAGS) $@ $^
+
 include Makefile.rules

lldb/test/API/functionalities/archives/TestBSDArchives.py

def test_frame_var_errors_when_archive_missing(self):
         error_strings = [
             'debug map object file "',
             'libfoo.a(a.o)" containing debug info does not exist, debug info will not be loaded',
         ]
         self.check_frame_variable_errors(thread, error_strings)
 
     @skipIfRemote
     @skipUnlessDarwin
+    def test_frame_var_errors_when_thin_archive_malformed(self):
+        """
+        Create thin archive libfoo.a and make it malformed to make sure
+        we don't crash and report an appropriate error when resolving
+        breakpoint using debug map.
+        """
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        libfoo_path = self.getBuildArtifact("libfoo.a")
+        libthin_path = self.getBuildArtifact("libfoo-thin.a")
+        objfile_a = self.getBuildArtifact("a.o")
+        # Replace the libfoo.a file with a thin archive containing the same
+        # debug information (a.o, b.o). Then remove a.o from the file system
+        # so we force an error when we set a breakpoint on a() function.
+        # Since the a.o is missing, the debug info won't be loaded and we
+        # should see an error when trying to break into a().
+        os.remove(libfoo_path)
+        shutil.copyfile(libthin_path, libfoo_path)
+        os.remove(objfile_a)
+
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+        # We won't be able to see source file
+        self.expect(
+            "b a",
+            substrs=["Breakpoint 1: where = a.out`a, address ="],
+        )
+        # Break at a() should fail
+        (target, process, thread, bkpt) = lldbutil.run_to_name_breakpoint(
+            self, "a", bkpt_module=exe
+        )
+        error_strings = [
+            '"a.o" object from the "',
+            "libfoo.a\" archive: either the .o file doesn't exist in the archive or the modification time (0x",
+            ") of the .o file doesn't match",
+        ]
+        self.check_frame_variable_errors(thread, error_strings)
+
+        # Break at b() should succeed
+        (target, process, thread, bkpt) = lldbutil.run_to_name_breakpoint(
+            self, "b", bkpt_module=exe
+        )
+        self.expect(
+            "thread list",
+            STOPPED_DUE_TO_BREAKPOINT,
+            substrs=["stopped", "stop reason = breakpoint"],
+        )
+        self.expect(
+            "frame variable", VARIABLES_DISPLAYED_CORRECTLY, substrs=["(int) arg = 2"]
+        )
+
+    @skipIfRemote
+    @skipUnlessDarwin
     def test_frame_var_errors_when_mtime_mistmatch_for_object_in_archive(self):
         """
         Break inside a() and modify the modification time for "a.o" within
         libfoo.a to make sure we can't load the debug information and
         report an appropriate error when doing 'frame variable'.
         """
         self.build()
         exe = self.getBuildArtifact("a.out")