Skip to content

Fix some offsets computation when using scroll #471

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DreydenGys
Copy link

@DreydenGys DreydenGys commented Jun 8, 2025

While I was working on a personal project using both goblin and scroll, I found some bugs in the way offset are computed for both DataDirectories and OptionalHeader.
Here some minor fixes without breaking changes.

While going through the code I notice that most parsing methods are present twices:

  • one implementation with the scroll traits
  • one using the method parse of each structs

Is there a reason why theses two implementations cohabit? Using only one could reduce bugs and improve maintainability.

@DreydenGys DreydenGys marked this pull request as draft June 8, 2025 16:34
@DreydenGys DreydenGys marked this pull request as ready for review June 8, 2025 16:43
Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

I’d like to see a test that fixes whatever bug you had, (which fails today without this patch), otherwise I’m not sure exactly what this fixes for you, can you go into more detail? This appears to only change the pwrite impls which won’t affect the parser iiuc?

@@ -628,7 +631,7 @@ impl ctx::TryIntoCtx<scroll::Endian> for OptionalHeader {
bytes.gwrite_with(self.windows_fields, offset, ctx)?;
bytes.gwrite_with(self.data_directories, offset, ctx)?;
}
_ => panic!(),
n => Err(Self::Error::BadMagic(n.into()))?,
Copy link
Owner

Choose a reason for hiding this comment

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

I’m not sure bad magic is the correct error here, I’m sure there’s something more applicable, but thank you for getting rid of this panic, that’s bad?

Copy link
Author

Choose a reason for hiding this comment

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

I used the same as the one used in src/pe/optional_headers.rs:597 and src/pe/optional_headers.rs:576 to be consistent.

@DreydenGys DreydenGys force-pushed the fix-pe-scroll branch 2 times, most recently from 2d1d56a to 639b23b Compare June 9, 2025 10:08
@DreydenGys
Copy link
Author

I noticed that for now, the only reason we rely on the method DataDirectories::parse is that we take the count parameters taken from the optional header. The thing is that no matter what we will construct an array of 16 (NUM_DATA_DIRECTORIES) internally. However as we don't store this count number internally, we will write the 16 entries when using TryIntoCtx.
We should either store this count if we know that PE files can have less than 16 entries or consider that it will always be 16 and transform the DataDirectories::Parse implementation a TryFromCtx without the count parameter.

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.

2 participants