Fix offset issue when reading StringFileInfo

[dmjb]

I ran into an issue while using this library to read version info from certain PE files. My main test case has been the exe of Powershell 7.3.4, but have found the same issue in other binaries. Using the latest revision of this library, reading the version info from pwsh.exe would lead to the following truncated and corrupted output:

{
  "\u001b\u0001LegalCopyright": "",
  "Comments": "PowerShell on Windows top-level project",
  "CompanyName": "Microsoft Corporation",
  "FileDescription": "pwsh",
  "FileVersion": "7.3.4.500",
  "InternalName": "pwsh.dll"
}

Debugging the code, I discovered the issue appears to stem from the version parsing code's handling of alignment. The ParseVersionResources function keeps track of a stringOffset where it expects to find the next string, and passes it to the parseString function. The parseString function calcuates its own offset into the file based on the stringOffset passed to it, aligns the new offset, and then reads the string from the file using the aligned offset. parseString then returns the length of the string to ParseVersionResources, where it is added to the stringOffset to get the expected location of the next string.

However, the length of the padding is not included in this calculation. As such, when ParseVersionResources adds the string length to its offset, it ends up being a few bytes short of where it should be, since it is adding the length of the string to a non aligned offset. As long as the difference between the expected location and the actual location is not a a multiple of four, the alignment process inside parseString ends up fixing the problem. However in the case of the pwsh.exe binary I was examining, there were two cases where the difference between the expected offset and the actual offset was exactly four bytes. As a result, the alignment did not adjust the offset, and the code attempted to read the string four bytes short of where it should have, resulting in junk data being loaded.

To fix this, I added a new private offset calculation function which returns the aligned offset, as well as the number of bytes added to achieve alignment. This latter value is added to the string length before being returned to ParseVersionResources, and gives the following correct result:

{
  "Assembly Version": "7.3.4.500",
  "Comments": "PowerShell on Windows top-level project",
  "CompanyName": "Microsoft Corporation",
  "FileDescription": "pwsh",
  "FileVersion": "7.3.4.500",
  "InternalName": "pwsh.dll",
  "LegalCopyright": "(c) Microsoft Corporation.",
  "OriginalFilename": "pwsh.dll",
  "ProductName": "PowerShell",
  "ProductVersion": "7.3.4 SHA: b59f05d5a1b2fceca231f75c53c203a02edf6203"
}

I have written a test case around the Powershell binary, but it seems the gitignore rules prevent the inclusion of .exe files. It's not clear to me if I should try and force-add the executable. The binary is relatively small (about 200-odd kb), freely available and is open source software. However, I will defer to the maintainers of this project if they want the binary and the test case included.

[LordNoteworthy]

LGTM ! Thanks for the catch @dmjb 👍

I just checked, the pwsh.exe binary file size is not very big that git will complain about it, so you can check in the binary the test folder.

And kudos for the detailed description.

[dmjb]

Thank you for your review @LordNoteworthy. I have included the test file and uncommented out my test case. The tests pass locally in my environment:

--- PASS: TestParseVersionResources (0.05s)
    --- PASS: TestParseVersionResources//Users/donbrowne/src/dmjb/pe/test/putty.exe (0.02s)
    --- PASS: TestParseVersionResources//Users/donbrowne/src/dmjb/pe/test/brave.exe (0.02s)
    --- PASS: TestParseVersionResources//Users/donbrowne/src/dmjb/pe/test/impbyord.exe (0.00s)
    --- PASS: TestParseVersionResources//Users/donbrowne/src/dmjb/pe/test/WdBoot.sys (0.00s)
    --- PASS: TestParseVersionResources//Users/donbrowne/src/dmjb/pe/test/shimeng.dll (0.00s)
    --- PASS: TestParseVersionResources//Users/donbrowne/src/dmjb/pe/test/pwsh.exe (0.00s)
PASS
ok  	github.com/saferwall/pe	(cached)

[LordNoteworthy]

Amazing ! Thanks a lot for the contribution.

Merging it.

FYI @rabbitstack

[LordNoteworthy]

Tag v1.4.3 contains the fix.

[rabbitstack]

Thanks for the fix @dmjb and @LordNoteworthy for the heads-up!