1426876 - Work around an lld bug in export mangling

Status: RESOLVED WORKSFORME

(Core :: Security: Process Sandboxing, enhancement, P3)

Description

[(Away)]

32-bit C stdcall functions are supposed to be mangled with a leading underscore and trailing @popbytes, e.g. foo -> _foo@4.

lld omits the underscore so we just get foo@4 (https://bugs.llvm.org/show_bug.cgi?id=35733).

This trips a fatal assert in the sandbox code when it gets null from calls like `GetProcAddress(..., "_TargetNtMapViewOfSection@44")`.

Comment 1

[(Away)]

I worked around this locally by adding a fallback to GetProcAddress that tries without the underscore: https://hg.mozilla.org/try/rev/74663141ac803ef895fb14ad2d0521bc55f8dd8d

Bob, what's the right way to land code in the sandbox tree? Does this have to be a .patch file, an upstream fix, something else... ?

Comment 2

[Bob Owen (:bobowen)]

(In reply to David Major [:dmajor] from comment #1)
> I worked around this locally by adding a fallback to GetProcAddress that
> tries without the underscore:
> https://hg.mozilla.org/try/rev/74663141ac803ef895fb14ad2d0521bc55f8dd8d
> 
> Bob, what's the right way to land code in the sandbox tree? Does this have
> to be a .patch file, an upstream fix, something else... ?

If it's a compile time failure or something that means we don't get a mostly working Firefox, then a patch added to security\sandbox\chromium-shim\patches\with_update\ and the patch name added to security\sandbox\chromium-shim\patches\with_update\patch_order.txt
(I changed this with the last chromium update and also made notes, so I should have documentation in tree soon (in the new year) for the update process I follow.)

Getting it up-streamed, if possible, would be great as well.

This sounds like an issue we had getting the mingw build working for tor.
For that we compiled without SANDBOX_EXPORTS defined which means the sandbox function patching will only work if the child is firefox.exe.
This was OK for tor because we only use plugin-container for GMP and 64-bit NPAPI.

Comment 3

[(Away)]

If this LLD change lands, we can drop the workaround: https://reviews.llvm.org/D41632

Comment 4

[(Away)]

This bug was fixed in lld.