Page MenuHomePhabricator

[COFF] Add exported functions to gfids table for /guard:cf
ClosedPublic

Authored by rnk on Nov 19 2018, 1:51 PM.

Details

Summary

MSVC does this, and we should to.

The .gfids table is a table of RVAs, so it's impossible for a DLL to
indicate that an imported symbol is address taken. Therefore, exports
appear to be listed as address taken by the DLL that exports them.

This fixes an issue that Firefox ran into here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1485016#c12

In Firefox, the export directive came from a .def file, but we need to
do this for any kind of export.

Event Timeline

rnk created this revision.Nov 19 2018, 1:51 PM
alex added a comment.Nov 19 2018, 2:12 PM

I don't know this code very well at all, so just one small comment that jumped out to me. Conceptually it looks like this solves the problem we were seeing. Thanks for jumping on this so quickly!

lld/COFF/Writer.cpp
1371

In the case where Config->Entry is a Defined, but not a DefinedCOFF, it won't get added with this patch, whereas it would have before. Can that situation happen?

Thanks! I applied the patch and confirmed that it fixes the problematic Firefox DLL. (I don't know if I'm allowed to formally approve it though...)

hans accepted this revision.Nov 20 2018, 12:52 AM

lgtm

This revision is now accepted and ready to land.Nov 20 2018, 12:52 AM
rnk marked an inline comment as done.Nov 26 2018, 5:47 PM

Thanks, landing soon.

lld/COFF/Writer.cpp
1371

(one week later, after the holidays...) So, it looks like that situation can happen. The one I came up with was import thunks. They can be exported, entry points, and address taken, although it's a bit ridiculous to import an export, or use an imported symbol as your entry point.

However, it's a pre-existing issue in the relocation scanning code. Even though it's a behavior change for entry point symbols, I think making them both have the same bug is actually somewhat better. I filed this bug for it: https://bugs.llvm.org/show_bug.cgi?id=39799

This revision was automatically updated to reflect the committed changes.