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

Remove check on debug directory size #313

Merged
merged 1 commit into from
Jun 5, 2022
Merged

Remove check on debug directory size #313

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
4 changes: 2 additions & 2 deletions src/pe/debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ impl<'a> CodeviewPDB70DebugInfo<'a> {

// calculate how long the eventual filename will be, which doubles as a check of the record size
let filename_length = idd.size_of_data as isize - 24;
if filename_length < 0 || filename_length > 1024 {
// the record is too short or too long to be plausible
if filename_length < 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

Is there no maximum bound to check against ? (Worried about regressions if removing this)

also curious the number 1024 was used, I wonder why that was chosen ? (Haven’t looked at this code in a while)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1024 is an odd choice - MAX_PATH + some small constant factor (to include the hash and other data in the section) seems reasonable, but 1024 is a bit bigger than I'd expect for the typical size of the directory.

That said, I don't think there is a maximum bound - and as this crate is typically used, the memory is already mapped/allocated, so it's mostly harmless to remove the check.

// the record is too short to be plausible
return Err(error::Error::Malformed(format!(
"ImageDebugDirectory size of data seems wrong: {:?}",
idd.size_of_data
Expand Down