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.

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

Fix panic when accessing union from invalid input #100

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]
- \[Rust\]: Add support for strings in unions
- \[Rust\]: Add support for structs in unions
- \[Rust\]: Fix panic when accessing union from invalid input

## [0.3.0] - 2022-02-06
### Added
Expand Down
19 changes: 11 additions & 8 deletions planus/src/table_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ impl<'buf> Table<'buf> {
buffer: SliceWithStartOffset<'buf>,
field_offset: usize,
) -> Result<Self, ErrorKind> {
let field_value = u32::from_buffer(buffer, field_offset)?;
let object_size = u32::from_buffer(buffer, field_offset)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is run primarily when a table is accessed from another table. The "field value" is an offset to where the actual table is. I would be up for a better name for this variable, but object_size is not better.

let object_offset = field_offset
.checked_add(field_value as usize)
.checked_add(object_size as usize)
.ok_or(ErrorKind::InvalidOffset)?;
let object = buffer.advance(object_offset)?;

Expand Down Expand Up @@ -92,12 +92,6 @@ impl<'buf> Table<'buf> {
type_: &'static str,
method: &'static str,
) -> crate::Result<Option<T>> {
let offset = self
.vtable
.get(2 * vtable_offset..2 * (vtable_offset + 2))
.expect("IMPOSSIBLE: trying to access invalid vtable offset for union");
let tag_offset = u16::from_le_bytes(offset[..2].try_into().unwrap()) as usize;
let value_offset = u16::from_le_bytes(offset[2..].try_into().unwrap()) as usize;
let make_error = |error_kind| crate::errors::Error {
source_location: crate::errors::ErrorLocation {
type_,
Expand All @@ -106,6 +100,15 @@ impl<'buf> Table<'buf> {
},
error_kind,
};

let offset = self
.vtable
.get(2 * vtable_offset..2 * (vtable_offset + 2))
.ok_or_else(|| make_error(ErrorKind::InvalidOffset))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

While looking into adding a few extra tests for this, I found out that this should only sometimes be considered an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot! Awesome! Yeap feel free to close this one and focus on the improved version. 🚀


let tag_offset = u16::from_le_bytes(offset[..2].try_into().unwrap()) as usize;
let value_offset = u16::from_le_bytes(offset[2..].try_into().unwrap()) as usize;

let tag = u8::from_buffer(self.object, tag_offset).map_err(make_error)?;
if tag_offset != 0 && value_offset != 0 && tag != 0 {
T::from_buffer(self.object, value_offset, tag)
Expand Down
2 changes: 2 additions & 0 deletions test/rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ pub mod planus_test;

pub mod hexdump;

mod table_reader;

#[cfg(test)]
pub mod tests {
use std::path::Path;
Expand Down
43 changes: 43 additions & 0 deletions test/rust/src/table_reader.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
use planus::{SliceWithStartOffset, TableReadUnion};

#[derive(Debug, PartialEq, Eq)]
struct A {
tag: u8,
offset: usize,
}

impl<'buf> TableReadUnion<'buf> for A {
fn from_buffer(
_: SliceWithStartOffset<'buf>,
offset: usize,
tag: u8,
) -> core::result::Result<Self, planus::errors::ErrorKind> {
Ok(Self { tag, offset })
}
}

#[test]
fn access_union() {
use planus::table_reader::Table;
let data: Vec<u8> = vec![
8, 0, 0, 0, // vtable size
4, 0, 1, 0, // vtable with tag offset (u16) and value offset (u16)
4, 0, 0, 0, // object size (u32)
12, 0, 0, 0, // vtable offset (i32)
1, 0, 0, 0, // tag
];
let data = SliceWithStartOffset {
buffer: &data,
offset_from_start: 0,
};
let table = Table::from_buffer(data, 8).unwrap();

// vtable has 1 entry
// => accessing index 0 must be ok
assert_eq!(
table.access_union::<A>(0, "", "").unwrap(),
Some(A { tag: 1, offset: 1 })
);
// => accessing index 1 must error
assert!(table.access_union::<A>(1, "", "").is_err())
}