Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

read/line: check for overflow when advancing the address #731

Merged
merged 1 commit into from
Jul 4, 2024
Merged

read/line: check for overflow when advancing the address #731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 96 additions & 41 deletions src/read/line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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>,
Expand All @@ -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),
Expand All @@ -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
Expand Down Expand Up @@ -800,21 +800,21 @@ impl LineRow {
&mut self,
instruction: LineInstruction<R>,
program: &mut Program,
) -> bool
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is a breaking change.

) -> 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
}

Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand All @@ -899,7 +909,7 @@ impl LineRow {
| LineInstruction::UnknownStandard1(_, _)
| LineInstruction::UnknownStandardN(_, _)
| LineInstruction::UnknownExtended(_, _) => false,
}
})
}

/// Perform any reset that was required after copying the previous row.
Expand Down Expand Up @@ -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);
Expand All @@ -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]
Expand All @@ -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;
Expand All @@ -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(())
}
}

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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]
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand Down Expand Up @@ -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);
}
Expand All @@ -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));
Expand All @@ -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());
Expand Down
5 changes: 3 additions & 2 deletions src/write/line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading