Bug 19139 - DWARF reader doesn't handle garbage in function names

In this bug, the DWARF debug info of the binary (generated by Intel's
ICC compiler) has interesting constructs like:

     [ 6b5a0]    subprogram
		 decl_line            (data2) 787
		 decl_column          (data1) 15
		 decl_file            (data1) 46
		 declaration          (flag)
		 accessibility        (data1) public (1)
		 type                 (ref4) [ 6b56a]
		 prototyped           (flag)
		 name                 (string) "ldiv"
		 MIPS_linkage_name    (string) "ldiv"
     [ 6b5b6]      formal_parameter
		   type                 (ref4) [ 5f2aa]
		   name                 (string) "$Ë2"
     [ 6b5bf]      formal_parameter
		   type                 (ref4) [ 5f2aa]
		   name                 (string) "$Ë3"

Note the strings that make up the name of the formal parameters of the
function, near the end:

     [ 6b5b6]      formal_parameter
		   type                 (ref4) [ 5f2aa]
		   name                 (string) "$Ë2"
     [ 6b5bf]      formal_parameter
		   type                 (ref4) [ 5f2aa]
		   name                 (string) "$Ë3"

The strings "$Ë2" and $Ë3" (which are the names of the
parameters of the function) are garbage.

Libabigail's DWARF reader naively uses those strings as names for the
function parameters, in the type of the function.

Then, the abixml writer emits an XML document, with these strings as
property values, representing the name of the type of the function.

And of course, the XML later chokes when it tries to read that XML
document, saying that the property is not valid UTF-8.

This patch addresses the issue by dropping those garbage names on the
floor, for function type names.  In that context, any string that is
not made of ASCII characters is considered as being garbage, for now.

The patch, in the abixml writer, also escapes function parameters
names so that they don't contain characters that are not allowed in
XML.  The abixml reader already handles the un-escaping of the names
it reads, so I think there is nothing to do there.

Ultimately, I guess I should get the unicode value of the characters
of that string, encode the string into UTF-8 and use the result as the
name for the parameter.  That would mean using UTF-8 strings for
function parameter names, and, for all declarations names.  But that
is too much for worfk too little gain for now.  The great majority of
the binaries we are dealing with are still using ASCII for declaration
names.

The patch also introduces a new test harness that runs "abidw
--abidiff" on a bunch of input binaries.  This harness runs over the
binaries that were submitted in this bug report.

	* include/abg-tools-utils.h (string_is_ascii): Declare new
	function ...
	* src/abg-tools-utils.cc (string_is_ascii): ... and define it.
	* src/abg-writer.cc (write_function_type): Escape forbidden XML
	characters in function type names.
	* src/abg-dwarf-reader.cc (build_function_type):  If a parameter
	name is not ascii, drop it on the floor.
	* tests/data/test-types-stability/pr19139-DomainNeighborMapInst.o:
	New test input binary.
	* tests/data/test-types-stability/pr19202-libmpi_gpfs.so.5.0:
	Likewise.
	* tests/data/Makefile.am: Add the new binaries above to the build
	system.
	* tests/test-types-stability.cc: New test harness.
	* tests/Makefile.am: Add the new test harness to the build system.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

diff --git a/include/abg-tools-utils.h b/include/abg-tools-utils.h
index 412c38f..68b1525 100644 (file)
--- a/include/abg-tools-utils.h
+++ b/include/abg-tools-utils.h
@@ -49,6 +49,7 @@ bool ensure_dir_path_created(const string&);
 bool ensure_parent_dir_created(const string&);
 bool check_file(const string& path, ostream& out);
 bool string_ends_with(const string&, const string&);
+bool string_is_ascii(const string&);
 
 class temp_file;
 

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index be2dde7..b5888b2 100644 (file)
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -46,6 +46,7 @@
 #include <sstream>
 #include "abg-dwarf-reader.h"
 #include "abg-sptr-utils.h"
+#include "abg-tools-utils.h"
 
 using std::string;
 
@@ -7150,6 +7151,10 @@ build_function_type(read_context&        ctxt,
            string name, linkage_name;
            location loc;
            die_loc_and_name(ctxt, &child, loc, name, linkage_name);
+           if (!tools_utils::string_is_ascii(name))
+             // Sometimes, bogus compiler emit names that are
+             // non-ascii garbage.  Let's just ditch that for now.
+             name.clear();
            bool is_artificial = die_is_artificial(&child);
            decl_base_sptr parm_type_decl;
            Dwarf_Die parm_type_die;

diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index c478345..5318a88 100644 (file)
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -26,6 +26,7 @@
 #include <time.h>
 #include <cstdlib>
 #include <cstring>
+#include <ctype.h>
 #include <libgen.h>
 #include <ext/stdio_filebuf.h> // For __gnu_cxx::stdio_filebuf
 #include <fstream>
@@ -331,6 +332,21 @@ string_ends_with(const string& str, const string& suffix)
                     suffix) == 0;
 }
 
+/// Test if a string is made of ascii characters.
+///
+/// @param str the string to consider.
+///
+/// @return true iff @p str is made of ascii characters.
+bool
+string_is_ascii(const string& str)
+{
+  for (string::const_iterator i = str.begin(); i != str.end(); ++i)
+    if (!isascii(*i))
+      return false;
+
+  return true;
+}
+
 /// The private data of the @ref temp_file type.
 struct temp_file::priv
 {

diff --git a/src/abg-writer.cc b/src/abg-writer.cc
index e06547b..3939968 100644 (file)
--- a/src/abg-writer.cc
+++ b/src/abg-writer.cc
@@ -2247,7 +2247,10 @@ write_function_type(const shared_ptr<function_type> decl, write_context& ctxt,
          ctxt.record_type_as_referenced(parm_type);
 
          if (!(*pi)->get_name().empty())
-           o << " name='" << (*pi)->get_name() << "'";
+           {
+             string name = xml::escape_xml_string((*pi)->get_name());
+             o << " name='" << name << "'";
+           }
        }
       if ((*pi)->get_artificial())
        o << " is-artificial='yes'";

diff --git a/tests/Makefile.am b/tests/Makefile.am
index cf8f2af..b7c5011 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -28,6 +28,7 @@ runtestdifffilter             \
 runtestdiffsuppr               \
 runtestabicompat               \
 runtestdiffpkg                 \
+runtesttypesstability          \
 $(CXX11_TESTS)
 
 EXTRA_DIST = runtestcanonicalizetypes.sh.in
@@ -84,6 +85,9 @@ runtestabicompat_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
 runtestdiffpkg_SOURCES = test-diff-pkg.cc
 runtestdiffpkg_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
 
+runtesttypesstability_SOURCES = test-types-stability.cc
+runtesttypesstability_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
+
 runtestsvg_SOURCES=test-svg.cc
 runtestsvg_LDADD=$(top_builddir)/src/libabigail.la
 

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 0b0e2ad..647457d 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -304,6 +304,8 @@ test-read-dwarf/test21-pr19092.so \
 test-read-dwarf/test21-pr19092.so.abi \
 test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so \
 test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi \
+test-types-stability/pr19139-DomainNeighborMapInst.o \
+test-types-stability/pr19202-libmpi_gpfs.so.5.0 \
 \
 test-diff-filter/test0-v0.cc           \
 test-diff-filter/test0-v1.cc           \

diff --git a/tests/data/test-types-stability/pr19139-DomainNeighborMapInst.o b/tests/data/test-types-stability/pr19139-DomainNeighborMapInst.o
new file mode 100644 (file)
index 0000000..19abc73
Binary files /dev/null and b/tests/data/test-types-stability/pr19139-DomainNeighborMapInst.o differ

diff --git a/tests/data/test-types-stability/pr19202-libmpi_gpfs.so.5.0 b/tests/data/test-types-stability/pr19202-libmpi_gpfs.so.5.0
new file mode 100644 (file)
index 0000000..e48b0ff
Binary files /dev/null and b/tests/data/test-types-stability/pr19202-libmpi_gpfs.so.5.0 differ

diff --git a/tests/test-types-stability.cc b/tests/test-types-stability.cc
new file mode 100644 (file)
index 0000000..3e515cc
--- /dev/null
+++ b/tests/test-types-stability.cc
@@ -0,0 +1,84 @@
+// -*- Mode: C++ -*-
+//
+// Copyright (C) 2013-2015 Red Hat, Inc.
+//
+// This file is part of the GNU Application Binary Interface Generic
+// Analysis and Instrumentation Library (libabigail).  This library is
+// free software; you can redistribute it and/or modify it under the
+// terms of the GNU Lesser General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option) any
+// later version.
+
+// This library is distributed in the hope that it will be useful, but
+// WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+// General Lesser Public License for more details.
+
+// You should have received a copy of the GNU Lesser General Public
+// License along with this program; see the file COPYING-LGPLV3.  If
+// not, see <http://www.gnu.org/licenses/>.
+
+// Author: Dodji Seketeli
+
+/// @file
+///
+/// This program tests that the representation of types by the
+/// internal representation of libabigail is stable through reading
+/// from ELF/DWARF, constructing an internal represenation, saving that
+/// internal presentation to the abixml format, reading from that
+/// abixml format and constructing an internal representation from it
+/// again.
+///
+/// This program thus compares the internal representation that is
+/// built from reading from ELF/DWARF and the one that is built from
+/// the abixml (which itself results from the serialization of the
+/// first internal representation to abixml).
+///
+/// The comparison is expected to yield the empty set.
+
+
+#include <string>
+#include <fstream>
+#include <iostream>
+#include <cstdlib>
+#include "abg-tools-utils.h"
+#include "test-utils.h"
+#include "abg-dwarf-reader.h"
+#include "abg-comparison.h"
+
+using std::string;
+using std::ofstream;
+using std::cerr;
+
+// A set of elf files to test type stability for.
+const char* elf_paths[] =
+{
+  "data/test-types-stability/pr19139-DomainNeighborMapInst.o",
+  "data/test-types-stability/pr19202-libmpi_gpfs.so.5.0",
+  // The below should always be the last element of array.
+  0
+};
+
+int
+main()
+{
+  using abigail::tests::get_src_dir;
+  using abigail::tests::get_build_dir;
+
+  bool is_ok = true;
+
+  for (const char** p = elf_paths; p && *p; ++p)
+    {
+      string abidw = get_build_dir() + "/tools/abidw";
+      string elf_path = get_src_dir() + "/tests/" + *p;
+      string cmd = abidw + " --abidiff " + elf_path;
+      if (system(cmd.c_str()))
+       {
+         cerr << "IR stability issue detected for binary "
+              << elf_path;
+         is_ok = false;
+       }
+    }
+
+  return !is_ok;
+}