Make some libstd++ formatters safer
authorWalter Erquinigo <wallace@fb.com>
Tue, 23 Nov 2021 17:32:30 +0000 (09:32 -0800)
committerWalter Erquinigo <wallace@fb.com>
Tue, 23 Nov 2021 21:52:32 +0000 (13:52 -0800)
We need to add checks that ensure that some core variables are valid, so
that we avoid printing out garbage data. The worst that could happen is
that an non-initialized variable is being printed as something with
123123432 children instead of 0.

Differential Revision: https://reviews.llvm.org/D114458

lldb/examples/synthetic/gnu_libstdcpp.py
lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/main.cpp
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/main.cpp

index efeb4fe..e2abc45 100644 (file)
@@ -2,10 +2,9 @@ from __future__ import division
 import lldb.formatters.Logger
 
 # C++ STL formatters for LLDB
-# These formatters are based upon the version of the GNU libstdc++
-# as it ships with Mac OS X 10.6.8 thru 10.8.0
-# You are encouraged to look at the STL implementation for your platform
-# before relying on these formatters to do the right thing for your setup
+# As there are many versions of the libcstd++, you are encouraged to look at the STL
+# implementation for your platform before relying on these formatters to do the right
+# thing for your setup
 
 def StdOptionalSummaryProvider(valobj, dict):
     has_value = valobj.GetNumChildren() > 0
@@ -76,8 +75,10 @@ class StdUnorderedMapSynthProvider:
             self.data_type = self.extract_type()
             self.skip_size = self.next.GetType().GetByteSize()
             self.data_size = self.data_type.GetByteSize()
+            if (not self.data_type.IsValid()) or (not self.next.IsValid()):
+                self.count = 0
         except:
-            pass
+            self.count = 0
         return False
 
     def get_child_index(self, name):
@@ -208,8 +209,11 @@ class AbstractListSynthProvider:
             current = self.next
             while current.GetChildMemberWithName(
                     '_M_next').GetValueAsUnsigned(0) != self.get_end_of_list_address():
-                size = size + 1
                 current = current.GetChildMemberWithName('_M_next')
+                if not current.IsValid():
+                    break
+                size = size + 1
+
             return size
         except:
             logger >> "Error determining the size"
@@ -250,10 +254,8 @@ class AbstractListSynthProvider:
         if list_type.IsReferenceType():
             list_type = list_type.GetDereferencedType()
         if list_type.GetNumberOfTemplateArguments() > 0:
-            data_type = list_type.GetTemplateArgumentType(0)
-        else:
-            data_type = None
-        return data_type
+            return list_type.GetTemplateArgumentType(0)
+        return lldb.SBType()
 
     def update(self):
         logger = lldb.formatters.Logger.Logger()
@@ -263,15 +265,20 @@ class AbstractListSynthProvider:
         try:
             self.impl = self.valobj.GetChildMemberWithName('_M_impl')
             self.data_type = self.extract_type()
-            self.data_size = self.data_type.GetByteSize()
-            self.updateNodes()
+            if (not self.data_type.IsValid()) or (not self.impl.IsValid()):
+                self.count = 0
+            elif not self.updateNodes():
+                self.count = 0
+            else:
+                self.data_size = self.data_type.GetByteSize()
         except:
-            pass
+            self.count = 0
         return False
 
     '''
     Method is used to extract the list pointers into the variables (e.g self.node, self.next, and optionally to self.prev)
-    and is mandatory to be overriden in each AbstractListSynthProvider subclass
+    and is mandatory to be overriden in each AbstractListSynthProvider subclass.
+    This should return True or False depending on wheter it found valid data.
     '''
     def updateNodes(self):
         raise NotImplementedError
@@ -296,6 +303,9 @@ class StdForwardListSynthProvider(AbstractListSynthProvider):
     def updateNodes(self):
         self.node = self.impl.GetChildMemberWithName('_M_head')
         self.next = self.node.GetChildMemberWithName('_M_next')
+        if (not self.node.IsValid()) or (not self.next.IsValid()):
+            return False
+        return True
 
     def get_end_of_list_address(self):
         return 0
