[PATCH 2/6] gdb: handle incorrect CU size in .gdb_index

[Andrew Burgess <aburgess@redhat.com>]

The CU list within the .gdb_index contains the offset of a CU and the
length of the CU.

The length field is not needed when parsing the .gdb_index, it is
currently only used within GDB to pre-fill the length field of the
dwarf2_per_cu object.  This pre-filled length is then validated in
dwarf2_per_cu::set_length when the real CU header is read from the
.debug_info.

Unfortunately, this validation is currently done using a gdb_assert,
which is the wrong approach.  Both the .gdb_index and the CU header
are untrusted external input.  Either of them (or both of them) might
be wrong, and though they should have the same value, this is not
guaranteed.

I cannot just replace the gdb_assert in dwarf2_per_cu::set_length with
a call to complaint, there are other paths where the length is
pre-filled from the actual CU header, in this case there should be no
possibility of a mismatch, and so gdb_assert is the correct approach.

I could add a new member variable to dwarf2_per_cu that tracks if the
pre-filled length came from the real CU header or from the .gdb_index,
then we could switch on this to either call gdb_assert or complaint.

But this seemed excessive when complaints are usually turned off by
default.

Instead, I propose we just ignore the CU length from the .gdb_index,
and pass 0 instead when initially creating the dwarf2_per_cu object.
A length of 0 indicates that the CU length is unknown.  When GDB reads
the actual CU header it will then set the length at this point.

The CU length in the .gdb_index is no longer being used, but I am not
proposing that we change the .gdb_index format, though we could in
theory bump the index version number and remove the CU length, saving
a little space.  However, changing the index format means more work
for downstream tools that read or write .gdb_index sections, so
avoiding that churn seems best to me.
---
 gdb/dwarf2/read-gdb-index.c                   | 27 ++++++++++++++++---
 .../gdb.dwarf2/gdb-index-corrupted.exp        | 14 +++++++++-
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
index 86512c4ab9c..e0895fb6aa8 100644
--- a/gdb/dwarf2/read-gdb-index.c
+++ b/gdb/dwarf2/read-gdb-index.c
@@ -573,11 +573,32 @@ create_cus_from_gdb_index_list (dwarf2_per_bfd *per_bfd,
 
       sect_offset sect_off
 	= (sect_offset) extract_unsigned_integer (cu_list, 8, BFD_ENDIAN_LITTLE);
-      ULONGEST length = extract_unsigned_integer (cu_list + 8, 8, BFD_ENDIAN_LITTLE);
+      /* Each entry in the CU list is a pair of (offset, length), but
+	 we deliberately ignore the length as this information is not
+	 needed.  When the real CU contents are read they also contain
+	 a length.  The length stored in the .gdb_index and the real
+	 length in the CU header should match, but both the .gdb_index
+	 and the real CU are untrusted input, so could be corrupt, in
+	 which case the lengths might not match.
+
+	 If we pre-fill the length here then the dwarf2_per_cu would
+	 need to track that the length came from the .gdb_index so
+	 that later on, when the length is read from the real CU, if
+	 the lengths don't match GDB should use the real length and
+	 warn about the mismatch.
+
+	 This feels like a lot of work just for a length field that we
+	 can safely ignore, especially when the complaint mechanism,
+	 through which we would report the length mismatch, is off
+	 by default.  Instead, pass 0 as the length to allocate_per_cu
+	 below, this indicates that the length is unknown, and can be
+	 filled in later when the real CU header is read.  The length
+	 from the .gdb_index is ignored completely.  */
       cu_list += 2 * 8;
 
-      dwarf2_per_cu_up per_cu = per_bfd->allocate_per_cu (section, sect_off,
-							  length, is_dwz);
+      /* Pass 0 for the length, see comment above.  */
+      dwarf2_per_cu_up per_cu
+	= per_bfd->allocate_per_cu (section, sect_off, 0, is_dwz);
       units.emplace_back (per_cu.get ());
       per_bfd->add_unit (std::move (per_cu));
     }
diff --git a/gdb/testsuite/gdb.dwarf2/gdb-index-corrupted.exp b/gdb/testsuite/gdb.dwarf2/gdb-index-corrupted.exp
index a57b3c26341..356b12f97ce 100644
--- a/gdb/testsuite/gdb.dwarf2/gdb-index-corrupted.exp
+++ b/gdb/testsuite/gdb.dwarf2/gdb-index-corrupted.exp
@@ -173,6 +173,16 @@ proc run_test { testname } {
 		name _start {{cu_label function global}}
 		name foo {{cu_label function global}} 0xffffff00
 	    }
+	} elseif { $testname eq "bad-cu-length" } {
+	    # Create an otherwise valid index, but record a bogus length
+	    # for the CU in the index's CU list; one that doesn't match the
+	    # actual length of the CU in .debug_info.  GDB should handle
+	    # this gracefully rather than crashing when it later comes to
+	    # expand the CU.
+	    gdb_index {} {
+		cu cu_label 0x1f
+		name _start {{cu_label function global}}
+	    }
 	} elseif { $testname eq "valid-index" } {
 	    # Generate a well-formed index that includes a valid main
 	    # function (valid language, with the name pointing at the
@@ -236,7 +246,8 @@ proc run_test { testname } {
 		   || $testname eq "main-language" \
 		   || $testname eq "long-cu-vector" \
 		   || $testname eq "name-offset-1" \
-		   || $testname eq "name-offset-2" } {
+		   || $testname eq "name-offset-2" \
+		   || $testname eq "bad-cu-length" } {
 	set expect_gdb_index_ignored false
 	set expect_start true
 	set expect_method false
@@ -360,6 +371,7 @@ set tests { empty-component \
 		long-cu-vector \
 		name-offset-1 \
 		name-offset-2 \
+		bad-cu-length \
 		valid-index }
 
 foreach_with_prefix test $tests {
-- 
2.25.4