Skip to content

Bugfix: devtools/symbol-check: Check PE libraries case-insensitively#18490

Closed
luke-jr wants to merge 1 commit intobitcoin:masterfrom
luke-jr:bugfix_symcheck_pe_case
Closed

Bugfix: devtools/symbol-check: Check PE libraries case-insensitively#18490
luke-jr wants to merge 1 commit intobitcoin:masterfrom
luke-jr:bugfix_symcheck_pe_case

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 1, 2020

For some reason, one of my gitian builds failed because "DLL" was uppercase where symbol-check.py expected it lowercase.

Since the PE platform is case insensitive, check it case-insensitively here too.

@luke-jr luke-jr changed the title Bugfix: Check PE libraries case-insensitively Bugfix: devtools/symbol-check: Check PE libraries case-insensitively Apr 1, 2020
@luke-jr luke-jr force-pushed the bugfix_symcheck_pe_case branch from d117685 to b75b64f Compare April 1, 2020 05:42
@fanquake
Copy link
Member

fanquake commented Apr 1, 2020

For some reason, one of my gitian builds failed because "DLL" was uppercase

Which version of Core/descriptors were you building? Which DLL was different?

Checking case insensitively was bought up in #18395, but we didn't see a reason to do it, given that in gitian, everything is set. If DLLs are being randomly named during gitian builds, that not only sounds like a reproducibility issue, but something that should be reported upstream. So it would be good to get details.

Checking insensitively would also seem to introduce the scenario where we could be linked against multiple DLLs, and the script wouldn't fail. I'd rather we check everything exactly, and get notified if the underlying DLLs change.

@laanwj
Copy link
Member

laanwj commented Apr 1, 2020

Yes, this is a determinism issue. So NACK on this.
Where does the DLL name come from? Must be either\

  1. The name directly specified on linking
  2. Encoded in the .LIB import library
  3. It uses the name of the DLL that happens to be on disk

(3) would be really bad (in this case we should report/fix the upstream as @fanquake suggests), in the other two cases, it shouldn't diverge on deterministic input.

@luke-jr
Copy link
Member Author

luke-jr commented Apr 1, 2020

Which version of Core/descriptors were you building?

A rebased #14137, with QtWinExtras split to a separate depends package.

Which DLL was different?

IMM32 and WINMM

Checking insensitively would also seem to introduce the scenario where we could be linked against multiple DLLs, and the script wouldn't fail.

What do you mean?

@fanquake
Copy link
Member

fanquake commented Apr 9, 2020

NACK

A rebased #14137, with QtWinExtras split to a separate depends package.

So nothing in master is broken, and the security checks are working as expected. If you're building code that isn't in this repository (that also modifies dependencies), I don't think there's an expectation that our symbol/security checks should pass.

Does this mean you're planning on reopening #17213 (the rebased version of #14137)? That was closed due to lack of interest, and I still don't think it's something worthwhile merging. Regardless, if changes need to be made to scripts to accommodate that, they should be part of that PR.

What do you mean?

I mean there's no point being lenient in these scripts if we don't need to be. As mentioned by @laanwj, given we control the input, and outputs are deterministic (otherwise there's an actual bug), I'm happy for us to keep checking for the exact libraries we expect. Even though in reality it may not be possible, there's no need to allow the output to contain a a.dll and/or a.DLL, and I'd rather the build fail if the output from gitian suddenly changes.

@fanquake
Copy link
Member

Going to close this. There are two NACKs, and I haven't seen an argument to change the current behaviour.

@fanquake fanquake closed this Apr 15, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants