fix(pe): correct SizeOfImage when appending a resource section - #79
Merged
Conversation
Embedding a resource rebuilds the PE and appends a new section holding the
resource directory. SizeOfImage was updated with `size_of_image +=
new_section_data.len()`, i.e. the *unaligned raw* length of the appended
resource data, which under-counts the image by up to one SectionAlignment
page (and was not updated at all when an existing last section was merely
extended). Per the PE spec SizeOfImage must cover the highest section's
VirtualAddress + VirtualSize rounded up to SectionAlignment.
On Windows an undersized SizeOfImage leaves the tail of the resource section
unmapped, which surfaces either as an access violation at load time or as the
runtime failing to locate the embedded section ("Could not find standalone
binary section", reported against `deno compile --icon`).
The PE resource writer was previously the third-party, unmaintained `editpe`
crate. Reduce the small part libsui actually uses into an owned module
(pe_edit.rs) and fix SizeOfImage there by recomputing it from the final
section table. BSD-2-Clause attribution is retained in LICENSE-editpe.
Add cross-platform regression tests that build a PE in memory and assert
SizeOfImage is SectionAlignment-aligned and covers every section, for small,
page-unaligned, large, and --icon resource payloads.
The branch that expands an existing (last) resource section computed the virtual_size delta after already mutating size_of_raw_data to the new size, so the delta was zero and virtual_size never grew. That leaves SizeOfImage under-covering an extended-in-place section, the same class of bug this change fixes elsewhere. Assign the grown sizes directly, matching the truncate and append-new-section paths. Inherited verbatim from editpe. Not on deno's compile path (a normal base binary's .rsrc is not the last section, so the append path is taken), but the branch is reachable for other inputs libsui now owns. Caught in review.
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
1 participant
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.
Embedding a resource into a PE rebuilds the image and appends a new section
holding the resource directory. The writer updated the optional header with
size_of_image += new_section_data.len(), adding the unaligned raw length ofthe appended resource data. That under-counts SizeOfImage by up to one
SectionAlignment page, and in the branch that merely extends an existing last
section it was not updated at all. Per the PE spec, SizeOfImage must cover the
highest section's VirtualAddress + VirtualSize rounded up to SectionAlignment.
On Windows an undersized SizeOfImage leaves the tail of the appended resource
section outside the mapped image. Depending on how much is clipped and how
strict the loader is, this shows up either as an access violation at load time
before any user code runs, or as the runtime failing to find the embedded
section at all. The latter is what surfaces as "Could not find standalone
binary section" from
deno compile --icon, where the icon bitmaps make theresource directory large enough to push its tail past the truncated SizeOfImage.
Both were reported downstream as denoland/deno#36206 and denoland/deno#36238;
2.9.2 worked and 2.9.3 did not, purely because the base binary's layout shifted
enough to start clipping, with no change to this code.
The resource writer was the third-party
editpecrate (0.1.0, unmaintained),pulled in only for its PE resource path. This reduces the part libsui actually
uses into an owned module, pe_edit.rs, dropping the resource parsers,
icon/version/manifest readers and the second
imagedependency, and fixesSizeOfImage there by recomputing it from the final section table rather than
incrementing it. The reduced code keeps editpe's serialization verbatim; its
BSD-2-Clause license and attribution are retained in LICENSE-editpe.
The regression tests build a PE in memory and inspect the resulting headers, so
they run on every platform rather than only Windows. They assert that
SizeOfImage is a multiple of SectionAlignment and covers every section's virtual
extent, across small, page-unaligned, large, and
--iconresource payloads.Reverting the fix fails all four with "SizeOfImage ... is not a multiple of
SectionAlignment".
A libsui version bump for release is left as a separate commit per the repo's
convention.