ASAN poisoning functions shouldn't be dllimport on Windows

RESOLVED FIXED in 4.20

Status

defect
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: dmajor, Assigned: emk)

Tracking

(Blocks 1 bug)

other
4.20
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Reporter

Description

3 years ago
19:25.41 In file included from d:/build/msys/s/central/layout/base/nsPresArena.h:23:
19:25.41 d:/build/msys/s/central/obj/asan/dist/include/nspr\plarena.h(103,17):  warning(clang): redeclaration of '__asan_poison_memory_region' should not add 'dllimport' attribute [-Wdll-attribute-on-redeclaration]
19:25.41 PR_IMPORT(void) __asan_poison_memory_region(void const volatile *addr, size_t size);
19:25.41                 ^
19:25.41 d:/build/msys/s/central/obj/asan/dist/include\mozilla/MemoryChecking.h(53,1):  note(clang): previous declaration is here
19:25.41 __asan_poison_memory_region(void const volatile *addr, size_t size);
19:25.41 ^

See https://hg.mozilla.org/mozilla-central/rev/c32fd9b9c355
We get similar warnings from everything in NSPR when we link it into a single sharedlib with NSS, since they're all declspec(dllimport). I tried to fix that in bug 1237863 but the fix wasn't correct so it caused bug 1242802. :-/
Assignee

Comment 2

11 months ago
> ASAN poisoning functions shouldn't be dllimport on Windows

Why not? We are importing ASAN poisoning functions from a DLL (clang_rt.asan_dynamic-*.dll). So we should mark them dllimport.

https://hg.mozilla.org/mozilla-central/rev/c32fd9b9c355#l1.13
>+// In clang-cl based ASAN, we link against the memory poisoning functions
>+// statically.

This comment is plain wrong.
Reporter

Comment 3

11 months ago
In the bug title I was probably just repeating what the compiler warning said. I think you're right that in practice these really are imports from a DLL.
Assignee

Comment 4

