read/line: check for overflow when advancing the address (gimli-rs#731)

philipc · web-flow · commit 450cb69ace4b · 2024-07-04T15:11:24.000+10:00
Addresses in line sequences should only increase.
The `addr2line` crate relies on this for its binary search.
diff --git a/src/read/line.rs b/src/read/line.rs
@@ -240,7 +240,7 @@ where
                 Err(err) => return Err(err),
                 Ok(None) => return Ok(None),
                 Ok(Some(instruction)) => {
-                    if self.row.execute(instruction, &mut self.program) {
+                    if self.row.execute(instruction, &mut self.program)? {
                         if self.row.tombstone {
                             // Perform any reset that was required for the tombstone row.
                             // Normally this is done when `next_row` is called again, but for
@@ -631,7 +631,7 @@ pub type LineNumberRow = LineRow;
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct LineRow {
     tombstone: bool,
-    address: Wrapping<u64>,
+    address: u64,
     op_index: Wrapping<u64>,
     file: u64,
     line: Wrapping<u64>,
@@ -652,7 +652,7 @@ impl LineRow {
             // "At the beginning of each sequence within a line number program, the
             // state of the registers is:" -- Section 6.2.2
             tombstone: false,
-            address: Wrapping(0),
+            address: 0,
             op_index: Wrapping(0),
             file: 1,
             line: Wrapping(1),
@@ -676,7 +676,7 @@ impl LineRow {
     /// generated by the compiler."
     #[inline]
     pub fn address(&self) -> u64 {
-        self.address.0
+        self.address
     }
 
     /// > An unsigned integer representing the index of an operation within a VLIW
@@ -800,21 +800,21 @@ impl LineRow {
         &mut self,
         instruction: LineInstruction<R>,
         program: &mut Program,
-    ) -> bool
+    ) -> Result<bool>
     where
         Program: LineProgram<R>,
         R: Reader,
     {
-        match instruction {
+        Ok(match instruction {
             LineInstruction::Special(opcode) => {
-                self.exec_special_opcode(opcode, program.header());
+                self.exec_special_opcode(opcode, program.header())?;
                 true
             }
 
             LineInstruction::Copy => true,
 
             LineInstruction::AdvancePc(operation_advance) => {
-                self.apply_operation_advance(operation_advance, program.header());
+                self.apply_operation_advance(operation_advance, program.header())?;
                 false
             }
 
@@ -846,13 +846,18 @@ impl LineRow {
             LineInstruction::ConstAddPc => {
                 let adjusted = self.adjust_opcode(255, program.header());
                 let operation_advance = adjusted / program.header().line_encoding.line_range;
-                self.apply_operation_advance(u64::from(operation_advance), program.header());
+                self.apply_operation_advance(u64::from(operation_advance), program.header())?;
                 false
             }
 
             LineInstruction::FixedAddPc(operand) => {
-                self.address += Wrapping(u64::from(operand));
-                self.op_index.0 = 0;
+                if !self.tombstone {
+                    self.address = self
+                        .address
+                        .checked_add(u64::from(operand))
+                        .ok_or(Error::AddressOverflow)?;
+                    self.op_index.0 = 0;
+                }
                 false
             }
 
@@ -879,8 +884,13 @@ impl LineRow {
             LineInstruction::SetAddress(address) => {
                 let tombstone_address = !0 >> (64 - program.header().encoding.address_size * 8);
                 self.tombstone = address == tombstone_address;
-                self.address.0 = address;
-                self.op_index.0 = 0;
+                if !self.tombstone {
+                    if address < self.address {
+                        return Err(Error::InvalidAddressRange);
+                    }
+                    self.address = address;
+                    self.op_index.0 = 0;
+                }
                 false
             }
 
@@ -899,7 +909,7 @@ impl LineRow {
             | LineInstruction::UnknownStandard1(_, _)
             | LineInstruction::UnknownStandardN(_, _)
             | LineInstruction::UnknownExtended(_, _) => false,
-        }
+        })
     }
 
     /// Perform any reset that was required after copying the previous row.
@@ -940,7 +950,11 @@ impl LineRow {
         &mut self,
         operation_advance: u64,
         header: &LineProgramHeader<R>,
-    ) {
+    ) -> Result<()> {
+        if self.tombstone {
+            return Ok(());
+        }
+
         let operation_advance = Wrapping(operation_advance);
 
         let minimum_instruction_length = u64::from(header.line_encoding.minimum_instruction_length);
@@ -950,15 +964,20 @@ impl LineRow {
             u64::from(header.line_encoding.maximum_operations_per_instruction);
         let maximum_operations_per_instruction = Wrapping(maximum_operations_per_instruction);
 
-        if maximum_operations_per_instruction.0 == 1 {
-            self.address += minimum_instruction_length * operation_advance;
+        let address_advance = if maximum_operations_per_instruction.0 == 1 {
             self.op_index.0 = 0;
+            minimum_instruction_length * operation_advance
         } else {
             let op_index_with_advance = self.op_index + operation_advance;
-            self.address += minimum_instruction_length
-                * (op_index_with_advance / maximum_operations_per_instruction);
             self.op_index = op_index_with_advance % maximum_operations_per_instruction;
-        }
+            minimum_instruction_length
+                * (op_index_with_advance / maximum_operations_per_instruction)
+        };
+        self.address = self
+            .address
+            .checked_add(address_advance.0)
+            .ok_or(Error::AddressOverflow)?;
+        Ok(())
     }
 
     #[inline]
@@ -967,7 +986,11 @@ impl LineRow {
     }
 
     /// Section 6.2.5.1
-    fn exec_special_opcode<R: Reader>(&mut self, opcode: u8, header: &LineProgramHeader<R>) {
+    fn exec_special_opcode<R: Reader>(
+        &mut self,
+        opcode: u8,
+        header: &LineProgramHeader<R>,
+    ) -> Result<()> {
         let adjusted_opcode = self.adjust_opcode(opcode, header);
 
         let line_range = header.line_encoding.line_range;
@@ -979,7 +1002,8 @@ impl LineRow {
         self.apply_line_advance(line_base + i64::from(line_advance));
 
         // Step 2
-        self.apply_operation_advance(u64::from(operation_advance), header);
+        self.apply_operation_advance(u64::from(operation_advance), header)?;
+        Ok(())
     }
 }
 
@@ -2480,7 +2504,7 @@ mod tests {
         let mut program = IncompleteLineProgram { header };
         let is_new_row = registers.execute(opcode, &mut program);
 
-        assert_eq!(is_new_row, expect_new_row);
+        assert_eq!(is_new_row, Ok(expect_new_row));
         assert_eq!(registers, expected_registers);
     }
 
@@ -2533,7 +2557,7 @@ mod tests {
         let opcode = LineInstruction::Special(52);
 
         let mut expected_registers = initial_registers;
-        expected_registers.address.0 += 3;
+        expected_registers.address += 3;
 
         assert_exec_opcode(header, initial_registers, opcode, expected_registers, true);
     }
@@ -2547,7 +2571,7 @@ mod tests {
         let opcode = LineInstruction::Special(55);
 
         let mut expected_registers = initial_registers;
-        expected_registers.address.0 += 3;
+        expected_registers.address += 3;
         expected_registers.line.0 += 3;
 
         assert_exec_opcode(header, initial_registers, opcode, expected_registers, true);
@@ -2563,7 +2587,7 @@ mod tests {
         let opcode = LineInstruction::Special(49);
 
         let mut expected_registers = initial_registers;
-        expected_registers.address.0 += 3;
+        expected_registers.address += 3;
         expected_registers.line.0 -= 3;
 
         assert_exec_opcode(header, initial_registers, opcode, expected_registers, true);
@@ -2592,7 +2616,7 @@ mod tests {
         let header = make_test_header(EndianSlice::new(&[], LittleEndian));
 
         let mut initial_registers = LineRow::new(&header);
-        initial_registers.address.0 = 1337;
+        initial_registers.address = 1337;
         initial_registers.line.0 = 42;
 
         let opcode = LineInstruction::Copy;
@@ -2609,23 +2633,20 @@ mod tests {
         let opcode = LineInstruction::AdvancePc(42);
 
         let mut expected_registers = initial_registers;
-        expected_registers.address.0 += 42;
+        expected_registers.address += 42;
 
         assert_exec_opcode(header, initial_registers, opcode, expected_registers, false);
     }
 
     #[test]
     fn test_exec_advance_pc_overflow() {
         let header = make_test_header(EndianSlice::new(&[], LittleEndian));
+        let mut registers = LineRow::new(&header);
+        registers.address = u64::MAX;
         let opcode = LineInstruction::AdvancePc(42);
-
-        let mut initial_registers = LineRow::new(&header);
-        initial_registers.address.0 = u64::MAX;
-
-        let mut expected_registers = initial_registers;
-        expected_registers.address.0 = 41;
-
-        assert_exec_opcode(header, initial_registers, opcode, expected_registers, false);
+        let mut program = IncompleteLineProgram { header };
+        let result = registers.execute(opcode, &mut program);
+        assert_eq!(result, Err(Error::AddressOverflow));
     }
 
     #[test]
@@ -2758,11 +2779,22 @@ mod tests {
         let opcode = LineInstruction::ConstAddPc;
 
         let mut expected_registers = initial_registers;
-        expected_registers.address.0 += 20;
+        expected_registers.address += 20;
 
         assert_exec_opcode(header, initial_registers, opcode, expected_registers, false);
     }
 
+    #[test]
+    fn test_exec_const_add_pc_overflow() {
+        let header = make_test_header(EndianSlice::new(&[], LittleEndian));
+        let mut registers = LineRow::new(&header);
+        registers.address = u64::MAX;
+        let opcode = LineInstruction::ConstAddPc;
+        let mut program = IncompleteLineProgram { header };
+        let result = registers.execute(opcode, &mut program);
+        assert_eq!(result, Err(Error::AddressOverflow));
+    }
+
     #[test]
     fn test_exec_fixed_add_pc() {
         let header = make_test_header(EndianSlice::new(&[], LittleEndian));
@@ -2773,12 +2805,24 @@ mod tests {
         let opcode = LineInstruction::FixedAddPc(10);
 
         let mut expected_registers = initial_registers;
-        expected_registers.address.0 += 10;
+        expected_registers.address += 10;
         expected_registers.op_index.0 = 0;
 
         assert_exec_opcode(header, initial_registers, opcode, expected_registers, false);
     }
 
+    #[test]
+    fn test_exec_fixed_add_pc_overflow() {
+        let header = make_test_header(EndianSlice::new(&[], LittleEndian));
+        let mut registers = LineRow::new(&header);
+        registers.address = u64::MAX;
+        registers.op_index.0 = 1;
+        let opcode = LineInstruction::FixedAddPc(10);
+        let mut program = IncompleteLineProgram { header };
+        let result = registers.execute(opcode, &mut program);
+        assert_eq!(result, Err(Error::AddressOverflow));
+    }
+
     #[test]
     fn test_exec_set_prologue_end() {
         let header = make_test_header(EndianSlice::new(&[], LittleEndian));
@@ -2855,7 +2899,7 @@ mod tests {
         let opcode = LineInstruction::SetAddress(3030);
 
         let mut expected_registers = initial_registers;
-        expected_registers.address.0 = 3030;
+        expected_registers.address = 3030;
 
         assert_exec_opcode(header, initial_registers, opcode, expected_registers, false);
     }
@@ -2868,11 +2912,22 @@ mod tests {
 
         let mut expected_registers = initial_registers;
         expected_registers.tombstone = true;
-        expected_registers.address.0 = !0;
 
         assert_exec_opcode(header, initial_registers, opcode, expected_registers, false);
     }
 
+    #[test]
+    fn test_exec_set_address_backwards() {
+        let header = make_test_header(EndianSlice::new(&[], LittleEndian));
+        let mut registers = LineRow::new(&header);
+        registers.address = 1;
+        let opcode = LineInstruction::SetAddress(0);
+
+        let mut program = IncompleteLineProgram { header };
+        let result = registers.execute(opcode, &mut program);
+        assert_eq!(result, Err(Error::InvalidAddressRange));
+    }
+
     #[test]
     fn test_exec_define_file() {
         let mut program = make_test_program(EndianSlice::new(&[], LittleEndian));
@@ -2888,7 +2943,7 @@ mod tests {
         };
 
         let opcode = LineInstruction::DefineFile(file);
-        let is_new_row = row.execute(opcode, &mut program);
+        let is_new_row = row.execute(opcode, &mut program).unwrap();
 
         assert!(!is_new_row);
         assert_eq!(Some(&file), program.header().file_names.last());
diff --git a/src/write/line.rs b/src/write/line.rs
@@ -1126,13 +1126,14 @@ mod convert {
                             Some(val) => address = Some(val),
                             None => return Err(ConvertError::InvalidAddress),
                         }
-                        from_row.execute(read::LineInstruction::SetAddress(0), &mut from_program);
+                        from_row
+                            .execute(read::LineInstruction::SetAddress(0), &mut from_program)?;
                     }
                     read::LineInstruction::DefineFile(_) => {
                         return Err(ConvertError::UnsupportedLineInstruction);
                     }
                     _ => {
-                        if from_row.execute(instruction, &mut from_program) {
+                        if from_row.execute(instruction, &mut from_program)? {
                             if !program.in_sequence() {
                                 program.begin_sequence(address);
                                 address = None;

Original file line number	Diff line number	Diff line change
`@@ -1126,13 +1126,14 @@ mod convert {`
`1126`	`1126`	`Some(val) => address = Some(val),`
`1127`	`1127`	`None => return Err(ConvertError::InvalidAddress),`
`1128`	`1128`	`}`
`1129`		`- from_row.execute(read::LineInstruction::SetAddress(0), &mut from_program);`
	`1129`	`+ from_row`
	`1130`	`+ .execute(read::LineInstruction::SetAddress(0), &mut from_program)?;`
`1130`	`1131`	`}`
`1131`	`1132`	`read::LineInstruction::DefineFile(_) => {`
`1132`	`1133`	`return Err(ConvertError::UnsupportedLineInstruction);`
`1133`	`1134`	`}`
`1134`	`1135`	`_ => {`
`1135`		`- if from_row.execute(instruction, &mut from_program) {`
	`1136`	`+ if from_row.execute(instruction, &mut from_program)? {`
`1136`	`1137`	`if !program.in_sequence() {`
`1137`	`1138`	`program.begin_sequence(address);`
`1138`	`1139`	`address = None;`