From a48501150b9ef64fd61d24f8cef2645237facc44 Mon Sep 17 00:00:00 2001 From: Walter Erquinigo Date: Tue, 23 Nov 2021 09:32:30 -0800 Subject: [PATCH] Make some libstd++ formatters safer 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 | 90 +++++++++++-------- .../Language/CPlusPlus/LibCxxUnorderedMap.cpp | 13 +-- .../bitset/TestDataFormatterGenericBitset.py | 4 +- .../TestDataFormatterGenericForwardList.py | 3 +- .../TestDataFormatterGenericMultiMap.py | 18 +++- .../generic/multimap/main.cpp | 28 +++--- .../TestDataFormatterGenericUnordered.py | 15 ++-- .../generic/unordered/main.cpp | 6 +- 8 files changed, 105 insertions(+), 72 deletions(-) diff --git a/lldb/examples/synthetic/gnu_libstdcpp.py b/lldb/examples/synthetic/gnu_libstdcpp.py index efeb4fe29546..e2abc452bebd 100644 --- a/lldb/examples/synthetic/gnu_libstdcpp.py +++ b/lldb/examples/synthetic/gnu_libstdcpp.py @@ -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') + 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() - map_type = self.valobj.GetType() - if map_type.IsReferenceType(): - logger >> "Dereferencing type" - map_type = map_type.GetDereferencedType() + # Get the type of std::pair. 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) - # Get the type of std::pair. 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() + # 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): diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp index 3a441973fc73..57c5ba87c397 100644 --- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp +++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp @@ -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(); diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py index 7dac8ec06c0b..3d7ff340caab 100644 --- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py +++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py @@ -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""" diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py index d54aedd4960b..28a01f17c684 100644 --- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py +++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/forward_list/TestDataFormatterGenericForwardList.py @@ -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) - diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py index b90964951fee..84be87ae478e 100644 --- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py +++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/TestDataFormatterGenericMultiMap.py @@ -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) - diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/main.cpp index 27bdc0a57729..a30daa7a5bfe 100644 --- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/main.cpp +++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multimap/main.cpp @@ -1,10 +1,10 @@ #include #include -#define intint_map std::multimap -#define strint_map std::multimap -#define intstr_map std::multimap -#define strstr_map std::multimap +#define intint_map std::multimap +#define strint_map std::multimap +#define intstr_map std::multimap +#define strstr_map std::multimap 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; } diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py index 664ff6921129..646836696ff0 100644 --- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py +++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py @@ -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) diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/main.cpp index 36ebefcd747b..00d37dcb4bd0 100644 --- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/main.cpp +++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered/main.cpp @@ -14,7 +14,11 @@ int thefoo_rw(int arg = 1) { } int main() { - std::unordered_map map; + + char buffer[sizeof(std::unordered_map)] = {0}; + std::unordered_map &corrupt_map = *(std::unordered_map *)buffer; + + std::unordered_map map; // Set break point at this line. map.emplace(1, "hello"); map.emplace(2, "world"); map.emplace(3, "this");