dwarflint: Fix attribute and form validation in .debug_abbrev and .debug_info

- and a test case that used to SIGSEGV

8 files changed, 68 insertions, 56 deletions

diff --git a/dwarflint/check_debug_abbrev.cc b/dwarflint/check_debug_abbrev.cc
index 0d1786d7..39b5f316 100644
--- a/dwarflint/check_debug_abbrev.cc
+++ b/dwarflint/check_debug_abbrev.cc
@@ -27,6 +27,13 @@
 # include <config.h>
 #endif
 
+#include <dwarf.h>
+#include <sstream>
+#include <cassert>
+#include <algorithm>
+
+#include "../libdw/c++/dwarf"
+
 #include "check_debug_info.hh"
 #include "check_debug_abbrev.hh"
 #include "pri.hh"
@@ -36,11 +43,6 @@
 #include "messages.hh"
 #include "misc.hh"
 
-#include <dwarf.h>
-#include <sstream>
-#include <cassert>
-#include <algorithm>
-
 checkdescriptor const *
 check_debug_abbrev::descriptor ()
 {
@@ -112,13 +114,14 @@ namespace
 
   void
   complain (where const *where,
-	    attribute const *attribute, form const *form,
+	    int attrib_name, int form_name,
 	    bool indirect, char const *qualifier)
   {
     wr_error (*where)
-      << "attribute " << *attribute << " with " << qualifier
-      << (indirect ? " indirect" : "") << " form "
-      << *form << '.' << std::endl;
+      << "attribute " << elfutils::dwarf::attributes::name (attrib_name)
+      << " with " << qualifier << (indirect ? " indirect" : "")
+      << " form " << elfutils::dwarf::forms::name (form_name)
+      << '.' << std::endl;
   }
 
   bool
@@ -369,7 +372,6 @@ namespace
 
 	    /* Now if both are zero, this was the last attribute.  */
 	    null_attrib = attrib_name == 0 && attrib_form == 0;
-	    attribute const *attribute = NULL;
 
 	    REALLOC (cur, attribs);
 
@@ -392,17 +394,25 @@ namespace
 		continue;
 	      }
 
-	    attribute = ver->get_attribute (attrib_name);
+	    attribute const *attribute = ver->get_attribute (attrib_name);
 	    if (attribute == NULL)
 	      {
 		wr_error (where)
 		  << "invalid or unknown name " << pri::hex (attrib_name)
 		  << '.' << std::endl;
 		// libdw should handle unknown attribute, as long as
-		// the form is kosher.
-		continue;
+		// the form is kosher, so don't fail the check.
 	      }
 
+	    form const *form = check_debug_abbrev::check_form
+	      (ver, attribute, attrib_form, &where, false);
+	    if (form == NULL)
+	      // Error message has been emitted in check_form.
+	      failed = true;
+
+	    if (form == NULL || attribute == NULL)
+	      continue;
+
 	    std::pair<std::map<unsigned, uint64_t>::iterator, bool> inserted
 	      = seen.insert (std::make_pair (attrib_name, attr_off));
 	    if (!inserted.second)
@@ -418,15 +428,6 @@ namespace
 		  failed = true;
 	      }
 
-	    form const *form = check_debug_abbrev::check_form
-	      (ver, attrib_form, attribute, &where, false);
-	    if (form == NULL)
-	      {
-		// Error message is emitted in check_form.
-		failed = true;
-		continue;
-	      }
-
 	    if (attrib_name == DW_AT_sibling)
 	      {
 		if (!cur->has_children)
@@ -483,9 +484,10 @@ check_debug_abbrev::check_debug_abbrev (checkstack &stack, dwarflint &lint)
 }
 
 form const *
