1485016 - Enable /guard:cf on firefox.exe with clang-cl for protection on system dlls

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Tom Ritter [:tjr]]

When we switched to clang builds on Windows, we lost the little CFG we had. We can re-enable it (I believe) on firefox.exe to enable protection on System Dlls.

Comment 1

[Tom Ritter [:tjr]]

I did a build of this; but for some reason all tests are failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7134ac7d404c3327a50490bec331fb390c6a4382&selectedJob=195351343

However the browser ran fine, and I manually tested CFG and it was working (meaning, my test-CFG function crashed.)

Comment 2

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 Try enabling guard:cf on firefox.exe and mozglue.dll on clang-cl r=dmajor

Comment 3

[(Away)]

Comment on attachment 9003288 [details]
Bug 1485016 Try enabling guard:cf on firefox.exe and mozglue.dll on clang-cl r=dmajor

David Major [:dmajor] has approved the revision.

Comment 4

[Tom Ritter [:tjr]]

So it looks like we're getting a stack buffer overflow with this change.  I was able to reproduce it locally first try running 

>  ./mach test browser/base/content/test/pageinfo/browser_pageinfo_firstPartyIsolation.js  --debugger=windbg

Backtrace:

>  # Child-SP          RetAddr           Call Site
> 00 00000069`919fa338 00007ffc`db1b69fc ntdll!RtlFailFast2
> 01 00000069`919fa340 00007ffc`db150ed8 ntdll!RtlpHandleInvalidUserCallTarget+0x5c
> 02 00000069`919fa370 00007ffc`af5524ea ntdll!LdrpHandleInvalidUserCallTarget+0x38
> 03 00000069`919fa430 00007ffc`af5908a1 MSVCP140!std::basic_ostream<char,std::char_traits<char> >::sentry::sentry+0x4a [f:\dd\vctools\crt\crtw32\stdhpp\ostream @ 120]
> *** WARNING: Unable to verify checksum for c:\mozilla-unified-2\obj-x86_64-pc-mingw32\dist\bin\mozglue.dll
> 04 00000069`919fa470 00007ffc`bf4ea7b7 MSVCP140!std::basic_ostream<char,std::char_traits<char> >::operator<<+0x41 [f:\dd\vctools\crt\crtw32\stdhpp\ostream @ 391]
> 05 00000069`919fa510 00007ffc`bf4ecc8e mozglue!mozToString+0x147 [c:\mozilla-unified-2\mfbt\decimal\moz-decimal-utils.h @ 76]
> 06 00000069`919fa670 00007ffc`bf4ed471 mozglue!blink::Decimal::toString+0x21e [c:\mozilla-unified-2\mfbt\decimal\Decimal.cpp @ 991]
> *** WARNING: Unable to verify checksum for c:\mozilla-unified-2\obj-x86_64-pc-mingw32\dist\bin\xul.dll
> 07 00000069`919fa740 00007ffc`899a1e0a mozglue!blink::Decimal::toString+0x31 [c:\mozilla-unified-2\mfbt\decimal\Decimal.cpp @ 1036]
> 08 00000069`919fa7d0 00007ffc`899a2824 xul!mozilla::dom::HTMLInputElement::SanitizeValue+0x68a [c:\mozilla-unified-2\dom\html\HTMLInputElement.cpp @ 4957]
> 09 00000069`919fa980 00007ffc`8999fa4c xul!mozilla::dom::HTMLInputElement::SetValueInternal+0x134 [c:\mozilla-unified-2\dom\html\HTMLInputElement.cpp @ 2820]
> 0a 00000069`919faa90 00007ffc`89f18b67 xul!mozilla::dom::HTMLInputElement::DoneCreatingElement+0x17c [c:\mozilla-unified-2\dom\html\HTMLInputElement.cpp @ 6330]
> 0b 00000069`919fab80 00007ffc`89f188c7 xul!nsXMLContentSink::HandleStartElement+0x297 [c:\mozilla-unified-2\dom\xml\nsXMLContentSink.cpp @ 1081]
> 0c 00000069`919fac40 00007ffc`89efbebe xul!nsXMLContentSink::HandleStartElement+0x27 [c:\mozilla-unified-2\dom\xml\nsXMLContentSink.cpp @ 998]
> 0d 00000069`919fac80 00007ffc`8882fafc xul!nsXBLContentSink::HandleStartElement+0x1e [c:\mozilla-unified-2\dom\xbl\nsXBLContentSink.cpp @ 255]
> 0e 00000069`919facd0 00007ffc`8a8b9e1a xul!nsExpatDriver::HandleStartElement+0x6c [c:\mozilla-unified-2\parser\htmlparser\nsExpatDriver.cpp @ 328]
> 0f 00000069`919fad40 00007ffc`8a8b6bb3 xul!doContent+0xc2a [c:\mozilla-unified-2\parser\expat\lib\xmlparse.c @ 2468]
> 10 00000069`919faea0 00007ffc`8a8b3ab0 xul!doProlog+0x2ee3 [c:\mozilla-unified-2\parser\expat\lib\xmlparse.c @ 4078]
> 11 00000069`919fafd0 00007ffc`8a8b33f9 xul!prologInitProcessor+0x110 [c:\mozilla-unified-2\parser\expat\lib\xmlparse.c @ 3629]
> 12 00000069`919fb0d0 00007ffc`88831508 xul!MOZ_XML_Parse+0x149 [c:\mozilla-unified-2\parser\expat\lib\xmlparse.c @ 1530]
> 13 00000069`919fb140 00007ffc`888318c3 xul!nsExpatDriver::ParseBuffer+0x88 [c:\mozilla-unified-2\parser\htmlparser\nsExpatDriver.cpp @ 894]
> 14 00000069`919fb1a0 00007ffc`88834efb xul!nsExpatDriver::ConsumeToken+0x383 [c:\mozilla-unified-2\parser\htmlparser\nsExpatDriver.cpp @ 988]
> 15 00000069`919fb370 00007ffc`88833e0a xul!nsParser::Tokenize+0xcb [c:\mozilla-unified-2\parser\htmlparser\nsParser.cpp @ 1541]
> 16 00000069`919fb3f0 00007ffc`888353f9 xul!nsParser::ResumeParse+0xfa [c:\mozilla-unified-2\parser\htmlparser\nsParser.cpp @ 1056]
> 17 00000069`919fb4c0 00007ffc`88e134ce xul!nsParser::OnDataAvailable+0x1c9 [c:\mozilla-unified-2\parser\htmlparser\nsParser.cpp @ 1439]
> 18 00000069`919fb560 00007ffc`89f0f5ae xul!nsSyncLoadService::PushSyncStreamToListener+0x15e [c:\mozilla-unified-2\dom\base\nsSyncLoadService.cpp @ 395]
> 19 00000069`919fb5f0 00007ffc`89ef6853 xul!nsXBLService::FetchBindingDocument+0x2fe [c:\mozilla-unified-2\dom\xbl\nsXBLService.cpp @ 1157]
> 1a 00000069`919fb6e0 00007ffc`89f0e79a xul!nsXBLService::LoadBindingDocumentInfo+0x463 [c:\mozilla-unified-2\dom\xbl\nsXBLService.cpp @ 0]
> 1b 00000069`919fb7a0 00007ffc`89f0ddc9 xul!nsXBLService::GetBinding+0xea [c:\mozilla-unified-2\dom\xbl\nsXBLService.cpp @ 0]
> 1c 00000069`919fba30 00007ffc`8a271bc9 xul!nsXBLService::LoadBindings+0x1e9 [c:\mozilla-unified-2\dom\xbl\nsXBLService.cpp @ 525]
> 1d 00000069`919fbc00 00007ffc`8a267805 xul!nsCSSFrameConstructor::LoadXBLBindingIfNeeded+0xd9 [c:\mozilla-unified-2\layout\base\nsCSSFrameConstructor.cpp @ 5615]
> 1e 00000069`919fbca0 00007ffc`8a268e0b xul!nsCSSFrameConstructor::AddFrameConstructionItemsInternal+0x55 [c:\mozilla-unified-2\layout\base\nsCSSFrameConstructor.cpp @ 0]
> 1f 00000069`919fbdb0 00007ffc`8a26f24c xul!nsCSSFrameConstructor::ProcessChildren+0x30b [c:\mozilla-unified-2\layout\base\nsCSSFrameConstructor.cpp @ 10118]
> 20 00000069`919fbfe0 00007ffc`8a272195 xul!nsCSSFrameConstructor::ConstructFrameFromItemInternal+0x84c [c:\mozilla-unified-2\layout\base\nsCSSFrameConstructor.cpp @ 3995]
> 21 00000069`919fc190 00007ffc`8a268aa6 xul!nsCSSFrameConstructor::ConstructFramesFromItem+0xd5 [c:\mozilla-unified-2\layout\base\nsCSSFrameConstructor.cpp @ 5984]
> 22 00000069`919fc210 00007ffc`8a275ee2 xul!nsCSSFrameConstructor::ConstructFramesFromItemList+0xb6 [c:\mozilla-unified-2\layout\base\nsCSSFrameConstructor.cpp @ 9991]
> 23 00000069`919fc290 00007ffc`8a2553b1 xul!nsCSSFrameConstructor::ContentAppended+0xb32 [c:\mozilla-unified-2\layout\base\nsCSSFrameConstructor.cpp @ 7042]
> 24 00000069`919fc510 00007ffc`8a258b2b xul!mozilla::RestyleManager::ProcessRestyledFrames+0x511 [c:\mozilla-unified-2\layout\base\RestyleManager.cpp @ 1442]
> 25 00000069`919fc940 00007ffc`8a2449cb xul!mozilla::RestyleManager::DoProcessPendingRestyles+0x35b [c:\mozilla-unified-2\layout\base\RestyleManager.cpp @ 3059]
> 26 00000069`919fcc10 00007ffc`88dafed3 xul!mozilla::PresShell::DoFlushPendingNotifications+0x40b [c:\mozilla-unified-2\layout\base\PresShell.cpp @ 4297]
> 27 00000069`919fccc0 00007ffc`88dc481e xul!nsIDocument::FlushPendingNotifications+0x193 [c:\mozilla-unified-2\dom\base\nsDocument.cpp @ 7445]
> 28 00000069`919fcd20 00007ffc`88dc36c6 xul!nsFocusManager::CheckIfFocusable+0x17e [c:\mozilla-unified-2\dom\base\nsFocusManager.cpp @ 1596]
> 29 00000069`919fcdf0 00007ffc`88dc4628 xul!nsFocusManager::SetFocusInner+0x36 [c:\mozilla-unified-2\dom\base\nsFocusManager.cpp @ 1216]
> 2a 00000069`919fcf50 00007ffc`88d1fef2 xul!nsFocusManager::SetFocus+0x58 [c:\mozilla-unified-2\dom\base\nsFocusManager.cpp @ 510]
> 2b 00000069`919fcfa0 00007ffc`8942febc xul!mozilla::dom::Element::Focus+0x42 [c:\mozilla-unified-2\dom\base\Element.cpp @ 348]
> 2c 00000069`919fcfe0 00007ffc`896bbfe0 xul!mozilla::dom::XULElement_Binding::focus+0xac [c:\mozilla-unified-2\obj-x86_64-pc-mingw32\dom\bindings\XULElementBinding.cpp @ 1659]
> 2d 00000069`919fd050 00007ffc`8b6794c6 xul!mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions>+0x1a0 [c:\mozilla-unified-2\dom\bindings\BindingUtils.cpp @ 3311]
> 2e 00000069`919fd110 00007ffc`8b679c13 xul!js::InternalCallOrConstruct+0x426 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 533]
> 2f 00000069`919fd220 00007ffc`8b679c84 xul!InternalCall+0x93 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 584]
> 30 00000069`919fd280 00007ffc`8b432e1e xul!js::Call+0x24 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 603]
> 31 00000069`919fd2b0 00007ffc`8b41f62b xul!js::ForwardingProxyHandler::call+0x39e [c:\mozilla-unified-2\js\src\proxy\Wrapper.cpp @ 176]
> 32 00000069`919fd3d0 00007ffc`8b429de7 xul!js::CrossCompartmentWrapper::call+0x19b [c:\mozilla-unified-2\js\src\proxy\CrossCompartmentWrapper.cpp @ 359]
> 33 00000069`919fd450 00007ffc`8b67991b xul!js::Proxy::call+0xd7 [c:\mozilla-unified-2\js\src\proxy\Proxy.cpp @ 510]
> 34 00000069`919fd4d0 00007ffc`8b679c13 xul!js::InternalCallOrConstruct+0x87b [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 509]
> 35 00000069`919fd5e0 00007ffc`8b66d669 xul!InternalCall+0x93 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 584]
> 36 00000069`919fd640 00007ffc`8b66c76d xul!Interpret+0xcc9 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 3239]
> 37 00000069`919fda20 00007ffc`8b679766 xul!js::RunScript+0x14d [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 425]
> 38 00000069`919fdb30 00007ffc`8b679c13 xul!js::InternalCallOrConstruct+0x6c6 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 557]
> 39 00000069`919fdc40 00007ffc`8b679c84 xul!InternalCall+0x93 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 584]
> 3a 00000069`919fdca0 00007ffc`8b54d7ee xul!js::Call+0x24 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 603]
> 3b 00000069`919fdcd0 00007ffc`8b4507f2 xul!js::CallSelfHostedFunction+0x8e [c:\mozilla-unified-2\js\src\vm\SelfHosting.cpp @ 1848]
> 3c 00000069`919fdd40 00007ffc`8b450681 xul!AsyncFunctionResume+0x162 [c:\mozilla-unified-2\js\src\vm\AsyncFunction.cpp @ 186]
> 3d 00000069`919fde50 00007ffc`8b2c81f3 xul!js::AsyncFunctionAwaitedFulfilled+0x11 [c:\mozilla-unified-2\js\src\vm\AsyncFunction.cpp @ 213]
> 3e 00000069`919fde80 00007ffc`8b6794c6 xul!PromiseReactionJob+0x4d3 [c:\mozilla-unified-2\js\src\builtin\Promise.cpp @ 1500]
> 3f 00000069`919fdfe0 00007ffc`8b679c13 xul!js::InternalCallOrConstruct+0x426 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 533]
> 40 00000069`919fe0f0 00007ffc`8b679c84 xul!InternalCall+0x93 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 584]
> 41 00000069`919fe150 00007ffc`8b3e3230 xul!js::Call+0x24 [c:\mozilla-unified-2\js\src\vm\Interpreter.cpp @ 603]
> 42 00000069`919fe180 00007ffc`890911dc xul!JS::Call+0x3f0 [c:\mozilla-unified-2\js\src\jsapi.cpp @ 2915]
> 43 00000069`919fe290 00007ffc`87f182eb xul!mozilla::dom::PromiseJobCallback::Call+0xac [c:\mozilla-unified-2\obj-x86_64-pc-mingw32\dom\bindings\PromiseBinding.cpp @ 25]
> 44 00000069`919fe340 00007ffc`87f0ddaf xul!mozilla::PromiseJobRunnable::Run+0x9b [c:\mozilla-unified-2\xpcom\base\CycleCollectedJSContext.cpp @ 228]
> 45 00000069`919fe550 00007ffc`898ff96e xul!mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint+0x3bf [c:\mozilla-unified-2\xpcom\base\CycleCollectedJSContext.cpp @ 575]
> 46 00000069`919fe660 00007ffc`89900314 xul!mozilla::EventListenerManager::HandleEventSubType+0x19e [c:\mozilla-unified-2\dom\events\EventListenerManager.cpp @ 1091]
> 47 00000069`919fe8a0 00007ffc`898f9bd3 xul!mozilla::EventListenerManager::HandleEventInternal+0x6e4 [c:\mozilla-unified-2\dom\events\EventListenerManager.cpp @ 1342]
> 48 00000069`919feb30 00007ffc`898f9514 xul!mozilla::EventTargetChainItem::HandleEvent+0x233 [c:\mozilla-unified-2\dom\events\EventDispatcher.cpp @ 426]
> 49 00000069`919febb0 00007ffc`898faf61 xul!mozilla::EventTargetChainItem::HandleEventTargetChain+0x4d4 [c:\mozilla-unified-2\dom\events\EventDispatcher.cpp @ 637]
> 4a 00000069`919fece0 00007ffc`898fb80a xul!mozilla::EventDispatcher::Dispatch+0xe21 [c:\mozilla-unified-2\dom\events\EventDispatcher.cpp @ 1112]
> 4b 00000069`919fee70 00007ffc`88de2b83 xul!mozilla::EventDispatcher::DispatchDOMEvent+0xea [c:\mozilla-unified-2\dom\events\EventDispatcher.cpp @ 1197]
> 4c 00000069`919feee0 00007ffc`89903670 xul!nsINode::DispatchEvent+0x73 [c:\mozilla-unified-2\dom\base\nsINode.cpp @ 1109]
> 4d 00000069`919fef60 00007ffc`898dd82d xul!mozilla::dom::EventTarget::DispatchEvent+0x30 [c:\mozilla-unified-2\dom\events\EventTarget.cpp @ 197]
> 4e 00000069`919fefb0 00007ffc`87f8b01b xul!mozilla::AsyncEventDispatcher::Run+0x14d [c:\mozilla-unified-2\dom\events\AsyncEventDispatcher.cpp @ 74]
> 4f 00000069`919ff020 00007ffc`87f8d182 xul!nsThread::ProcessNextEvent+0x5db [c:\mozilla-unified-2\xpcom\threads\nsThread.cpp @ 1182]
> 50 00000069`919ff580 00007ffc`883c7192 xul!NS_ProcessNextEvent+0x42 [c:\mozilla-unified-2\xpcom\threads\nsThreadUtils.cpp @ 519]
> 51 00000069`919ff5d0 00007ffc`8839d799 xul!mozilla::ipc::MessagePump::Run+0xf2 [c:\mozilla-unified-2\ipc\glue\MessagePump.cpp @ 97]
> 52 00000069`919ff640 00007ffc`8839d718 xul!MessageLoop::RunHandler+0x49 [c:\mozilla-unified-2\ipc\chromium\src\base\message_loop.cc @ 319]
> 53 00000069`919ff690 00007ffc`8a074498 xul!MessageLoop::Run+0x58 [c:\mozilla-unified-2\ipc\chromium\src\base\message_loop.cc @ 299]
> 54 00000069`919ff6e0 00007ffc`8a0d2f0c xul!nsBaseAppShell::Run+0x28 [c:\mozilla-unified-2\widget\nsBaseAppShell.cpp @ 160]
> 55 00000069`919ff720 00007ffc`8b10f92d xul!nsAppShell::Run+0xdc [c:\mozilla-unified-2\widget\windows\nsAppShell.cpp @ 415]
> 56 00000069`919ff770 00007ffc`8b1ba6db xul!nsAppStartup::Run+0x3d [c:\mozilla-unified-2\toolkit\components\startup\nsAppStartup.cpp @ 291]
> 57 00000069`919ff7a0 00007ffc`8b1bb716 xul!XREMain::XRE_mainRun+0xc9b [c:\mozilla-unified-2\toolkit\xre\nsAppRunner.cpp @ 4799]
> 58 00000069`919ff9e0 00007ffc`8b1bbefa xul!XREMain::XRE_main+0x4b6 [c:\mozilla-unified-2\toolkit\xre\nsAppRunner.cpp @ 4944]
> *** WARNING: Unable to verify checksum for firefox.exe
> 59 00000069`919ffaa0 00007ff7`6c5816a6 xul!XRE_main+0xca [c:\mozilla-unified-2\toolkit\xre\nsAppRunner.cpp @ 5036]
> 5a 00000069`919ffc40 00007ff7`6c58122e firefox!NS_internal_main+0x3c6 [c:\mozilla-unified-2\browser\app\nsBrowserApp.cpp @ 311]
> 5b 00000069`919ffde0 00007ff7`6c5c333c firefox!wmain+0x22e [c:\mozilla-unified-2\toolkit\xre\nsWindowsWMain.cpp @ 143]
> 5c 00000069`919ffe90 00007ffc`d99e1fe4 firefox!__scrt_common_main_seh+0x110 [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 283]
> 5d 00000069`919ffed0 00007ffc`db12cb31 KERNEL32!BaseThreadInitThunk+0x14
> 5e 00000069`919fff00 00000000`00000000 ntdll!RtlUserThreadStart+0x21
> 0:000> r
> rax=0000000000000000 rbx=00007ffcbf4fa930 rcx=000000000000000a
> rdx=00007ffcbf4fa930 rsi=00000069919fa4a8 rdi=00000069919fa548
> rip=00007ffcdb164e10 rsp=00000069919fa338 rbp=0000000000000000
>  r8=00007ffcd7573880  r9=00007ffcd7573888 r10=0000000000000000
> r11=0000b196c1ad3adf r12=0000000000000032 r13=00007ffcbf4eacf0
> r14=00000069919fa550 r15=00000069919fa548
> iopl=0         nv up ei pl zr na po nc
> cs=0033  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000246
> ntdll!RtlFailFast2:
> 00007ffc`db164e10 cd29            int     29h

Comment 5

[(Away)]

It's actually a CFG failure rather than a stack overflow.

CFG is doing weird things to ostringstream's vtable. Here's what it should look like, from today's Nightly (clang-cl but not CFG):

> 0:000> x mozglue!*ostringstream*vftable*
> 00000001`8002a1d8 mozglue!std::basic_ostringstream<char,std::char_traits<char>,std::allocator<char> >::`vftable' = <no type information>
> 0:000> dqs 01`8002a1d8 
> 00000001`8002a1d8  00000001`8001c030 mozglue!std::basic_ostringstream<char,std::char_traits<char>,std::allocator<char> >::`vector deleting destructor'
> 00000001`8002a1e0  00000001`8001c140 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::~basic_stringbuf
> 00000001`8002a1e8  00000001`800299b0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Lock
> 00000001`8002a1f0  00000001`800299c0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Unlock

And in the clang-cl CFG build there are two extra garbage entries:

> 0:000> x mozglue!*ostringstream*vftable*
> 00000001`8002ba28 mozglue!std::basic_ostringstream<char,std::char_traits<char>,std::allocator<char> >::`vftable' = <no type information>
> 0:000> dqs 1`8002ba28
> 00000001`8002ba28  00000001`8001d450 mozglue!std::basic_ostringstream<char,std::char_traits<char>,std::allocator<char> >::`vector deleting destructor'
> 00000001`8002ba30  00000088`00000000
> 00000001`8002ba38  00000000`00000000
> 00000001`8002ba40  00000001`8001d5b0 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::~basic_stringbuf
> 00000001`8002ba48  00000001`8002a5a0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Lock
> 00000001`8002ba50  00000001`8002a5b0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Unlock

I haven't traced it super carefully but I _think_ the string code is trying to call a member function through its vtable at one of those garbage offsets.

Comment 6

[Tom Ritter [:tjr]]

So while confirmign everything for filing upstream, I encountered something weird. I can reproduce the crash; but both CFG and non-CFG have the NULL vtable entries.

Built off

changeset:   481547:48a45df79f32
fxtree:      central
parent:      481546:cfadc4196e56
parent:      481458:0dcc490db922
user:        Andreea Pavel <[e-mail]>
date:        Tue Aug 14 19:15:33 2018 +0300
summary:     Merge mozilla-inbound to mozilla-central. a=merge


Using clang-341558 from 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc86efd0169a160288f31a98bdb7a869cf14f68a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=197928699


https://ritter.vg/misc/ff/nocfg-firefox-63.0a1.en-US.win64.installer.exe
https://ritter.vg/misc/ff/cfg-firefox-63.0a1.en-US.win64.installer.exe


CFG (test fails, crashes):

> 0:000> x mozglue!*ostringstream*vftable*
> *** WARNING: Unable to verify checksum for c:\mozilla-unified-2\obj-x86_64-pc-mingw32\dist\bin\mozglue.dll
> 00007ffc`d0e5ba28 mozglue!std::basic_ostringstream<char,std::char_traits<char>,std::allocator<char> >::`vftable' = <no type information>
> 0:000> dqs 00007ffc`d0e5ba28
> 00007ffc`d0e5ba28  00007ffc`d0e4d4c0 mozglue!std::basic_ostringstream<char,std::char_traits<char>,std::allocator<char> >::`vector deleting destructor'
> 00007ffc`d0e5ba30  00000088`00000000
> 00007ffc`d0e5ba38  00000000`00000000
> 00007ffc`d0e5ba40  00007ffc`d0e4d620 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::~basic_stringbuf [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 74]
> 00007ffc`d0e5ba48  00007ffc`d0e5a910 mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Lock
> 00007ffc`d0e5ba50  00007ffc`d0e5a920 mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Unlock
> 00007ffc`d0e5ba58  00007ffc`d0e4d660 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::overflow [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 119]
> 00007ffc`d0e5ba60  00007ffc`d0e4d8b0 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::pbackfail [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 189]
> 00007ffc`d0e5ba68  00007ffc`d0e5a980 mozglue!std::basic_streambuf<char,std::char_traits<char> >::showmanyc
> 00007ffc`d0e5ba70  00007ffc`d0e4d910 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::underflow [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 206]
> 00007ffc`d0e5ba78  00007ffc`d0e5a9a0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::uflow
> 00007ffc`d0e5ba80  00007ffc`d0e5a9b0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::xsgetn
> 00007ffc`d0e5ba88  00007ffc`d0e5a9c0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::xsputn
> 00007ffc`d0e5ba90  00007ffc`d0e4d980 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::seekoff [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 226]
> 00007ffc`d0e5ba98  00007ffc`d0e4db10 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::seekpos [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 275]
> 00007ffc`d0e5baa0  00007ffc`d0e5a960 mozglue!std::basic_streambuf<char,std::char_traits<char> >::setbuf




No CFG (test scucceeds, does not crash): 

> 0:000> x mozglue!*ostringstream*vftable*
> *** WARNING: Unable to verify checksum for c:\mozilla-unified-2\obj-x86_64-pc-mingw32\dist\bin\mozglue.dll
> 00007ffc`ca06ba28 mozglue!std::basic_ostringstream<char,std::char_traits<char>,std::allocator<char> >::`vftable' = <no type information>
> 0:000> dqs 00007ffc`ca06ba28
> 00007ffc`ca06ba28  00007ffc`ca05d3f0 mozglue!std::basic_ostringstream<char,std::char_traits<char>,std::allocator<char> >::`vector deleting destructor'
> 00007ffc`ca06ba30  00000088`00000000
> 00007ffc`ca06ba38  00000000`00000000
> 00007ffc`ca06ba40  00007ffc`ca05d550 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::~basic_stringbuf [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 74]
> 00007ffc`ca06ba48  00007ffc`ca06a7c0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Lock
> 00007ffc`ca06ba50  00007ffc`ca06a7d0 mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Unlock
> 00007ffc`ca06ba58  00007ffc`ca05d590 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::overflow [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 119]
> 00007ffc`ca06ba60  00007ffc`ca05d7e0 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::pbackfail [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 189]
> 00007ffc`ca06ba68  00007ffc`ca06a830 mozglue!std::basic_streambuf<char,std::char_traits<char> >::showmanyc
> 00007ffc`ca06ba70  00007ffc`ca05d840 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::underflow [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 206]
> 00007ffc`ca06ba78  00007ffc`ca06a850 mozglue!std::basic_streambuf<char,std::char_traits<char> >::uflow
> 00007ffc`ca06ba80  00007ffc`ca06a860 mozglue!std::basic_streambuf<char,std::char_traits<char> >::xsgetn
> 00007ffc`ca06ba88  00007ffc`ca06a870 mozglue!std::basic_streambuf<char,std::char_traits<char> >::xsputn
> 00007ffc`ca06ba90  00007ffc`ca05d8b0 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::seekoff [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 226]
> 00007ffc`ca06ba98  00007ffc`ca05da40 mozglue!std::basic_stringbuf<char,std::char_traits<char>,std::allocator<char> >::seekpos [C:\PROGRA~2\MIB055~1\2017\COMMUN~1\VC\Tools\MSVC\1413~1.261\include\sstream @ 275]
> 00007ffc`ca06baa0  00007ffc`ca06a810 mozglue!std::basic_streambuf<char,std::char_traits<char> >::setbuf

Comment 7

[Tom Ritter [:tjr]]

The latest try run for this is https://treeherder.mozilla.org/#/jobs?repo=try&revision=1da624e4a0199bc2dd4587c1b079749e1cf6d0ee&selectedJob=210622063

Alex investigated CFG support in clang: It does the linker work, but none of the compiler work, for CFG. Meaning that it marks functions whose address is taken as valid indirect call targets in the PE metadata, allowing system/other CFG protected DLLs to be protected, but it will not protect indirect jumps in the code you’re compiling. Means that a callback from win32.dll will be protected, but not any of our virtual calls in XUL.dll.

Based on this, unless someone enjoys the debugging puzzle, it's not really worth the effort to figure out the crashes to enable this. If someone did figure out the crashes, we would be interested in enabling it though.

Comment 8

[(Away)]

Ok, I took a closer look, and you're right, the null vtable entries are not relevant.

The real problem is that this function:
mozglue!std::basic_streambuf<char,std::char_traits<char> >::_Lock

which msvcp140 tries to call through our ostringstream's vtable, is not marked as a valid call target.

In case it matters, that _Lock method is just a stub that jumps back to msvcp140:
jmp     qword ptr [mozglue!_imp_?_Lock?$basic_streambufDU?$char_traitsDstdstdUEAAXXZ]

I tried the repro below, and _Lock is a valid target, both when I compile this as an .exe and a .dll, so there's something more subtle going on in the case of mozglue.

#include <iomanip>
#include <sstream>

int main(int, char**) {
  std::ostringstream o;
  o << std::setprecision(1) << 42;
  return 0;
}

Comment 9

[(Away)]

It turns out that this is because Decimal.cpp, being in mfbt/ and not under mozglue/, didn't have -guard:cf during compilation. /facepalm

Comment 10

[(Away)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c1963a6b3de2aa3b50d594ffdf86f1bfbd07dd5

I tried setting -guard:cf in mfbt/, and locally it fixes the one crash I was looking at, but there's still tons of broken tests.

We'll probably have to guard-ify all of the USE_LIBS in browser/app/moz.build and mozglue/build/moz.build. That's brittle though. I wonder if we could just set the flag globally; if clang is only doing the linker side of things, maybe it won't cause the regressions that kept us from enabling the flag globally with MSVC.

Comment 11

[Tom Ritter [:tjr]]

(In reply to David Major [:dmajor] from comment #10)
> I tried setting -guard:cf in mfbt/, and locally it fixes the one crash I was
> looking at, but there's still tons of broken tests.
> 
> We'll probably have to guard-ify all of the USE_LIBS in
> browser/app/moz.build and mozglue/build/moz.build. That's brittle though. I
> wonder if we could just set the flag globally; if clang is only doing the
> linker side of things, maybe it won't cause the regressions that kept us
> from enabling the flag globally with MSVC.

Yes, we should be able to apply it globally without the msvc regressions. I applied it globally, which I expected to work (and double checked decimal.cpp), but it still seems to crash. Or fail at least.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=332b54f8e0cbd8a762f078e4de50ee2a14831b97

Comment 12

[(Away)]

(In reply to Tom Ritter [:tjr] from comment #11)
> Yes, we should be able to apply it globally without the msvc regressions. I
> applied it globally, which I expected to work (and double checked
> decimal.cpp), but it still seems to crash. Or fail at least.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=332b54f8e0cbd8a762f078e4de50ee2a14831b97

We have some functions e.g. https://searchfox.org/mozilla-central/search?q=DllGetClassObject&redirect=false that are generated by MIDL and some SDK macros, and the only thing that makes them visible to the outside world is the .def file, and they're not getting marked as valid targets.

Comment 13

[(Away)]

\o/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=23a4fd0515441c84d00eddb1518d3503db0ffdc9

Comment 14

[(Away)]

Um, it just occurred to me that I didn't actually enable CFG in that push. :-|

Comment 15

[(Away)]

I tested again with CFG actually enabled, and it's still failing left and right on try, but the sampling of failures that I looked at didn't reproduce for me locally, where I use clang tip.

This likely means that there are additional CFG fixes on 8.0.0 besides D54723 that we need. Next steps (for which I don't have cycles right now) would be to push a tip revision on try and confirm that we're green. If so, then we'd have to decide whether to spend the time to hunt down all the necessary patches, or bite the bullet and move our automation builds back to trunk.

Comment 16

[Tom Ritter [:tjr]]

(In reply to David Major [:dmajor] from comment #15)
> I tested again with CFG actually enabled, and it's still failing left and
> right on try, but the sampling of failures that I looked at didn't reproduce
> for me locally, where I use clang tip.
> 
> This likely means that there are additional CFG fixes on 8.0.0 besides
> D54723 that we need. Next steps (for which I don't have cycles right now)
> would be to push a tip revision on try and confirm that we're green. If so,
> then we'd have to decide whether to spend the time to hunt down all the
> necessary patches, or bite the bullet and move our automation builds back to
> trunk.

Took me a few tries, but here's a bump to clang trunk using the commit that D54723 landed in. Still some crashes, but maybe reproducible? https://treeherder.mozilla.org/#/jobs?repo=try&revision=74c718cac0a4c4e2a9c695034a7da4d065825564&selectedJob=214445881

Comment 17

[(Away)]

(In reply to Tom Ritter [:tjr] from comment #16)
> Took me a few tries, but here's a bump to clang trunk using the commit that
> D54723 landed in. Still some crashes, but maybe reproducible?
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=74c718cac0a4c4e2a9c695034a7da4d065825564&selectedJob=2
> 14445881

Oh sorry, I forgot to mention that I had to use "guard:cf,nolongjmp" in ldflags. (Thanks to dxf for suggesting it to me a while back.) According to comments in chromium's build system, it's needed because the longjmp support depends on the compiler-side work that clang-cl doesn't yet have.

It's green! https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=31af62233167225280ad96a1c0d9a2966bdfcfe8

Comment 18

[Tom Ritter [:tjr]]

So if we wanted to land this (assuming we didn't have perf or compatibility problems, and we'd do it in the next Nightly cycle) - would we want to bump to clang trunk or backport patches?

Comment 19

[Tom Ritter [:tjr]]

Ran some Talos Tests.

Here's a try run bumping to clang trunk:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e63046e6fe02b511cd38945412529afd0127aeb
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=1e63046e6fe02b511cd38945412529afd0127aeb&framework=1&showOnlyComparable=1&showOnlyImportant=1&selectedTimeRange=172800

Here's a try run bumping to clang trunk and turning on CFG:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ce4956bf1cd91f8a67861c05cc7f577360ab6a3
And a comparison against the clang bump:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1e63046e6fe02b511cd38945412529afd0127aeb&newProject=try&newRevision=7ce4956bf1cd91f8a67861c05cc7f577360ab6a3&framework=1&showOnlyComparable=1&showOnlyImportant=1


There's some preliminary results as of this writing, but I'm going to send in retriggers to try and get some high confidence numbers.

Comment 20

[Tom Ritter [:tjr]]

Good news!

Bumping to clang trunk gets us considerable (9-15%) and consistent wins on a few tests. It might hurt responsiveness slightly.

Adding CFG does consistently hurt startup performance, but not much on 64 bit, and ~ 3.8% on 32 bit. 

Here's a full comparison of clangbump and CFG vs -central: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=7ce4956bf1cd&framework=1&filter=startup&showOnlyComparable=1&selectedTimeRange=172800

Joel, what are your thoughts on these results?

Comment 21

[(Away)]

(In reply to Tom Ritter [:tjr] from comment #18)
> So if we wanted to land this (assuming we didn't have perf or compatibility
> problems, and we'd do it in the next Nightly cycle) - would we want to bump
> to clang trunk or backport patches?

I talked to glandium this week and it sounds like we're ok for a one-time bump to trunk after merge day.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

I don't like how we lose responsiveness, that is one of the 3 areas of focus for performance in 2019 (page load, energy, latency - which responsiveness is related to)

Overall, most of the regressions are on win7, we are moving faster towards 64bit everywhere, so I don't worry about those.  If there is anything to do to reduce the responsiveness losses, that would be better.

Comment 23

[Tom Ritter [:tjr]]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #22)
> I don't like how we lose responsiveness, that is one of the 3 areas of focus
> for performance in 2019 (page load, energy, latency - which responsiveness
> is related to)
> 
> Overall, most of the regressions are on win7, we are moving faster towards
> 64bit everywhere, so I don't worry about those.  If there is anything to do
> to reduce the responsiveness losses, that would be better.

So we did a different compiler version, and it seems like it has changed the performance characteristics. We don't lose on responsiveness anymore, but we have more smaller losses on windows7-32. And if you look at the 2-day window there's huge gains for windows10-64-qr

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=80b648c837c7a318c3a2b01d8b3218df85e798cb&framework=1&showOnlyComparable=1&showOnlyImportant=1&selectedTimeRange=172800

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

this is promising- as for win7, this is still a very large portion of our active release users, but Firefox stands alone in providing new and updated products for windows 7- I am inclined to accept windows 7 regressions over windows 10 + QR gains.

2 things:
1) I have added raptor jobs to the push, lets wait for those results to come in
2) once those are in, lets sanity check with the perf team since this isn't just a single test change anymore.

Comment 25

[(Away)]

We should probably be using PGO builds for the perf comparisons as that's ultimately what users will get.

The fact that relatively small compiler changes can swing the numbers makes me wonder if these deltas are "real" -- that is, even if they are statistically significant within a single pair of pushes, they may be lost in the noise between pushes.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

good point on the PGO issue- we could also push again to try and compare two different builds

Comment 27

[Tom Ritter [:tjr]]

(In reply to David Major [:dmajor] from comment #25)
> We should probably be using PGO builds for the perf comparisons as that's
> ultimately what users will get.

Whoops; yes, my mistake. I added those too.

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #26)
> good point on the PGO issue- we could also push again to try and compare two
> different builds

Sent in https://treeherder.mozilla.org/#/jobs?repo=try&revision=482d09aa404e3b2cd0c1a6318a365725a427ffc6

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

as a note, raptor is showing similar trends:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=80b648c837c7a318c3a2b01d8b3218df85e798cb&framework=10&showOnlyComparable=1&showOnlyImportant=1&selectedTimeRange=172800

waiting on pgo builds, then more jobs to schedule, etc.

Comment 29

[Tom Ritter [:tjr]]

The pgo comparisons for both builds seem to indicate not much change?

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

I agree, pgo is balancing things out.

Comment 31

[(Away)]

Let me know if you have a new patch in bug 1512921 that you want me to look at.

Comment 32

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 Enable CFG for Windows builds

And this patch is rebased and includes the longjump bit.

Comment 33

[(Away)]

Comment on attachment 9031539 [details] [diff] [review]
Bug 1485016 Enable CFG for Windows builds

This one is just enough outside my comfort zone that I'd prefer an extra set of eyes on it.

Comment 34

[Nathan Froyd [:froydnj]]

Comment on attachment 9031539 [details] [diff] [review]
Bug 1485016 Enable CFG for Windows builds

Review of attachment 9031539 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/toolchain.configure
@@ +1594,5 @@
> +        if c_compiler.type == 'clang-cl':
> +            flags.append("-guard:cf")
> +            js_flags.append("-guard:cf")
> +            ldflags.append("-guard:cf,nolongjmp")
> +            js_ldflags.append("-guard:cf,nolongjmp")

Can you add a brief comment here explaining why `nolongjmp` is needed?

Comment 35

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 - Enable CFG for Windows builds r=froydnj

Carry forward r+ from Comment 34

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e548edaf318
Enable CFG for Windows builds r=froydnj

Comment 37

[Dorel Luca [:dluca]]

Backed out changeset 7e548edaf318 (bug 1485016) for build bustage

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217850857&repo=mozilla-inbound&lineNumber=1372

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7e548edaf31847113f4021aef20cfa5b3818ba9d

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80195c303212ba537cd3c9d35a622900336b896b

Comment 38

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 - Enable CFG for Windows builds r=froydnj

Carry forward r+ from Comment 34

I had a typo that only/fortunately broke the Android builds, fixing that and they're green again.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61ae84746b34
Enable CFG for Windows builds. r=froydnj

Comment 40

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/61ae84746b34

Comment 41

[Narcis Beleuzu [:NarcisB]]

Backed out as requested by tjr in https://bugzilla.mozilla.org/show_bug.cgi?id=1515631#c3

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/644731a71be81b31a049deee70170af3ca36efa1

Comment 42

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 - Enable CFG for Windows builds r?dmajor

Comment 43

[(Away)]

Comment on attachment 9039172 [details] [diff] [review]
Bug 1485016 - Enable CFG for Windows builds r?dmajor

Review of attachment 9039172 [details] [diff] [review]:
-----------------------------------------------------------------

To avoid re-causing bug 1515702 I think we should disallow CFG with compilers before clang 8.

::: build/moz.configure/toolchain.configure
@@ +1673,5 @@
> +# This function is a bit confusing. It adds or removes hardening flags in
> +# three stuations: if --enable-hardening is passed; if --disable-hardening
> +# is passed, and if no flag is passed.
> +#
> +# At time of this comment writing, all flags are actually added in the

I'm not so sure about this comment block: we have a ton of features that have equivalent behavior in the --enable case and the not-specified case; so in that sense hardening flags aren't particularly special. But if you think it helps clarify things, OK.

@@ +1731,1 @@
>      # If ASAN _is_ on, undefine FOTIFY_SOURCE just to be safe

Not required, but if you end up making a change to this diff, might want to fix that FOTIFY typo while you're there.

Comment 44

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 - Enable CFG for Windows builds r?dmajor

(In reply to David Major [:dmajor] from comment #43)

To avoid re-causing bug 1515702 I think we should disallow CFG with
compilers before clang 8.

Added

::: build/moz.configure/toolchain.configure
@@ +1673,5 @@

+# This function is a bit confusing. It adds or removes hardening flags in
+# three stuations: if --enable-hardening is passed; if --disable-hardening
+# is passed, and if no flag is passed.
+#
+# At time of this comment writing, all flags are actually added in the

I'm not so sure about this comment block: we have a ton of features that
have equivalent behavior in the --enable case and the not-specified case; so
in that sense hardening flags aren't particularly special. But if you think
it helps clarify things, OK.

I've had two or three non-build people confused about this recently, so I think it has value.

@@ +1731,1 @@

 # If ASAN _is_ on, undefine FOTIFY_SOURCE just to be safe

Not required, but if you end up making a change to this diff, might want to
fix that FOTIFY typo while you're there.

Fixed.

Comment 45

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 - Change clang-cl compiler version to be the clang version rather than MSVC r?dmajor

Comment 46

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 - Change clang-cl compiler version to be the clang version rather than MSVC r?dmajor

Comment 47

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 - Change clang-cl compiler version to be the clang version rather than MSVC r?dmajor

Comment 48

[Mike Hommey [:glandium]]

Comment on attachment 9039438 [details] [diff] [review]
Bug 1485016 - Change clang-cl compiler version to be the clang version rather than MSVC r?dmajor

Review of attachment 9039438 [details] [diff] [review]:
-----------------------------------------------------------------

Half of this is dealt with in bug 1523146. Can you "rebase"?

Comment 49

[(Away)]

Comment on attachment 9039188 [details] [diff] [review]
Bug 1485016 - Enable CFG for Windows builds r?dmajor

This looks good pending the other supporting patches.

Comment 50

[Tom Ritter [:tjr]]

Attachment: Bug 1485016 - Change clang-cl compiler version to be the clang version rather than MSVC r?glandium

Comment 51

[Tom Ritter [:tjr]]

Confirmed a clang-7 doesn't add the flags but a clang-8 does.

Comment 52

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c95ac72ccc07
https://hg.mozilla.org/integration/mozilla-inbound/rev/30a8448f1d27

Comment 53

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
potential sec improvement for uplift to 66

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c95ac72ccc07
https://hg.mozilla.org/mozilla-central/rev/30a8448f1d27

Comment 55

[Liz Henry (:lizzard) (relman/hg->git project)]

Would you like to request uplift to beta? Thanks!

Comment 56

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See How Do You Triage for more information

Comment 57

[Tom Ritter [:tjr]]

So I put this into -beta and see at least two consistent crashers... I wonder if these are known issues...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d006dfc650decfd245cab5d9559931508aac5d16&selectedJob=226925240

Comment 58

[Tom Ritter [:tjr]]

Comment on attachment 9039188 [details] [diff] [review]
Bug 1485016 - Enable CFG for Windows builds r?dmajor

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

We'd like to land this small security improvement in 66

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

Bug 1485016 (before this one), Bug 1485016 (after this one)

Risk to taking this patch

High

Why is the change risky/not risky? (and alternatives if risky)

We're enabling a Windows compiler mitigation that has the potential for unexpected behavior. We would need to observe crashes on Beta and see if we detect anything unusual; and back it out if so.

String changes made/needed

Comment 59

[Tom Ritter [:tjr]]

Fixed try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01b08e7e740730893823a046c74311c79167deff

Should be good now.

Comment 60

[(Away)]

Make sure to include the arm64 piece from bug 1525588 if this gets uplifted.

Comment 61

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9039188 [details] [diff] [review]
Bug 1485016 - Enable CFG for Windows builds r?dmajor

OK for beta uplift - land the patch in bug 1523003 first, then this patch in bug 1485016, then in bug 1525588

Comment 62

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0456b8fcad6f
https://hg.mozilla.org/releases/mozilla-beta/rev/76698f80b156