Re: [Rpm-maint] [PATCH] debugedit: Check .debug_str index is valid before use.
On 03/12/2018 03:27 PM, Mark Wielaard wrote: 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. Yup, the logic to update the test root contents is far from perfect... 'rm -rf tests/testing' works for forcing the issue though. 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. Ack, thanks for the explanation and the fast fix! Applied. - Panu - Cheers, Mark ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH] debugedit: Check .debug_str index is valid before use.
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,
MarkFrom 8e6eb541067da7848b600b95c6ff8c3bad5c1f0d Mon Sep 17 00:00:00 2001
From: Mark Wielaard
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
---
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
- com
Re: [Rpm-maint] [PATCH] debugedit: Check .debug_str index is valid before use.
On 03/09/2018 04:24 PM, Panu Matilainen wrote:
On 03/07/2018 05:25 PM, Mark Wielaard wrote:
debugedit would blindly use an .debug_str index from the .debug_info or
.debug_line sections assuming it would result in a valid string. Which
would crash and burn if the DWARF data was bogus when the string was
used. So check whenever converting an string index into a char pointer
so we can produce a more helpful error message.
https://bugzilla.redhat.com/show_bug.cgi?id=1543912
Signed-off-by: Mark Wielaard
---
tools/debugedit.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/tools/debugedit.c b/tools/debugedit.c
index 57cd830..6c71cbc 100644
--- a/tools/debugedit.c
+++ b/tools/debugedit.c
@@ -820,6 +820,9 @@ record_file_string_entry_idx (struct strings
*strings, size_t old_idx)
struct stridxentry *entry = string_find_new_entry (strings, old_idx);
if (entry != NULL)
{
+ if (old_idx >= debug_sections[DEBUG_STR].size)
+ error (1, 0, "Bad string pointer index %zd", old_idx);
+
Strent *strent;
const char *old_str = (char *)debug_sections[DEBUG_STR].data +
old_idx;
const char *file = skip_dir_prefix (old_str, base_dir);
@@ -870,6 +873,9 @@ record_existing_string_entry_idx (struct strings
*strings, size_t old_idx)
struct stridxentry *entry = string_find_new_entry (strings, old_idx);
if (entry != NULL)
{
+ if (old_idx >= debug_sections[DEBUG_STR].size)
+ error (1, 0, "Bad string pointer index %zd", old_idx);
+
const char *str = (char *)debug_sections[DEBUG_STR].data +
old_idx;
Strent *strent = strtab_add_len (strings->str_tab,
str, strlen (str) + 1);
@@ -1533,6 +1539,10 @@ 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;
free (comp_dir);
@@ -1558,6 +1568,10 @@ edit_attributes (DSO *dso, unsigned char *ptr,
struct abbrev_tag *t, int phase)
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)
{
I was somewhat tempted to mumble something about putting these
common-looking checks into a function but ... meh, it's Friday :P
Applied, thanks for the patch!
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
- Panu -
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH] debugedit: Check .debug_str index is valid before use.
On 03/07/2018 05:25 PM, Mark Wielaard wrote:
debugedit would blindly use an .debug_str index from the .debug_info or
.debug_line sections assuming it would result in a valid string. Which
would crash and burn if the DWARF data was bogus when the string was
used. So check whenever converting an string index into a char pointer
so we can produce a more helpful error message.
https://bugzilla.redhat.com/show_bug.cgi?id=1543912
Signed-off-by: Mark Wielaard
---
tools/debugedit.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/tools/debugedit.c b/tools/debugedit.c
index 57cd830..6c71cbc 100644
--- a/tools/debugedit.c
+++ b/tools/debugedit.c
@@ -820,6 +820,9 @@ record_file_string_entry_idx (struct strings *strings,
size_t old_idx)
struct stridxentry *entry = string_find_new_entry (strings, old_idx);
if (entry != NULL)
{
+ if (old_idx >= debug_sections[DEBUG_STR].size)
+ error (1, 0, "Bad string pointer index %zd", old_idx);
+
Strent *strent;
const char *old_str = (char *)debug_sections[DEBUG_STR].data + old_idx;
const char *file = skip_dir_prefix (old_str, base_dir);
@@ -870,6 +873,9 @@ record_existing_string_entry_idx (struct strings *strings,
size_t old_idx)
struct stridxentry *entry = string_find_new_entry (strings, old_idx);
if (entry != NULL)
{
+ if (old_idx >= debug_sections[DEBUG_STR].size)
+ error (1, 0, "Bad string pointer index %zd", old_idx);
+
const char *str = (char *)debug_sections[DEBUG_STR].data + old_idx;
Strent *strent = strtab_add_len (strings->str_tab,
str, strlen (str) + 1);
@@ -1533,6 +1539,10 @@ 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;
free (comp_dir);
@@ -1558,6 +1568,10 @@ edit_attributes (DSO *dso, unsigned char *ptr, struct
abbrev_tag *t, int phase)
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)
{
I was somewhat tempted to mumble something about putting these
common-looking checks into a function but ... meh, it's Friday :P
Applied, thanks for the patch!
- Panu -
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
