Skip to content

changes to fix PE header corruption when inserting exception stack tr…#190

Open
eivindbakkestuen wants to merge 3 commits into
project-jedi:masterfrom
eivindbakkestuen:fix/jcldebug-sizeofheaders-signtool
Open

changes to fix PE header corruption when inserting exception stack tr…#190
eivindbakkestuen wants to merge 3 commits into
project-jedi:masterfrom
eivindbakkestuen:fix/jcldebug-sizeofheaders-signtool

Conversation

@eivindbakkestuen

Copy link
Copy Markdown

Fix JCLDEBUG insertion producing a PE that signtool rejects (0x800700C1)

When the header area lacks room for the new JCLDEBUG section header,
InsertDebugDataIntoExecutableFile grows it but never updated
OptionalHeader.SizeOfHeaders, and gave .bss/.tls a bogus PointerToRawData.
The loader tolerates this; signtool rejects it with ERROR_BAD_EXE_FORMAT.

  • JclDebug.pas: CheckHeadersSpace updates SizeOfHeaders and is now called
    from both the 32-bit and 64-bit paths (32-bit had no header room check);
    MovePointerToRawData skips zero-raw sections; recompute PE checksum.
  • JclPeImage.pas: same fix in generic PeInsertSection32/64 via a shared
    InsertHeaderSpace helper.

Co-Authored-By: Claude Opus 4.8 (1M context)

Comment thread jcl/source/windows/JclDebug.pas Outdated
Comment thread jcl/source/windows/JclDebug.pas
Comment thread jcl/source/windows/JclDebug.pas
Comment thread jcl/source/windows/JclDebug.pas
Comment thread jcl/source/windows/JclPeImage.pas
Comment thread jcl/source/windows/JclPeImage.pas Outdated
- JclDebug: rename CheckHeadersSpace -> AdjustHeadersSpace (it now adjusts
  SizeOfHeaders through a var parameter)
- JclDebug: wrap the MovePointerToRawData loop body in begin/end so the
  zero-raw-section comment and guard are clearly inside the loop
- JclDebug: add blank lines before the SizeOfHeaders and checksum comment blocks
- JclPeImage: add the mandatory blank line before InsertHeaderSpace
- JclPeImage: factor the duplicated 32/64-bit header-grow blocks into a single
  nested helper EnsureSectionHeaderSpace. The section table is at the same file
  offset for both targets, so the 32-bit mapping helpers locate it for both;
  only the per-target SizeOfHeaders bump stays in the caller.

No behavior change. Verified compiling under dcc32/dcc64, and that the grow path
still yields signtool-acceptable 32- and 64-bit images (a faithful pre-fix image
is still rejected with 0x800700C1).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eivindbakkestuen

Copy link
Copy Markdown
Author

an updated commit has been pushed.

@eivindbakkestuen eivindbakkestuen requested a review from obones June 7, 2026 23:16
@eivindbakkestuen

Copy link
Copy Markdown
Author

Is there something more I need to look at to make the pull request pass? I can't work it out here; there's "1 requested change" above but it doesn't display anything useful to me.

@ronaldhoek

Copy link
Copy Markdown
Contributor

Is there something more I need to look at to make the pull request pass? I can't work it out here; there's "1 requested change" above but it doesn't display anything useful to me.

One of the change requests made by @obones has not been marked as resolved.

@obones

obones commented Jun 8, 2026

Copy link
Copy Markdown
Member

The remaining change requested appears to have been done even if Github does not display it properly.
But I have left it open because I need more time to assess it

And please remember, this is a volunteer based effort there, so delays are expected.

@obones

obones commented Jun 10, 2026

Copy link
Copy Markdown
Member

This looks good now.
Please rebase onto master and then I'll merge it

@eivindbakkestuen

Copy link
Copy Markdown
Author

I've tried rebasing, git tells me everything up-to-date. above it says "No conflicts with base branch Changes can be cleanly merged.". Googled for other instructions, not sure where to go from here. :(

@ronaldhoek

Copy link
Copy Markdown
Contributor

This looks good now. Please rebase onto master and then I'll merge it

Why rebase? The merged commit will normally be placed on top of the current master. So you should get a normal Git history.

@obones

obones commented Jun 11, 2026

Copy link
Copy Markdown
Member

This looks good now. Please rebase onto master and then I'll merge it

Why rebase? The merged commit will normally be placed on top of the current master. So you should get a normal Git history.

Because I prefer having branches not crossing each other.
Right now, this would give this:

   /--*--\
--*---*--*--*--
      \--*--/

While I prefer this:

   /--*--\/--*--\
--*------*------*--

I'm really missing the "rebase" button that I get with Gitlab, and the "rebase and merge" option here at Github is misleading, it's doing "rebase and fastforward" thus not creating the merge commit that I'd want.

@ronaldhoek

Copy link
Copy Markdown
Contributor

I've tried rebasing, git tells me everything up-to-date. above it says "No conflicts with base branch Changes can be cleanly merged.". Googled for other instructions, not sure where to go from here. :(

You need to rebase on the 'master' from the original JCL repo not the master of your own fork (or first fast-foreward your fork and then rebase)

image

@obones

obones commented Jun 11, 2026

Copy link
Copy Markdown
Member

I've tried rebasing, git tells me everything up-to-date. above it says "No conflicts with base branch Changes can be cleanly merged.". Googled for other instructions, not sure where to go from here. :(

That's how I do it:

  1. Check out "master"
  2. Fetch "upstream/master" (provided you created the upstream origin that points to the official JCL repo)
  3. Rebase "master" onto "upstream/master" (this gives a fast forward)
  4. Push to "origin/master" (that's your fork)
  5. Checkout your MR branch
  6. Rebase your branch onto master
  7. Push (--force-with-lease) your branch to origin

Steps 2 and 3 are done automatically with TortoiseSVN when using the "Pull" dialog and checking the "Launch rebase after fetch" check box at the bottom:

image

@ronaldhoek

Copy link
Copy Markdown
Contributor

You can easily sync your 'master' of your fork in Github using

Before
image

After
image

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.

4 participants