1370611 - Ban the usage of GetFileVersionInfoSize() on m-c due to it silently loading DLLs under the hood

Status: NEW

(Core :: General, enhancement)

Description

[(no longer active)]

This API is a performance disaster, see bug 1370609, bug 1358369 and I'm sure there are others.

We should go through our usages of this API and remove every single last one of them instead of waiting to find these bugs one by one.

Comment 1

[Marco Castelluccio [:marco]]

I think we are using this for blocklisting purposes (both DLL blocklist and DXVA blocklist).

We are also using this in the "modules" ping introduced by bug 1330833, but that's only collected once a week.

Comment 2

[(Away)]

I think the tone here is a bit harsh, and declaring GetFileVersionInfoSize forbidden may not be solving the real problem.

Bug 1370609 is a great find because (IIUC) that code should be able to avoid version-checking altogether, whether through GetFileVersionInfo or any other means.

But, if there are callers where you really _must_ read the file header, it's gonna show up on some profile somewhere, whether it's from GetFileVersionInfo or from some custom API replacement we try to write.

Also in some cases the cost may not be so high: in the DLL blocklist, we're only going to call the API if the filename is on the blocklist, and either we deny the load (and save a bunch of cycles!) or we go on to do a full LdrLoadDll anyway, much more costly than the GetFileVersion, and hopefully gaining cache benefits from having just touched that file.

I think a better approach to this bug would be "Try to avoid DLL version checks where possible" -- for example, refactorings like making the blocklist not call the API if the block is for ALL_VERSIONS anyway.

Comment 3

[(Away)]

> blocklist not call the API if the block is for ALL_VERSIONS anyway.
Oh, I wasn't reading the code closely enough; this already happens.

Comment 4

[(no longer active)]

Firstly note that LoadLibrary in the general case does a lot more than just read the file header, such as examining the search path.  Also since it does memory mapped IO it's not clear to me if it does read only the number of bytes that it needs in order to determine the version information.

Maybe the tone was harsh, sorry if it was, but my point was that we should triage the rest of the call sites to this function and get rid of them where they can be perf issues similar to the ones we have discovered before.  The blocklist case specifically doesn't seem to be an issue to me for the reasons that dmajor mentioned.