-check_debug_abbrev::check_form (dwarf_version const *ver, int form_name,
-				attribute const *attribute, where const *where,
-				bool indirect)
+check_debug_abbrev::check_form (dwarf_version const *ver,
+				attribute const *attribute,
+				int form_name,
+				where const *where, bool indirect)
 {
   form const *form = ver->get_form (form_name);
   if (form == NULL)
@@ -496,14 +498,19 @@ check_debug_abbrev::check_form (dwarf_version const *ver, int form_name,
       return NULL;
     }
 
-  if (!ver->form_allowed (attribute->name (), form_name))
+  if (attribute != NULL)
     {
-      complain (where, attribute, form, indirect, "invalid");
-      return NULL;
+
+      int attrib_name = attribute->name ();
+      if (!ver->form_allowed (attribute, form))
+	{
+	  complain (where, attrib_name, form_name, indirect, "invalid");
+	  return NULL;
+	}
+      else if (attrib_name == DW_AT_sibling
+	       && sibling_form_suitable (ver, form) == sfs_long)
+	complain (where, attrib_name, form_name, indirect, "unsuitable");
     }
-  else if (attribute->name () == DW_AT_sibling
-	   && sibling_form_suitable (ver, form_name) == sfs_long)
-    complain (where, attribute, form, indirect, "unsuitable");
 
   return form;
 }
diff --git a/dwarflint/check_debug_abbrev.hh b/dwarflint/check_debug_abbrev.hh
index bdc7f8c6..28231647 100644
--- a/dwarflint/check_debug_abbrev.hh
+++ b/dwarflint/check_debug_abbrev.hh
@@ -85,8 +85,8 @@ public:
 
   check_debug_abbrev (checkstack &stack, dwarflint &lint);
   static form const *check_form (dwarf_version const *ver,
+				 attribute const *attr,
 				 int form_name,
-				 attribute const *attribute,
 				 where const *where,
 				 bool indirect);
 
diff --git a/dwarflint/check_debug_info.cc b/dwarflint/check_debug_info.cc
index 45af96b8..b712daab 100644
--- a/dwarflint/check_debug_info.cc
+++ b/dwarflint/check_debug_info.cc
@@ -652,9 +652,13 @@ namespace
 	     it->name != 0 || it->form != 0; ++it)
 	  {
 	    where.ref = &it->where;
+	    int form_name = it->form;
 
+	    // In following, attribute may be NULL, but form never
+	    // should.  We always need to know the form to be able to
+	    // read .debug_info, so we fail in check_debug_abbrev if
+	    // it's invalid or unknown.
 	    attribute const *attribute = ver->get_attribute (it->name);
-	    int form_name = it->form;
 	    form const *form = ver->get_form (form_name);
 	    if (attribute != NULL
 		&& ver->form_class (form, attribute) == cl_indirect)
@@ -665,10 +669,12 @@ namespace
 		  return -1;
 		form_name = value;
 		form = check_debug_abbrev::check_form
-		  (ver, form_name, attribute, &where, true);
+		  (ver, attribute, form_name, &where, true);
+		// N.B. check_form emits appropriate error messages.
 		if (form == NULL)
 		  return -1;
 	      }
+	    assert (form != NULL);
 
 	    dw_class cls = attribute != NULL
 	      ? ver->form_class (form, attribute)
@@ -715,7 +721,7 @@ namespace
 	      (cl_reference, cl_loclistptr, cl_lineptr, cl_macptr,
 	       cl_rangelistptr);
 
-	    if (cls != max_dw_class && ref_classes.test (cls))
+	    if (form != NULL && cls != max_dw_class && ref_classes.test (cls))
 	      {
 		form_bitness_t bitness = form->bitness ();
 		if ((bitness == fb_32 && cu->head->offset_size == 8)
diff --git a/dwarflint/dwarf_gnu.cc b/dwarflint/dwarf_gnu.cc
index df3e85b1..6335abb4 100644
--- a/dwarflint/dwarf_gnu.cc
+++ b/dwarflint/dwarf_gnu.cc
@@ -80,15 +80,12 @@ namespace
     {}
 
     virtual bool
-    form_allowed (int attribute_name, int form_name) const
+    form_allowed (attribute const *attr, form const *form) const
     {
-      if (attribute_name == DW_AT_GNU_odr_signature)
-	{
-	  form const *f = get_form (form_name);
-	  return f->classes ()[cl_constant] && f->width (NULL) == fw_8;
-	}
+      if (attr->name () == DW_AT_GNU_odr_signature)
+	return form->classes ()[cl_constant] && form->width (NULL) == fw_8;
       else
-	return std_dwarf::form_allowed (attribute_name, form_name);
+	return std_dwarf::form_allowed (attr, form);
     }
   };
 }