@@ -312,6 +322,9 @@ class StdListSynthProvider(AbstractListSynthProvider):
         self.node = self.impl.GetChildMemberWithName('_M_node')
         self.prev = self.node.GetChildMemberWithName('_M_prev')
         self.next = self.node.GetChildMemberWithName('_M_next')
+        if self.node_address == 0 or (not self.node.IsValid()) or (not self.next.IsValid()) or (not self.prev.IsValid()):
+            return False
+        return True
 
     def get_end_of_list_address(self):
         return self.node_address
@@ -398,7 +411,7 @@ class StdVectorSynthProvider:
                 else:
                     self.count = 0
             except:
-                pass
+                self.count = 0
             return False
 
     class StdVBoolImplementation(object):
@@ -443,7 +456,10 @@ class StdVectorSynthProvider:
                 self.start_p = self.m_start.GetChildMemberWithName('_M_p')
                 self.finish_p = self.m_finish.GetChildMemberWithName('_M_p')
                 self.offset = self.m_finish.GetChildMemberWithName('_M_offset')
-                self.valid = True
+                if self.offset.IsValid() and self.start_p.IsValid() and self.finish_p.IsValid():
+                    self.valid = True
+                else:
+                    self.valid = False
             except:
                 self.valid = False
             return False
@@ -530,29 +546,31 @@ class StdMapLikeSynthProvider:
             self.Mt = self.valobj.GetChildMemberWithName('_M_t')
             self.Mimpl = self.Mt.GetChildMemberWithName('_M_impl')
             self.Mheader = self.Mimpl.GetChildMemberWithName('_M_header')
-
-            map_type = self.valobj.GetType()
-            if map_type.IsReferenceType():
-                logger >> "Dereferencing type"
-                map_type = map_type.GetDereferencedType()
-
-            # Get the type of std::pair<key, value>. It is the first template
-            # argument type of the 4th template argument to std::map.
-            allocator_type = map_type.GetTemplateArgumentType(3)
-            self.data_type = allocator_type.GetTemplateArgumentType(0)
-            if not self.data_type:
-                # GCC does not emit DW_TAG_template_type_parameter for
-                # std::allocator<...>. For such a case, get the type of
-                # std::pair from a member of std::map.
-                rep_type = self.valobj.GetChildMemberWithName('_M_t').GetType()
-                self.data_type = rep_type.GetTypedefedType().GetTemplateArgumentType(1)
-
-            # from libstdc++ implementation of _M_root for rbtree
-            self.Mroot = self.Mheader.GetChildMemberWithName('_M_parent')
-            self.data_size = self.data_type.GetByteSize()
-            self.skip_size = self.Mheader.GetType().GetByteSize()
+            if not self.Mheader.IsValid():
+                self.count = 0
+            else:
+                map_type = self.valobj.GetType()
+                if map_type.IsReferenceType():
+                    logger >> "Dereferencing type"
+                    map_type = map_type.GetDereferencedType()
+
+                # Get the type of std::pair<key, value>. It is the first template
+                # argument type of the 4th template argument to std::map.
+                allocator_type = map_type.GetTemplateArgumentType(3)
+                self.data_type = allocator_type.GetTemplateArgumentType(0)
+                if not self.data_type:
+                    # GCC does not emit DW_TAG_template_type_parameter for
+                    # std::allocator<...>. For such a case, get the type of
+                    # std::pair from a member of std::map.
+                    rep_type = self.valobj.GetChildMemberWithName('_M_t').GetType()
+                    self.data_type = rep_type.GetTypedefedType().GetTemplateArgumentType(1)
+
+                # from libstdc++ implementation of _M_root for rbtree
+                self.Mroot = self.Mheader.GetChildMemberWithName('_M_parent')
+                self.data_size = self.data_type.GetByteSize()
+                self.skip_size = self.Mheader.GetType().GetByteSize()
         except:
-            pass
+            self.count = 0
         return False
 
     def num_children(self):
index 3a44197..57c5ba8 100644 (file)
@@ -62,9 +62,7 @@ lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
 
 size_t lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
     CalculateNumChildren() {
-  if (m_num_elements != UINT32_MAX)
-    return m_num_elements;
-  return 0;
+  return m_num_elements;
 }
 
 lldb::ValueObjectSP lldb_private::formatters::
