pe: skip raw writes for virtual-only sections - #541
Open
mq1n wants to merge 1 commit into
Open
Conversation
PE sections such as .bss can reserve virtual memory without carrying raw on-disk data. The writer previously expanded those virtual bytes and could write zeroes at file offset 0, corrupting the DOS/PE headers during rewrite. Treat zero-raw-size sections as virtual-only, preserve their section table entries, and skip writing nonexistent section bytes. The regression coverage uses synthetic section data so the test does not require a bundled executable fixture. Constraint: Virtual-only PE sections may legally have SizeOfRawData == 0 and PointerToRawData == 0
mq1n
marked this pull request as ready for review
June 17, 2026 19:43
Owner
|
@kkent030315 if you could review would appreciate :) |
m4b
reviewed
Aug 30, 2026
| let mut output = vec![HEADER_SENTINEL; SECTION_TABLE_OFFSET + 0x80]; | ||
| let mut section_table_offset = SECTION_TABLE_OFFSET; | ||
|
|
||
| pe.write_sections(&mut output, &mut section_table_offset, None, scroll::LE) |
Owner
There was a problem hiding this comment.
it's also generally a good idea for a test to "round trip", e.g, you write a PE file, and then we can parse it back again
|
|
||
| let section_start: usize = self.pointer_to_raw_data.try_into().map_err(|_| { | ||
| Error::Malformed(format!("Virtual address cannot fit in platform `usize`")) | ||
| Error::Malformed("Virtual address cannot fit in platform `usize`".to_string()) |
Owner
There was a problem hiding this comment.
generally this is nice but it is technically diff noise (and not related to your PR)
Comment on lines
+355
to
+360
| let section = SectionTable { | ||
| pointer_to_raw_data: 4, | ||
| size_of_raw_data: 4, | ||
| virtual_size: 4, | ||
| ..SectionTable::default() | ||
| }; |
| let section_end: usize = section_start | ||
| + usize::try_from(self.size_of_raw_data).map_err(|_| { | ||
| Error::Malformed(format!("Virtual size cannot fit in platform `usize`")) | ||
| Error::Malformed("Virtual size cannot fit in platform `usize`".to_string()) |
| let original_bytes = pe_bytes.get(section_start..section_end).map(Cow::Borrowed); | ||
| let original_bytes = match pe_bytes.get(section_start..section_end) { | ||
| Some(bytes) => bytes, | ||
| None => return Ok(None), |
Owner
There was a problem hiding this comment.
I think this changes the semantics; you now return none if we can't get the original bytes, but before it would return Ok(original_bytes) if the is_some() check did not pass. Is this intentional or refactor artifact? Could you give some motivation about why this change was made?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SizeOfRawData == 0as virtual-only on diskRoot cause
Some PE files, including the local Free Pascal sample used to reproduce this, contain a
.bsssection with a nonzero virtual size but no raw on-disk data. The writer previously expanded that virtual-only section into zero bytes and wrote them atPointerToRawData == 0, which could corrupt the DOS/PE headers.Validation
cargo fmt --checkcargo testfreepascal_cli.exerewrite remained byte-identical by SHA256Notes
This is a narrow PE writer fix and does not special-case Free Pascal binaries. The regression test uses synthetic section data instead of adding a binary executable fixture.