Fix for resource data alignment. #32

tc-hib · 2020-12-05T11:32:01Z

Probably fixes #12 and #26

Resource data has to be aligned, otherwise Windows and other tools might not read them properly.
I checked that with Resource Hacker, UPX and Windows.
windres does align resources too.

People are starting to use my fork, which is wrong because I'd never maintain it, so I'm making this pull request even though I'm not sure my fix is "right". I hope you'll find some time to review and pull it.

Thanks.

akavel · 2020-12-06T19:51:53Z

Wow, thanks! Is there a chance you could add one tiny thing to this PR - one line in AUTHORS with your preferred name and email? I'd love to immediately merge it then ❤️

tc-hib · 2020-12-06T22:26:20Z

Thank you, I didn't think such a small fix would deserve credits.
I won't write my e-mail address because I already receive enough spam :)

akavel · 2020-12-06T22:26:27Z

Thank you! Hmmm, interesting: I added some simple test, hoping to try and make merging easier by testing PRs - but the test seems to fail with this PR :( any idea what could be going on?

I intended to merge this, but now it doesn't seem so straightforward :( sorry for that :(

tc-hib · 2020-12-06T23:01:16Z

My PR worked with goversioninfo.
It looks like I've missed something because in your test my version adds several bytes but the file size doesn't change!

akavel · 2020-12-06T23:15:41Z

Hm, I searched for where Size is used, and it seems also called in StringsHeader (in NewRDATA and NewRSRC), I guess it's possible this may be relevant too? (I don't really remember what my code does anymore, so I'm also going blind here 😅 ) Hm, also coff.AddData maybe? or not?

tc-hib · 2020-12-07T01:12:37Z

I've fixed it, though I am not comfortable enough with the code to say it will work in any case...
You should add more tests. Your icon is already aligned.
I can provide an icon which is not aligned.

tc-hib · 2020-12-07T07:29:45Z

Hello,
Yesterday I pushed in a hurry. My fix was uncomplete. Now it should be better :)
My original fix was working by accident because I've always had and icon group that was naturally aligned on 4 bytes, and I only switched to 8 bytes for this PR.

tc-hib · 2020-12-07T08:02:01Z

Aligned the icon group too.
Should be OK now, but I've got to get back to my real work so I'll have more tests tonight.

akavel

I continue to have my breath taken away by amazement at your will to help. Thanks again! All the comments here are fully optional on your side, I'm perfectly OK with tweaking the PR myself if you care enough to put in some cool test case/cases (esp. with unaligned data).

binutil/walk.go

binutil/writer.go

akavel · 2020-12-07T22:35:21Z

binutil/writer.go

@@ -6,6 +6,8 @@ import (
 	"reflect"
 )

+var pad [8]byte


[optional] I'd prefer this to be declared locally only where it's used (i.e. above l.39), if possible, to minimize the context a person reading this codebase has to keep in mind

rsrc/rsrc.go

akavel · 2020-12-07T23:00:32Z

binutil/sizedfile.go

@@ -17,6 +17,7 @@ type SizedFile struct {

 func (r *SizedFile) Read(p []byte) (n int, err error) { return r.s.Read(p) }
 func (r *SizedFile) Size() int64                      { return r.s.Size() }
+func (r *SizedFile) AlignedSize() int64               { return Align(r.s.Size()) }


[optional] I'm tempted to try introducing just a single helper struct:

struct Aligned { SizedReader } func (a *Aligned) Padding() []byte { s := a.Size() aligned := (s-1)&^7 + 8 pad := [8]byte{} return pad[aligned-s] }

and special-casing it where needed (with len(a.Padding()) when just size is needed). Then wrapping any objects that need to be aligned at the place where they're used - instead of adding .AlignedSize() to SizedFile and __GRPICONDIR. I would assume it's the use site that determines if we need to align a particular object, instead of alignment being the object's intrinsic quality. But I'm more than happy to take a stab at this refactoring once you're done with your awesome work and once you've added some tests. I'm thus adding this comment more as a note to myself to not forget about that idea later.

Yes, I agree too, my fix is illogical for sure.
I've never taken time to fully understand the code, as I am uncomfortable with its generic nature (and I'm new to Go).
It is still unclear to me where the call site is, so I'd rather let you fix that :)

