[PATCH] libdw: dwarf_formsdata should return a signed value

[PATCH] libdw: dwarf_formsdata should return a signed value

[Petr Machata]

The function dwarf_formsdata is used for decoding signed values, but
except for the variable-length DW_FORM_sdata, it uses unsigned
primitives to decode the value. This is not a problem for 64-bit values,
but the smaller values come decoded wrong. Fix by changing to signed
primitives for decoding the fixed-length forms.

Add a test case that uses dwarf_aggregate_size to determine an array
size whose lower bound is -1, encoded using DW_FORM_data1, and upper
bound 255 with DW_FORM_data2. When the -1 is decoded wrongly, it comes
back as 255, and the array size is 1. The correct array size should be
257.

Signed-off-by: Petr Machata <pmachata@gmail.com>
---
 libdw/ChangeLog             |   7 ++++
 libdw/dwarf_formsdata.c     |   8 ++---
 tests/run-aggregate-size.sh |   8 ++++-
 tests/testfile-sizes4.o.bz2 | Bin 0 -> 387 bytes
 tests/testfile-sizes4.s     |  77 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 95 insertions(+), 5 deletions(-)
 create mode 100644 tests/testfile-sizes4.o.bz2
 create mode 100644 tests/testfile-sizes4.s

Regarding testfile-sizes4.s, that's a product of a TCL script written using
dwarf.exp from GDB. I can post it as well (actually would prefer it like
that),but I think you bounced it before already (I found a TCL script foranother
test case from way back in my elfutils directory. I seem to recall you
didn'tlike this approach for whatever reason.)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 7cfc7825..479dd42b 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,10 @@
+2018-01-14  Petr Machata  <pmachata@gmail.com>
+
+	* dwarf_formsdata.c (dwarf_formsdata):
+	<DW_FORM_data1>: Cast to signed char.
+	<DW_FORM_data2,4,8>: Use read_*sbyte_unaligned instead of
+	read_*ubyte_unaligned.
+
 2017-12-26  Mark Wielaard  <mark@klomp.org>
 
 	* libdwP.h (struct Dwarf_Abbrev): Pack struct. Remove attrcnt,
diff --git a/libdw/dwarf_formsdata.c b/libdw/dwarf_formsdata.c
index e7deaee1..bc2b508d 100644
--- a/libdw/dwarf_formsdata.c
+++ b/libdw/dwarf_formsdata.c
@@ -53,25 +53,25 @@ dwarf_formsdata (Dwarf_Attribute *attr, Dwarf_Sword *return_sval)
 	  __libdw_seterrno (DWARF_E_INVALID_DWARF);
 	  return -1;
 	}
-      *return_sval = *attr->valp;
+      *return_sval = (signed char) *attr->valp;
       break;
 
     case DW_FORM_data2:
       if (datap + 2 > endp)
 	goto invalid;
-      *return_sval = read_2ubyte_unaligned (attr->cu->dbg, attr->valp);
+      *return_sval = read_2sbyte_unaligned (attr->cu->dbg, attr->valp);
       break;
 
     case DW_FORM_data4:
       if (datap + 4 > endp)
 	goto invalid;
-      *return_sval = read_4ubyte_unaligned (attr->cu->dbg, attr->valp);
+      *return_sval = read_4sbyte_unaligned (attr->cu->dbg, attr->valp);
       break;
 
     case DW_FORM_data8:
       if (datap + 8 > endp)
 	goto invalid;
-      *return_sval = read_8ubyte_unaligned (attr->cu->dbg, attr->valp);
+      *return_sval = read_8sbyte_unaligned (attr->cu->dbg, attr->valp);
       break;
 
     case DW_FORM_sdata:
diff --git a/tests/run-aggregate-size.sh b/tests/run-aggregate-size.sh
index 6d8aa240..08d57bbf 100755
--- a/tests/run-aggregate-size.sh
+++ b/tests/run-aggregate-size.sh
@@ -63,7 +63,9 @@
 #
 # gcc -std=c99 -g -c -o testfile-sizes3.o sizes.c
 
-testfiles testfile-sizes1.o testfile-sizes2.o testfile-sizes3.o
+# The file testfile-size4.o is hand-crafted.
+
+testfiles testfile-sizes1.o testfile-sizes2.o testfile-sizes3.o testfile-sizes4.o
 
 testrun_compare ${abs_builddir}/aggregate_size -e testfile-sizes1.o <<\EOF
 c size 1
@@ -104,4 +106,8 @@ f size 4
 b size 4
 EOF
 
+testrun_compare ${abs_builddir}/aggregate_size -e testfile-sizes4.o <<\EOF
+v size 257
+EOF
+
 exit 0
diff --git a/tests/testfile-sizes4.o.bz2 b/tests/testfile-sizes4.o.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..046e0a23429c166c659349f628c39170bbb4818f
GIT binary patch
literal 387
zcmV-}0et>KT4*^jL0KkKS*U1%zyJYNfAs(V>Th(1qytN(LIL;ppJ2d1006)MKmY&;
zumMI1hJq6+dsOu{DWf1XWM}{Y05m;ErkaNtGHApxG{8W>nqZ6*6HPP#VFZaN$V1Wr
zpk!zPpa99EKn(_(N{z;abVA6Etg=-jpBQVd%dq;e6Q>9sa+xWbp{9draa^JPJRN<z
zxTM=LEHNo0y4X>G7UMVI%lII?kh^@`&;bZ`8l9vj(N&GCdoUne%}m2_-C-sJeC$dc
zRs=NKY&V)*jSAC3*!qNUJ2&v)o8u3XuT?+XU(%w3Ol_&e+S<9PJno*e2s+^yC2Wuv
zYYL>0n@d&qy3D-8C5BTLMaZa%@P*vOUdIVvr$m7B7*vxZ#l?7usFf6{7O7%ox7xzP
zn4E{qj4D#xi1vKslj!Or-mJ>1XagNuBz3(2vJWi57V$TVwSIoYVz!`!_C;<?CRIGg
hmYOX-mb_;`4XN-OZz-5%<Y!O#yOJrwgoQ&y4glQWvNiw!

literal 0
HcmV?d00001

diff --git a/tests/testfile-sizes4.s b/tests/testfile-sizes4.s
new file mode 100644
index 00000000..a2430215
--- /dev/null
+++ b/tests/testfile-sizes4.s
@@ -0,0 +1,77 @@
+        .section .debug_info
+.Lcu1_begin:
+        .4byte        .Lcu1_end - .Lcu1_start
+.Lcu1_start:
+        .2byte        4                 /* Version */
+        .4byte        .Labbrev1_begin   /* Abbrevs */
+        .byte        8                  /* Pointer size */
+        .uleb128        2               /* Abbrev (DW_TAG_compile_unit) */
+        .uleb128        3               /* Abbrev (DW_TAG_variable) */
+        .ascii        "v\0"
+        .4byte        .Llabel1 - .Lcu1_begin
+.Llabel1:
+        .uleb128        4               /* Abbrev (DW_TAG_array_type) */
+        .4byte        .Llabel2 - .Lcu1_begin
+        .uleb128        5               /* Abbrev (DW_TAG_subrange_type) */
+        .byte        -1
+        .2byte        255
+        .byte        0x0                /* Terminate children */
+.Llabel2:
+        .uleb128        6               /* Abbrev (DW_TAG_base_type) */
+        .byte        1
+        .byte        0x0                /* Terminate children */
+.Lcu1_end:
+        .section .note.gnu.build-id, "a", %note
+        .4byte        4
+        .4byte        8
+        .4byte        3
+        .ascii        "GNU\0"
+        .byte        0x01
+        .byte        0x02
+        .byte        0x03
+        .byte        0x04
+        .byte        0x05
+        .byte        0x06
+        .byte        0x07
+        .byte        0x08
+        .section .debug_abbrev
+.Labbrev1_begin:
+        .uleb128        2               /* Abbrev start */
+        .uleb128        0x11            /* DW_TAG_compile_unit */
+        .byte        1                  /* has_children */
+        .byte        0x0                /* Terminator */
+        .byte        0x0                /* Terminator */
+        .uleb128        3               /* Abbrev start */
+        .uleb128        0x34            /* DW_TAG_variable */
+        .byte        0                  /* has_children */
+        .uleb128        0x03            /* DW_AT_name */
+        .uleb128        0x08            /* DW_FORM_string */
+        .uleb128        0x49            /* DW_AT_type */
+        .uleb128        0x13            /* DW_FORM_ref4 */
+        .byte        0x0                /* Terminator */
+        .byte        0x0                /* Terminator */
+        .uleb128        4               /* Abbrev start */
+        .uleb128        0x01            /* DW_TAG_array_type */
+        .byte        1                  /* has_children */
+        .uleb128        0x49            /* DW_AT_type */
+        .uleb128        0x13            /* DW_FORM_ref4 */
+        .byte        0x0                /* Terminator */
+        .byte        0x0                /* Terminator */
+        .uleb128        5               /* Abbrev start */
+        .uleb128        0x21            /* DW_TAG_subrange_type */
+        .byte        0                  /* has_children */
+        .uleb128        0x22            /* DW_AT_lower_bound */
+        .uleb128        0x0b            /* DW_FORM_data1 */
+        .uleb128        0x2f            /* DW_AT_upper_bound */
+        .uleb128        0x05            /* DW_FORM_data2 */
+        .byte        0x0                /* Terminator */
+        .byte        0x0                /* Terminator */
+        .uleb128        6               /* Abbrev start */
+        .uleb128        0x24            /* DW_TAG_base_type */
+        .byte        0                  /* has_children */
+        .uleb128        0x0b            /* DW_AT_byte_size */
+        .uleb128        0x0b            /* DW_FORM_data1 */
+        .byte        0x0                /* Terminator */
+        .byte        0x0                /* Terminator */
+        .byte        0x0                /* Terminator */
+        .byte        0x0                /* Terminator */
-- 
2.14.3

Re: [PATCH] libdw: dwarf_formsdata should return a signed value

[Mark Wielaard]

Hi Petr,

On Sun, Jan 14, 2018 at 03:22:05AM +0100, Petr Machata wrote:
> The function dwarf_formsdata is used for decoding signed values, but
> except for the variable-length DW_FORM_sdata, it uses unsigned
> primitives to decode the value. This is not a problem for 64-bit values,
> but the smaller values come decoded wrong. Fix by changing to signed
> primitives for decoding the fixed-length forms.
> 
> Add a test case that uses dwarf_aggregate_size to determine an array
> size whose lower bound is -1, encoded using DW_FORM_data1, and upper
> bound 255 with DW_FORM_data2. When the -1 is decoded wrongly, it comes
> back as 255, and the array size is 1. The correct array size should be
> 257.

Thanks for the patch and testcase. The change is obviously correct.
I wonder why we never noticed before. Applied to master including adding
the new test files to EXTRA_DIST to make sure they get into the dist
tar ball.

Cheers,

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 831532b2..758f20e6 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@
+2018-01-14  Petr Machata  <pmachata@gmail.com>
+
+	* testfile-sizes4.o.bz2: New test file.
+	* testfile-sizes4.s: New test source.
+	* run-aggregate-size.sh: Check testfile-sizes4.o v size 257.
+
 2017-12-23  Mark Wielaard  <mark@klomp.org>
 
 	* backtrace-subr.sh (check_native_core): Use a lock file and try
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 64cb5bd9..1fce4474 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -333,7 +333,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
 	     run-deleted.sh run-linkmap-cut.sh linkmap-cut-lib.so.bz2 \
 	     linkmap-cut.bz2 linkmap-cut.core.bz2 \
 	     run-aggregate-size.sh testfile-sizes1.o.bz2 testfile-sizes2.o.bz2 \
-	     testfile-sizes3.o.bz2 run-peel-type.sh \
+	     testfile-sizes3.o.bz2 testfile-sizes4.o.bz2 testfile-sizes4.s \
+	     run-peel-type.sh \
 	     run-readelf-A.sh testfileppc32attrs.o.bz2 \
 	     testfilesparc64attrs.o.bz2 testfileppc64attrs.o.bz2 \
 	     testfile-debug-types.bz2 \

Re: [PATCH] libdw: dwarf_formsdata should return a signed value

[Mark Wielaard]

On Mon, 2018-01-15 at 00:06 +0100, Petr Machata wrote:
> Ugh, sorry about that. It's been a while since I last posted, I
> forgot the routine!

No worries at all. This is all mechanics/boilerplate.
We have a buildbot now, which would have caught that:
https://builder.wildebeest.org/buildbot/
It doesn't yet have a "try" bot. Which would be helpful in these kind
of cases.

Cheers,

Mark

P.S. Sending HTML email on the other hand... :)
The mailinglist will just drop it (or did you get a bounce message?)

Re: [PATCH] libdw: dwarf_formsdata should return a signed value

[Petr Machata]

2018-01-15 9:23 GMT+01:00 Mark Wielaard <mark@klomp.org>:
> On Mon, 2018-01-15 at 00:06 +0100, Petr Machata wrote:
>> Ugh, sorry about that. It's been a while since I last posted, I
>> forgot the routine!
>
> No worries at all. This is all mechanics/boilerplate.
> We have a buildbot now, which would have caught that:
> https://builder.wildebeest.org/buildbot/

Ha, cool! Can / should I use it next time I have stuff to send?

> P.S. Sending HTML email on the other hand... :)

I forgot to switch the gmail web interface to plain text. Yeah, I know.

> The mailinglist will just drop it (or did you get a bounce message?)

I got a bounce message.

Petr

Re: [PATCH] libdw: dwarf_formsdata should return a signed value

[Mark Wielaard]

On Mon, Jan 15, 2018 at 12:34:27PM +0100, Petr Machata wrote:
> 2018-01-15 9:23 GMT+01:00 Mark Wielaard <mark@klomp.org>:
> > On Mon, 2018-01-15 at 00:06 +0100, Petr Machata wrote:
> >> Ugh, sorry about that. It's been a while since I last posted, I
> >> forgot the routine!
> >
> > No worries at all. This is all mechanics/boilerplate.
> > We have a buildbot now, which would have caught that:
> > https://builder.wildebeest.org/buildbot/
> 
> Ha, cool! Can / should I use it next time I have stuff to send?

There is no try-server setup yet, sorry.
But I certainly would like to have one.
Or we could make it monitor git branches user/buildbot and
do builds from those. have to find an example setup to enable.

For now it will do every commit pushed to master though.

Cheers,

Mark