Fix for resource data alignment.

[tc-hib]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

[tc-hib]

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

[akavel]

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 :disappointed: 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]

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.

[akavel]

@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]

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]

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]

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....