11 months ago
Mmm, we can't add dllimport because declaretions in sanitizer/asan_interface.h have no dll linkages and we can't touch this file :(
Assignee

Comment 5

11 months ago
Again, NSPR is in another repo and this warning spams too many directories.
Attachment #8997645 - Flags: review?(dmajor)
Assignee

Updated

11 months ago
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Assignee

Updated

11 months ago
Duplicate of this bug: 1480657
Assignee

Updated

11 months ago
Reporter

Comment 7

11 months ago
Comment on attachment 8997645 [details] [diff] [review]
Temporarily allow warnings from MemoryChecking.h

Please surround this area with a `#pragma clang diagnostic {push,pop}`.
Attachment #8997645 - Flags: review?(dmajor) → review+
Assignee

Comment 8

10 months ago
(In reply to David Major [:dmajor] from comment #7)
> Comment on attachment 8997645 [details] [diff] [review]
> Temporarily allow warnings from MemoryChecking.h
> 
> Please surround this area with a `#pragma clang diagnostic {push,pop}`.

I had to remove `#pragma clang diagnostic {push,pop}` because plarena.h may be included after MemoryChecking.h.

Comment 9

10 months ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd8445b019f
Temporarily allow warnings from MemoryChecking.h. r=dmajor
Assignee

Updated

10 months ago
Keywords: leave-open
Reporter

Comment 10

10 months ago
(In reply to Masatoshi Kimura [:emk] from comment #8)
> I had to remove `#pragma clang diagnostic {push,pop}` because plarena.h may
> be included after MemoryChecking.h.

Oh, good point... that's pretty unpleasant though. Hopefully this change doesn't live for too long.
Assignee

Updated

10 months ago
Keywords: leave-open

Comment 13

10 months ago
Comment on attachment 8998473 [details] [diff] [review]
Stop ASAN poisoning functions from using dllimport on Windows

IIUC this change achieves consistency with another header file, that is included earlier, and which doesn't use dllimport. This is reasonable. r=kaie

I'll handle uplifting. I'll run a try build first.
Attachment #8998473 - Flags: review?(kaie) → review+

Updated

10 months ago
Target Milestone: --- → 4.20

Comment 14

10 months ago
Comment on attachment 8998473 [details] [diff] [review]
Stop ASAN poisoning functions from using dllimport on Windows

The test produced build failures. Instead of empty, maybe you need to define it as "type_".

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25298b5d7b870fd0eb6b72f920d0f8df4905e5b5&selectedJob=194109937

16:08:49     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/clang/bin/clang-cl.exe -fms-compatibility-version=19.13.26128 -FoUnified_cpp_certverifier0.obj -c -Iz:/build/build/src/obj-firefox/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 '-DDLL_PREFIX=""' '-DDLL_SUFFIX=".dll"' -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/security/certverifier -Iz:/build/build/src/obj-firefox/security/certverifier -Iz:/build/build/src/security/manager/ssl -Iz:/build/build/src/security/pkix/include -Iz:/build/build/src/security/pkix/lib -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -U_FORTIFY_SOURCE -Xclang -fno-common -Qunused-arguments -fsanitize=address -fsanitize-blacklist=z:/build/build/src/build/sanitizers/asan_blacklist_win.txt -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -U_FORTIFY_SOURCE -Xclang -fno-common -W3 -Gy -Zc:inline -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -wd4065 -Wno-inline-new-delete -Wno-invalid-offsetof -Wno-microsoft-enum-value -Wno-microsoft-include -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-invalid-noreturn -Wno-inconsistent-missing-override -Wno-implicit-exception-spec-mismatch -Wno-unused-local-typedef -Wno-ignored-attributes -Wno-used-but-marked-unused -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O2 -gline-tables-only -Oy- -W4 -wd4324 -wd4355 -wd4464 -wd4480 -wd4481 -wd4510 -wd4512 -wd4514 -wd4610 -wd4619 -wd4623 -wd4625 -wd4626 -wd4628 -wd4640 -wd4710 -wd4711 -wd4820 -wd4548 -wd4668 -wd4987 -wd4456 -wd4458 -wd4061 -wd4365 -wd4774 -wd4100 -wd4127 -wd4946  -Xclang -MP -Xclang -dependency-file -Xclang .deps/Unified_cpp_certverifier0.obj.pp -Xclang -MT -Xclang Unified_cpp_certverifier0.obj    z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:29:
16:08:49     INFO -  In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.cpp:7:
16:08:49     INFO -  In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.h:14:
16:08:49     INFO -  In file included from z:/build/build/src/security/manager/ssl\ScopedNSSTypes.h:16:
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss\cert.h:13:
16:08:49     INFO -  z:/build/build/src/obj-firefox/dist/include/nspr\plarena.h(112,26):  error: C++ requires a type specifier for all declarations
16:08:49     INFO -  PL_ASAN_VISIBILITY(void) __asan_poison_memory_region(
16:08:49     INFO -                           ^
16:08:49     INFO -  z:/build/build/src/obj-firefox/dist/include/nspr\plarena.h(114,26):  error: C++ requires a type specifier for all declarations
16:08:49     INFO -  PL_ASAN_VISIBILITY(void) __asan_unpoison_memory_region(
16:08:49     INFO -                           ^
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:29:
16:08:49     INFO -  In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.cpp:7:
16:08:49     INFO -  In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.h:14:
16:08:49     INFO -  In file included from z:/build/build/src/security/manager/ssl\ScopedNSSTypes.h:16:
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss\cert.h:21:
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss/keyt.h:8:
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss/keythi.h:9:
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss/pkcs11t.h:1797:
16:08:49     INFO -  z:/build/build/src/obj-firefox/dist/include/nss/pkcs11n.h(440,9):  warning: unknown pragma ignored [-Wunknown-pragmas]
16:08:49     INFO -  #pragma deprecated(CKT_NSS_UNTRUSTED, CKT_NSS_MUST_VERIFY, CKT_NSS_VALID)
16:08:49     INFO -          ^
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:29:
16:08:49     INFO -  In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.cpp:7:
16:08:49     INFO -  In file included from z:/build/build/src/security/certverifier/CTDiversityPolicy.h:14:
16:08:49     INFO -  In file included from z:/build/build/src/security/manager/ssl\ScopedNSSTypes.h:18:
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/dist/include/nss\cryptohi.h:11:
16:08:49     INFO -  z:/build/build/src/obj-firefox/dist/include/nss/blapit.h(66,9):  warning: unknown pragma ignored [-Wunknown-pragmas]
16:08:49     INFO -  #pragma deprecated(DSA_SUBPRIME_LEN, DSA_SIGNATURE_LEN, DSA_QBITS)
16:08:49     INFO -          ^
16:08:49     INFO -  In file included from z:/build/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:110:
16:08:49     INFO -  In file included from z:/build/build/src/security/certverifier/NSSCertDBTrustDomain.cpp:16:
16:08:49     INFO -  z:/build/build/src/obj-firefox/dist/include/nss\certdb.h(34,9):  warning: unknown pragma ignored [-Wunknown-pragmas]
16:08:49     INFO -  #pragma deprecated(CERTDB_VALID_PEER)
16:08:49     INFO -          ^
16:08:49     INFO -  3 warnings and 2 errors generated.
Attachment #8998473 - Flags: review+ → review-

Comment 16

10 months ago
https://hg.mozilla.org/projects/nspr/rev/6e31156d7002
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Reporter

Comment 18

10 months ago
Comment on attachment 9001560 [details] [diff] [review]
Revert changeset 5bd8445b019f as final NSPR fix has landed

Thanks!

If you s/Revert/Back out/ in the commit message then our hgweb can link the original changeset to this one.
Attachment #9001560 - Flags: review?(dmajor) → review+

Comment 19

10 months ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/635d66906fe4
Backed out changeset 5bd8445b019f as final NSPR fix has landed. r=dmajor
Assignee

Updated

10 months ago
Depends on: 1485028
You need to log in before you can comment on or make changes to this bug.