Hi Panu,

On Mon, 2018-03-12 at 14:07 +0200, Panu Matilainen wrote:
> Actually, this breaks a bunch of testcases:
> 
> 139: rpmbuild debuginfo subpackages multiple         FAILED 
> (rpmbuild.at:973)
> 140: rpmbuild debuginfo subpackages multiple unique  FAILED 
> (rpmbuild.at:1058)
> 141: rpmbuild debuginfo subpackages multiple unique debugsource FAILED 
> (rpmbuild.at:1143)
> 142: rpmbuild debuginfo subpackages multiple excluded FAILED 
> (rpmbuild.at:1231)
> 143: rpmbuild debuginfo subpackages multiple excluded FAILED 
> (rpmbuild.at:1296)
> 
> They're all failing with messages like this:
> 
> /home/pmatilai/repos/rpm/tests/testing/usr/lib/rpm/debugedit: 
> /home/pmatilai/repos/rpm/tests/testing/build/BUILDROOT/test-1.0-1.x86_64/bin/hello2:
>  
> Bad string pointer index 678 for unit name

That is embarrassing. I was sure I got zero FAIL on the testsuite
before sending the patch. But now I see the same (on Fedora 27).

It might be because ./tests/testing/usr/lib/rpm/debugedit doesn't seem
to be regenerated by the build. So you only see it on a fully fresh
build, not on an incremental one.

This is an interesting bug, because it was latent. We were already
accessing a bad string before, but for some reason we got away with it.

This code walks over the DIE tree in two phases. In phase zero we want
to collect the comp_dir (either from the DW_AT_comp_dir or the
DW_AT_name of the compile unit). The bug happens in phase 1, when we
don't need to collect the comp_dir. But we still do, wrongly using the
old string table... So the error is kind of correct.

Fix attached, which looks a bit funny, but really only wraps the
affected code in an if (phase == 0) block.

Cheers,

Mark
From 8e6eb541067da7848b600b95c6ff8c3bad5c1f0d Mon Sep 17 00:00:00 2001
From: Mark Wielaard <m...@klomp.org>
Date: Mon, 12 Mar 2018 14:16:15 +0100
Subject: [PATCH] debugedit: Only try to collect comp_dir in phase zero.

edit_attributes is run twice. Once for phase zero in which all strings are
collected. Then then for phase one in which the strings are rewritten. In
phase zero we also try to collect the comp_dir (either from the
DW_AT_comp_dir or the DW_AT_name of the compile unit). We were also
collecting the comp_dir is phase 1, which is unnecessary, and would not
actually work, since we would be using to old string table index for that,
which had already been rewritten.

Caught by the new string table index checks.

Signed-off-by: Mark Wielaard <m...@klomp.org>
---
 tools/debugedit.c | 52 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/tools/debugedit.c b/tools/debugedit.c
index 6c71cbcbe..84568dd29 100644
--- a/tools/debugedit.c
+++ b/tools/debugedit.c
@@ -1539,14 +1539,18 @@ edit_attributes (DSO *dso, unsigned char *ptr, struct abbrev_tag *t, int phase)
 		{
 		  const char *dir;
 		  size_t idx = do_read_32_relocated (ptr);
-		  if (idx >= debug_sections[DEBUG_STR].size)
-		    error (1, 0,
-			   "%s: Bad string pointer index %zd for comp_dir",
-			   dso->filename, idx);
-		  dir = (char *) debug_sections[DEBUG_STR].data + idx;
+		  /* In phase zero we collect the comp_dir.  */
+		  if (phase == 0)
+		    {
+		      if (idx >= debug_sections[DEBUG_STR].size)
+			error (1, 0,
+			       "%s: Bad string pointer index %zd for comp_dir",
+			       dso->filename, idx);
+		      dir = (char *) debug_sections[DEBUG_STR].data + idx;
 
-		  free (comp_dir);
-		  comp_dir = strdup (dir);
+		      free (comp_dir);
+		      comp_dir = strdup (dir);
+		    }
 
 		  if (dest_dir != NULL && phase == 0)
 		    {
@@ -1566,25 +1570,29 @@ edit_attributes (DSO *dso, unsigned char *ptr, struct abbrev_tag *t, int phase)
 		 unit. If starting with / it is a full path name.
 		 Note that we don't handle DW_FORM_string in this
 		 case.  */
-	      char *name;
 	      size_t idx = do_read_32_relocated (ptr);
-	      if (idx >= debug_sections[DEBUG_STR].size)
-		error (1, 0,
-		       "%s: Bad string pointer index %zd for unit name",
-		       dso->filename, idx);
-	      name = (char *) debug_sections[DEBUG_STR].data + idx;
-	      if (*name == '/' && comp_dir == NULL)
-		{
-		  char *enddir = strrchr (name, '/');
 
-		  if (enddir != name)
+	      /* In phase zero we will look for a comp_dir to use.  */
+	      if (phase == 0)
+		{
+		  if (idx >= debug_sections[DEBUG_STR].size)
+		    error (1, 0,
+			   "%s: Bad string pointer index %zd for unit name",
+			   dso->filename, idx);
+		  char *name = (char *) debug_sections[DEBUG_STR].data + idx;
+		  if (*name == '/' && comp_dir == NULL)
 		    {
-		      comp_dir = malloc (enddir - name + 1);
-		      memcpy (comp_dir, name, enddir - name);
-		      comp_dir [enddir - name] = '\0';
+		      char *enddir = strrchr (name, '/');
+
+		      if (enddir != name)
+			{
+			  comp_dir = malloc (enddir - name + 1);
+			  memcpy (comp_dir, name, enddir - name);
+			  comp_dir [enddir - name] = '\0';
+			}
+		      else
+			comp_dir = strdup ("/");
 		    }
-		  else
-		    comp_dir = strdup ("/");
 		}
 
 	      /* First pass (0) records the new name to be
-- 
2.14.3

_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Reply via email to