(should I click on "Resolve conversation" ? I'm new to github too!)

akavel · 2020-12-07T23:19:37Z

Also, sorry for the tests failing, I forgot to commit testdata/manifest.xml 🤦 hopefully the new commit in master ameliorates that.

tc-hib · 2020-12-09T11:45:53Z

OK, sorry, I've just understood there was a Commit suggestion I was supposed to click

akavel · 2020-12-09T21:30:43Z

Don't worry too much about my suggestions above now :D honestly, the main thing I'm kinda unsure now if you're still planning to add the tests you mentioned, with some unaligned files maybe? or not? that would be really great if you had some sweet reproducer that fails without your fixes that you could share! :)

EDIT: Ah, though I only now just realized, that given the existing tests pass, it should be ok to merge this PR anyway. Sorry that it took me so long 😞 That said, if you'd be able to give some test files/cases, that would be really cool, as it would help us make sure this doesn't break again at some point in the future!

tc-hib · 2020-12-10T08:34:42Z

I'm a little busy at work, I'll try to test this a little more at the end of the week.
I can provide icons that produce bugs, but I don't know how to automate tests, maybe using command line tools to compare the output (windres) or to extract the resources. go build won't complain on unaligned resources.

Probably fixes #12 and #26

Co-authored-by: Mateusz Czapliński <[email protected]>

akavel · 2020-12-10T09:05:07Z

@tc-hib got it, and totally understand. I am also not really sure how to test it well, that's part of my problem with this whole app now, so I don't expect you to do that and magically fix my problems for me 😆 it was honestly just a question, given that you wrote "I will add tests", I got kinda excited 😁 anyway now that I realized I can merge this anyway, I'm gonna merge this anyway, just will try to take a bit more time to refactor it slightly to be more comfortable with that. Hopefully today evening. If not, I'll probably merge it as-is anyway. In the meantime I'm gonna push a somewhat rebased PR to cleanup the history a bit. According to the diffs I did afterwards, it seems to me I didn't lose anything from this PR while rebasing 😅 So, if you manage to find some time at any point to try and wrestle a bit with my problems of testing, that'd be more than awesome 😁 if not, I totally understand it, and for sure am already immensely grateful for your contribution here.

tc-hib · 2020-12-10T09:15:57Z

I forgot to fix the silly formula I used to align:
I wrote (s-1)&^7 + 8 (was following some reasoning i guess)
But it is equivalent to (s+7)&^7

Can you please fix it before merging? Thanks.

akavel · 2020-12-10T17:44:46Z

Could you send me the sample icons that produce bugs for you? and also if possible, some link describing what exactly needs to be aligned, if you found one? or did you track it by manually comparing rsrc output with windres output, similar as I did originally? I'm trying to run upx locally as part of the tests, and it does fail for (only) the manifest & icon case without your fix; but more of them would be useful, and also I now see that the padding you're adding is more complex than I thought, so if you have some links that I could learn from and maybe try to grasp it for myself, and hopefully find a way to thus simplify, I'd be grateful; for now, I think I'll take the risk and merge as is.

akavel · 2020-12-10T18:36:19Z

Ehhh, ok, I found your highly informative comments from May... now that took me some time didn't it.... so based on your note that "in resource data entry you should only find offsets that are multiples of 4", I think I'll try to simplify this code to just do that... seems to help in the manifest & icon case with upx... maybe hopefully that will be enough? 🤞 ....... this approach would be much more "proper" if it works.... please do note that without the immense help you've provided via this PR and patience and perseverance and discussion, I'd absolutely not be able to do that!!! and also that your further testcases would also be super helpful still....

@tc-hib

Simpler approach to achieve the main goal of #32. Based esp. on a comment by @tc-hib: > in [resource data entry]( > https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#resource-data-entry) > you should only find offsets that are multiples of 4. (#12 (comment)) HUUUUUUGE THANKS to @tc-hib, for his work with PR #32, for his patience, perseverance and replies, and for the invaluable comment quoted above.

tc-hib changed the title ~~Quick and dirty fix for resource data alignment.~~ Fix for resource data alignment. Dec 7, 2020

tc-hib mentioned this pull request Dec 7, 2020

Fix alignment of resource data (can only work with next version of rsrc) josephspurrier/goversioninfo#47

Closed

akavel reviewed Dec 7, 2020

View reviewed changes

tc-hib and others added 10 commits December 10, 2020 09:49

Quick and dirty fix for resource data alignment.

9547050

Probably fixes #12 and #26

Credit myself

ac911da

Fix offset when unaligned data is written (_GRPICONDIR for example).

78bbfc2

Ooops, fix my fix

3175c91

Align icon group too

855d754

Align icon group too

31af992

Update binutil/walk.go

ba893ed

Co-authored-by: Mateusz Czapliński <[email protected]>

Update binutil/writer.go

3beff5a

Co-authored-by: Mateusz Czapliński <[email protected]>

Update rsrc/rsrc.go

39fd24b

Co-authored-by: Mateusz Czapliński <[email protected]>

refactor: make pad a local var, not global

1bbd5bb

akavel merged commit 1bbd5bb into akavel:master Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for resource data alignment. #32

Fix for resource data alignment. #32

tc-hib commented Dec 5, 2020 •

edited

Loading

akavel commented Dec 6, 2020 •

edited

Loading

tc-hib commented Dec 6, 2020

akavel commented Dec 6, 2020 •

edited

Loading

tc-hib commented Dec 6, 2020

akavel commented Dec 6, 2020

tc-hib commented Dec 7, 2020 •

edited

Loading

tc-hib commented Dec 7, 2020

tc-hib commented Dec 7, 2020

akavel left a comment

akavel Dec 7, 2020

tc-hib Dec 8, 2020

akavel Dec 7, 2020

tc-hib Dec 9, 2020 •

edited

Loading

akavel commented Dec 7, 2020

tc-hib commented Dec 9, 2020

akavel commented Dec 9, 2020 •

edited

Loading

tc-hib commented Dec 10, 2020

akavel commented Dec 10, 2020

tc-hib commented Dec 10, 2020 •

edited

Loading

akavel commented Dec 10, 2020

akavel commented Dec 10, 2020

Fix for resource data alignment. #32

Fix for resource data alignment. #32

Conversation

tc-hib commented Dec 5, 2020 • edited Loading

akavel commented Dec 6, 2020 • edited Loading

tc-hib commented Dec 6, 2020

akavel commented Dec 6, 2020 • edited Loading

tc-hib commented Dec 6, 2020

akavel commented Dec 6, 2020

tc-hib commented Dec 7, 2020 • edited Loading

tc-hib commented Dec 7, 2020

tc-hib commented Dec 7, 2020

akavel left a comment

Choose a reason for hiding this comment

akavel Dec 7, 2020

Choose a reason for hiding this comment

tc-hib Dec 8, 2020

Choose a reason for hiding this comment

akavel Dec 7, 2020

Choose a reason for hiding this comment

tc-hib Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

akavel commented Dec 7, 2020

tc-hib commented Dec 9, 2020

akavel commented Dec 9, 2020 • edited Loading

tc-hib commented Dec 10, 2020

akavel commented Dec 10, 2020

tc-hib commented Dec 10, 2020 • edited Loading

akavel commented Dec 10, 2020

akavel commented Dec 10, 2020

tc-hib commented Dec 5, 2020 •

edited

Loading

akavel commented Dec 6, 2020 •

edited

Loading

akavel commented Dec 6, 2020 •

edited

Loading

tc-hib commented Dec 7, 2020 •

edited

Loading

tc-hib Dec 9, 2020 •

edited

Loading

akavel commented Dec 9, 2020 •

edited

Loading

tc-hib commented Dec 10, 2020 •

edited

Loading