@@ -160,7 +158,7 @@ lldb::ValueObjectSP lldb_private::formatters::
 
 bool lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
     Update() {
-  m_num_elements = UINT32_MAX;
+  m_num_elements = 0;
   m_next_element = nullptr;
   m_elements_cache.clear();
   ValueObjectSP table_sp =
@@ -195,8 +193,13 @@ bool lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
 
   if (!num_elements_sp)
     return false;
-  m_num_elements = num_elements_sp->GetValueAsUnsigned(0);
+
   m_tree = table_sp->GetChildAtNamePath(next_path).get();
+  if (m_tree == nullptr)
+    return false;
+
+  m_num_elements = num_elements_sp->GetValueAsUnsigned(0);
+
   if (m_num_elements > 0)
     m_next_element =
         table_sp->GetChildAtNamePath(next_path).get();
index 7dac8ec..3d7ff34 100644 (file)
@@ -27,7 +27,7 @@ class GenericBitsetDataFormatterTestCase(TestBase):
             for j in range(2*i, len(primes), i):
                 primes[j] = 0
         self.primes = primes
-    
+
     def getBitsetVariant(self, size, variant):
         if variant == VALUE:
             return "std::bitset<" + str(size) + ">"
@@ -47,7 +47,7 @@ class GenericBitsetDataFormatterTestCase(TestBase):
             children.append(ValueCheck(value=str(bool(child.GetValueAsUnsigned())).lower()))
             self.assertEqual(child.GetValueAsUnsigned(), self.primes[i],
                     "variable: %s, index: %d"%(name, size))
-        self.expect_var_path(name,type=self.getBitsetVariant(size,variant),children=children) 
+        self.expect_var_path(name,type=self.getBitsetVariant(size,variant),children=children)
 
     def do_test_value(self, stdlib_type):
         """Test that std::bitset is displayed correctly"""
index d54aedd..28a01f1 100644 (file)
@@ -21,7 +21,7 @@ class TestDataFormatterGenericForwardList(TestBase):
         self.line = line_number('main.cpp', '// break here')
         self.namespace = 'std'
 
-    
+
     def do_test(self, stdlib_type):
         """Test that std::forward_list is displayed correctly"""
         self.build(dictionary={stdlib_type: "1"})
@@ -59,4 +59,3 @@ class TestDataFormatterGenericForwardList(TestBase):
     @add_test_categories(["libc++"])
     def test_libcpp(self):
          self.do_test(USE_LIBCPP)
-    
index b909649..84be87a 100644 (file)
@@ -20,7 +20,7 @@ class GenericMultiMapDataFormatterTestCase(TestBase):
     def setUp(self):
         TestBase.setUp(self)
         self.namespace = 'std'
-    
+
     def findVariable(self, name):
         var = self.frame().FindVariable(name)
         self.assertTrue(var.IsValid())
@@ -70,6 +70,17 @@ class GenericMultiMapDataFormatterTestCase(TestBase):
         self.addTearDownHook(cleanup)
 
         multimap = self.namespace + "::multimap"
+
+        self.expect('frame variable ii',
+                    substrs=[multimap, 'size=0',
+                             '{}'])
+
+        self.expect('frame variable si',
+                    substrs=[multimap, 'size=0',
+                             '{}'])
+
+        lldbutil.continue_to_breakpoint(self.process(), bkpt)
+
         self.expect('frame variable ii',
                     substrs=[multimap, 'size=0',
                              '{}'])
@@ -84,7 +95,7 @@ class GenericMultiMapDataFormatterTestCase(TestBase):
                 '[0] = (first = 0, second = 0)',
                 '[1] = (first = 1, second = 1)',
             ])
-        
+
         self.check("ii", 2)
 
         lldbutil.continue_to_breakpoint(self.process(), bkpt)
@@ -97,7 +108,7 @@ class GenericMultiMapDataFormatterTestCase(TestBase):
                              '[3] = ',
                              'first = 3',
                              'second = 1'])
-        
+
         self.check("ii", 4)
 
         lldbutil.continue_to_breakpoint(self.process(), bkpt)
@@ -326,4 +337,3 @@ class GenericMultiMapDataFormatterTestCase(TestBase):
     @add_test_categories(["libc++"])
     def test_with_run_command_libcpp(self):
         self.do_test_with_run_command(USE_LIBCPP)
-    
index 27bdc0a..a30daa7 100644 (file)
@@ -1,10 +1,10 @@
 #include <string>
 #include <map>
 
-#define intint_map std::multimap<int, int> 
-#define strint_map std::multimap<std::string, int> 
-#define intstr_map std::multimap<int, std::string> 
-#define strstr_map std::multimap<std::string, std::string> 
+#define intint_map std::multimap<int, int>
+#define strint_map std::multimap<std::string, int>
+#define intstr_map std::multimap<int, std::string>
+#define strstr_map std::multimap<std::string, std::string>
 
 int g_the_foo = 0;
 
@@ -18,10 +18,10 @@ int thefoo_rw(int arg = 1)
        return g_the_foo;
 }
 
-int main()
+int main()  // Set break point at this line.
 {
     intint_map ii;
-    
+
     ii.emplace(0,0); // Set break point at this line.
     ii.emplace(1,1);
        thefoo_rw(1);  // Set break point at this line.
@@ -36,10 +36,10 @@ int main()
     ii.emplace(85,1234567);
 
     ii.clear();
-    
+
     strint_map si;
     thefoo_rw(1);  // Set break point at this line.
-       
+
     si.emplace("zero",0);
        thefoo_rw(1);  // Set break point at this line.
        si.emplace("one",1);
@@ -50,7 +50,7 @@ int main()
 
     si.clear();
     thefoo_rw(1);  // Set break point at this line.
-       
+
     intstr_map is;
     thefoo_rw(1);  // Set break point at this line.
     is.emplace(85,"goofy");
@@ -58,20 +58,20 @@ int main()
     is.emplace(2,"smart");
     is.emplace(3,"!!!");
     thefoo_rw(1);  // Set break point at this line.
-       
+
     is.clear();
     thefoo_rw(1);  // Set break point at this line.
-       
+
     strstr_map ss;
     thefoo_rw(1);  // Set break point at this line.
-       
+
     ss.emplace("ciao","hello");
     ss.emplace("casa","house");
     ss.emplace("gatto","cat");
     thefoo_rw(1);  // Set break point at this line.
     ss.emplace("a Mac..","..is always a Mac!");
-    
+
     ss.clear();
-    thefoo_rw(1);  // Set break point at this line.    
+    thefoo_rw(1);  // Set break point at this line.
     return 0;
 }
index 664ff69..6468366 100644 (file)
@@ -1,10 +1,3 @@
-"""
-Test lldb data formatter subsystem.
-"""
-
-
-
-import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
@@ -49,6 +42,12 @@ class GenericUnorderedDataFormatterTestCase(TestBase):
         self.addTearDownHook(cleanup)
 
         ns = self.namespace
+
+        # We check here that the map shows 0 children even with corrupt data.
+        self.look_for_content_and_continue(
+            "corrupt_map", ['%s::unordered_map' %
+                    ns, 'size=0 {}'])
+
         self.look_for_content_and_continue(
             "map", ['%s::unordered_map' %
                     ns, 'size=5 {', 'hello', 'world', 'this', 'is', 'me'])
@@ -84,4 +83,4 @@ class GenericUnorderedDataFormatterTestCase(TestBase):
 
     @add_test_categories(["libc++"])
     def test_with_run_command_libcpp(self):
-        self.do_test_with_run_command(USE_LIBCPP)
\ No newline at end of file
+        self.do_test_with_run_command(USE_LIBCPP)
index 36ebefc..00d37dc 100644 (file)
@@ -14,7 +14,11 @@ int thefoo_rw(int arg = 1) {
 }
 
 int main() {
-  std::unordered_map<int, std::string> map;
+
+  char buffer[sizeof(std::unordered_map<int, std::string>)] = {0};
+  std::unordered_map<int, std::string> &corrupt_map = *(std::unordered_map<int, std::string> *)buffer;
+
+  std::unordered_map<int, std::string> map; // Set break point at this line.
   map.emplace(1, "hello");
   map.emplace(2, "world");
   map.emplace(3, "this");