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

Conversation

jorgecarleitao
Copy link
Contributor

@jorgecarleitao jorgecarleitao commented Jun 12, 2022

Checklist

  • Updated CHANGELOG.md with relevant changes
  • Added tests for any new/fixed functionality
  • Added/updated documentation for new/changed code
  • Checked that README.md still makes sense (and updated it if necessary)

Closes #99

Copy link
Collaborator

@TethysSvensson TethysSvensson left a comment

Choose a reason for hiding this comment

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

I think this is a great PR, however after adding more tests locally I found out that it still had some problems (that originated in my bad suggestions on #99).

I'm making an updated PR that contains a rebase of your commit, so I think we should close this one.

@@ -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 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. 🚀

@TethysSvensson
Copy link
Collaborator

Closing as this was included in #102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic on invalid flatbuffers
2 participants