diff --git a/dwarflint/dwarf_version.cc b/dwarflint/dwarf_version.cc
index 14e7961a..fc4595a6 100644
--- a/dwarflint/dwarf_version.cc
+++ b/dwarflint/dwarf_version.cc
@@ -150,25 +150,19 @@ dwarf_version::form_allowed (int form) const
 }
 
 bool
-dwarf_version::form_allowed (int attribute_name, int form_name) const
+dwarf_version::form_allowed (attribute const *attr, form const *form) const
 {
-  attribute const *attribute = this->get_attribute (attribute_name);
-  assert (attribute != NULL);
-  dw_class_set const &attr_classes = attribute->classes ();
-
-  form const *form = this->get_form (form_name);
-  assert (form != NULL);
+  dw_class_set const &attr_classes = attr->classes ();
   dw_class_set const &form_classes = form->classes ();
-
   return (attr_classes & form_classes).any ();
 }
 
 sibling_form_suitable_t
-sibling_form_suitable (dwarf_version const *ver, int form)
+sibling_form_suitable (dwarf_version const *ver, form const *form)
 {
-  if (!ver->form_allowed (DW_AT_sibling, form))
+  if (!ver->form_allowed (ver->get_attribute (DW_AT_sibling), form))
     return sfs_invalid;
-  else if (form == DW_FORM_ref_addr)
+  else if (form->name () == DW_FORM_ref_addr)
     return sfs_long;
   else
     return sfs_ok;
diff --git a/dwarflint/dwarf_version.hh b/dwarflint/dwarf_version.hh
index f7a1df5d..43df29e9 100644
--- a/dwarflint/dwarf_version.hh
+++ b/dwarflint/dwarf_version.hh
@@ -202,7 +202,8 @@ public:
 
   /// Figure out whether, in given DWARF version, given attribute is
   /// allowed to have given form.
-  virtual bool form_allowed (int attribute_name, int form_name) const;
+  virtual bool form_allowed (attribute const *attr, form const *form) const
+    __attribute__ ((nonnull (1, 2)));
 
   /// Answer a class of FORM given ATTRIBUTE as a context.  If there's
   /// exactly one candidate class, that's the one answered.  If
@@ -238,6 +239,7 @@ enum sibling_form_suitable_t
     sfs_invalid, ///< This form isn't allowed at DW_AT_sibling
   };
 sibling_form_suitable_t sibling_form_suitable (dwarf_version const *ver,
-					       int form);
+					       form const *form)
+  __attribute__ ((nonnull (1, 2)));
 
 #endif//DWARFLINT_DWARF_VERSION_HH
diff --git a/dwarflint/tests/garbage-7.bz2 b/dwarflint/tests/garbage-7.bz2
new file mode 100644
index 00000000..41963b62
--- /dev/null
+++ b/dwarflint/tests/garbage-7.bz2
diff --git a/dwarflint/tests/run-bad.sh b/dwarflint/tests/run-bad.sh
index 108f527b..423e1f41 100755
--- a/dwarflint/tests/run-bad.sh
+++ b/dwarflint/tests/run-bad.sh
@@ -28,7 +28,7 @@
 srcdir=$srcdir/tests
 
 testfiles hello.bad-1 hello.bad-3 garbage-1 garbage-2 garbage-3 garbage-4 \
-    garbage-5 garbage-6
+    garbage-5 garbage-6 garbage-7
 
 testrun_compare ./dwarflint hello.bad-1 <<EOF
 error: .debug_info: DIE 0x83: abbrev section at 0x0 doesn't contain code 83.
@@ -72,3 +72,9 @@ error: .debug_abbrev: abbr. attribute 0xc: attribute stmt_list with invalid form
 error: .debug_abbrev: abbr. attribute 0x23: attribute frame_base with invalid form block1.
 error: .debug_abbrev: abbr. attribute 0x34: attribute location with invalid form block1.
 EOF
+
+testrun_compare ./dwarflint garbage-7 <<EOF
+error: .debug_abbrev: abbr. attribute 0x7e: invalid or unknown name 0x703.
+error: .debug_abbrev: abbr. attribute 0x7e: invalid form 0x0.
+error: .debug_abbrev: abbreviation 122: missing zero to mark end-of-table.
+EOF