Instrument setup.exe in the SyzyAsan builds.

Instrument setup.exe in the SyzyAsan builds.


This does several things:
- Update the rules that generate syzygy/setup.exe so it's an instrumented copy of setup.exe (instead of a simple copy).
- Use this instrumented version in syzygy/mini_installer.exe
- Add the SyzyAsan runtime library (syzyasan_rtl) as a runtime dependency for setup.exe, make sure that it can be found by the installer.

BUG=557759

[Sébastien Marchand, 2016-12-08 01:45:52]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 01:46:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 02:52:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 02:52:11]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[grt (UTC plus 2), 2016-12-08 14:20:06]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2016-12-08 14:20:07]

Cool start! If I'm not mistaken, we'll need changes to:
- create_installer_archive.py to include the runtime in packed_files.rc,
- mini_installer.cc to extract the runtime next to setup.exe, and
- maybe AddInstallerCopyTasks in install_worker.cc to copy the runtime next to
setup.exe where it's installed

[Sébastien Marchand, 2016-12-08 16:10:50]

On 2016/12/08 14:20:07, grt (UTC plus 1) wrote:
> Cool start! If I'm not mistaken, we'll need changes to:
> - create_installer_archive.py to include the runtime in packed_files.rc,
> - mini_installer.cc to extract the runtime next to setup.exe, and
> - maybe AddInstallerCopyTasks in install_worker.cc to copy the runtime next to
> setup.exe where it's installed

Yep, I think that I should be able to re-use (or get inspired from) the
component
installer stuffs. I'll add a [WIP] tag to my CL to reflect the fact that I'm not
done yet :)

[Sébastien Marchand, 2016-12-08 16:11:04]

Description was changed from

==========
Instrument setup.exe in the SyzyAsan builds.

BUG=557759
==========

to

==========
Instrument setup.exe in the SyzyAsan builds.

BUG=557759
==========

[Sébastien Marchand, 2016-12-08 21:15:34]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 21:16:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 23:43:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 23:43:40]

Dry run: This issue passed the CQ dry run.

[Sébastien Marchand, 2016-12-09 02:03:33]

This still isn't ready for real review, but as you're on the other side of the
world I thought that it'd be useful to share my progress now. With this new
patchset I'm getting syzyasan_rtl.dll and an instrumented version of setup.exe
in
C:\Users\%username%\AppData\Local\Chromium\Application\{version_number}\Installer

I don't know if it's the version of Chrome that's being used during the
installation or if it's just there for some other reasons (i.e. make it easier
to apply the delta patches), but this is promising.

I still need to add comments (and rewrite some) as well as editing the CL
description with more details on why I'm doing things this way, but so far I'm
pretty optimistic that we'll have this working before the end of the quarter
(end of next week for me as I'll then be on vacations).

[grt (UTC plus 2), 2016-12-09 08:30:54]

This is looking pretty good!

...\Chrome\Application\W.X.Y.Z\Installer\setup.exe is a copy of setup that is
used for uninstall, the patch source for diff updates, and a few other assorted
tasks. Having the runtime in place for it is one necessity.

When run by mini_installer.exe, setup.exe is in a temp dir and will need the
runtime with it. I think that generalizing the component build stuff here and in
mini_installer.cc should be enough for that.

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (left):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:373: resources.append((file,
'BN', os.path.join(installer_dir, file)))
this puts the component DLLs in as uncompressed blobs. for official
non-component SyzyASAN canaries, we may want the runtime to be LZX compressed
with makecab.exe as is the case with setup.exe (see PrepareSetupExec above). is
the runtime so small that it won't be a gain to compress? since setup.exe is
compressed for both component and non-component builds, i think it's likely okay
to compress all dependent dlls always rather than having extra logic to handle
component (developer-only) vs. official with SyzyASAN. i don't think there will
be a large build time cost, but maybe it's worth checking.

please search for COMPONENT_BUILD in mini_installer.cc. you'll see that the code
to extract these extra resources isn't included in official builds. that'll have
to change. for the sake of consistency, maybe it would be a good thing for setup
and all of its dependencies to be LZX compressed so that the handling can be
uniform both here and in mini_installer.cc.

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:494: # TODO(jbauman): Remove
when GYP is deprecated on Windows.
w00t!

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:482: 
nit: extra blank line

[grt (UTC plus 2), 2016-12-09 08:34:54]

grt@chromium.org changed reviewers:
+ gab@chromium.org

[grt (UTC plus 2), 2016-12-09 08:34:54]

Hey Gab: for component builds, does setup.exe have its dependent DLLs next to it
in ...\Application\W.X.Y.Z\Installer, or does it use manifest magic to use the
DLLs one directory up? If the latter, can we use the same strategy to have
setup.exe find the SyzyASAN runtime? Thanks!

[Sébastien Marchand, 2016-12-09 12:23:12]

Description was changed from

==========
Instrument setup.exe in the SyzyAsan builds.

BUG=557759
==========

to

==========
Instrument setup.exe in the SyzyAsan builds.

This is still a WIP, several things are missing:
- Comments!
- I need to do more manual tests (for the SyzyAsan installer and to make sure
that I'm not breaking anything else).
- Comments!

This does several things:
- Update the rules that generate syzygy/setup.exe so it's an instrumented copy
of setup.exe (instead of a simple copy).
- Use this instrumented version in syzygy/mini_installer.exe
- Add the SyzyAsan runtime library (syzyasan_rtl) as a runtime dependency for
setup.exe, make sure that it can be found by the installer.

BUG=557759
==========

[gab, 2016-12-09 19:23:25]

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (left):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:494: # TODO(jbauman): Remove
when GYP is deprecated on Windows.
On 2016/12/09 08:30:54, grt (UTC plus 1) wrote:
> w00t!

@grt: re. does setup.exe use the version manifest trick? Not anymore, it's been
using this explicit list for while, and with GN
(https://codereview.chromium.org/2075863003) it uses an automatically generated
list of runtime deps :).

Also, to simplify further post-XP, we could do get rid of the version manifest
in favor of an application config (Win7+) http://crbug.com/581133

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:506: # TODO(jbauman): Remove
when GYP is deprecated on Windows.
While you're here, also resolve this? :)

[gab, 2016-12-09 19:24:46]

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:506: # TODO(jbauman): Remove
when GYP is deprecated on Windows.
On 2016/12/09 19:23:24, gab wrote:
> While you're here, also resolve this? :)

Actually, cleaning up the post GYP era is slightly more work than belongs in
this CL. I'll ping jbauman to find an owner for this.

[gab, 2016-12-12 16:20:00]

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:506: # TODO(jbauman): Remove
when GYP is deprecated on Windows.
On 2016/12/09 19:24:46, gab wrote:
> On 2016/12/09 19:23:24, gab wrote:
> > While you're here, also resolve this? :)
> 
> Actually, cleaning up the post GYP era is slightly more work than belongs in
> this CL. I'll ping jbauman to find an owner for this.

FYI, jbauman took care of this in https://codereview.chromium.org/2556253009/ :)

[grt (UTC plus 2), 2016-12-12 20:12:33]

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:506: # TODO(jbauman): Remove
when GYP is deprecated on Windows.
On 2016/12/12 16:20:00, gab wrote:
> On 2016/12/09 19:24:46, gab wrote:
> > On 2016/12/09 19:23:24, gab wrote:
> > > While you're here, also resolve this? :)
> > 
> > Actually, cleaning up the post GYP era is slightly more work than belongs in
> > this CL. I'll ping jbauman to find an owner for this.
> 
> FYI, jbauman took care of this in https://codereview.chromium.org/2556253009/
:)

Thanks, Gab!

[Sébastien Marchand, 2016-12-13 16:52:07]

Patchset #4 (id:60001) has been deleted

[Sébastien Marchand, 2017-03-06 21:26:39]

Patchset #7 (id:140001) has been deleted

[Sébastien Marchand, 2017-03-06 21:26:47]

Patchset #6 (id:120001) has been deleted

[Sébastien Marchand, 2017-03-06 21:26:55]

Patchset #5 (id:100001) has been deleted

[Sébastien Marchand, 2017-03-06 21:27:11]

Patchset #4 (id:80001) has been deleted

[Sébastien Marchand, 2017-03-06 21:41:56]

Patchset #4 (id:160001) has been deleted

[Sébastien Marchand, 2017-03-06 21:52:26]

There's nothing blocking this anymore, I've tried a mini_installer with this
patchset on a VM and it worked as expected (an instrumented version of setup.exe
was used during the build). There's still some cleanup to do (mostly in
create_installer_archive.py) but not so much.

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (left):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:373: resources.append((file,
'BN', os.path.join(installer_dir, file)))
On 2016/12/09 08:30:54, grt (UTC plus 1) wrote:
> this puts the component DLLs in as uncompressed blobs. for official
> non-component SyzyASAN canaries, we may want the runtime to be LZX compressed
> with makecab.exe as is the case with setup.exe (see PrepareSetupExec above).
is
> the runtime so small that it won't be a gain to compress? since setup.exe is
> compressed for both component and non-component builds, i think it's likely
okay
> to compress all dependent dlls always rather than having extra logic to handle
> component (developer-only) vs. official with SyzyASAN. i don't think there
will
> be a large build time cost, but maybe it's worth checking.
> 
> please search for COMPONENT_BUILD in mini_installer.cc. you'll see that the
code
> to extract these extra resources isn't included in official builds. that'll
have
> to change. for the sake of consistency, maybe it would be a good thing for
setup
> and all of its dependencies to be LZX compressed so that the handling can be
> uniform both here and in mini_installer.cc.

Sure, if you think that all the dll dependencies should always be LZX compressed
then this could probably be done on a separate CL before this one.

syzyasan_rtl.dll is ~500ko, compressing it reduce its size to ~250ko.

What's your preference here? Should I split this CL in two (split the Syzyasan
part from the LZX compression stuff).

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:482: 
On 2016/12/09 08:30:54, grt (UTC plus 1) wrote:
> nit: extra blank line

Done.

[grt (UTC plus 2), 2017-03-10 07:57:36]

Nice! I gave it a try in my checkout to see how things worked. I see that the
runtime is getting extracted from the mini_installer and used. Yay. I also see
that setup.exe crashes during exit while tearing down TLS. Boo. Here's the crash
stack:

1:004:x86> kp
 # ChildEBP RetAddr  
00 00cff798 00f3846c setup!`anonymous namespace'::OnThreadExitInternal(struct
`anonymous-namespace'::TlsVectorEntry * tls_data = 0x2756f5f8)+0x35
[c:\src\chromium\src\base\threading\thread_local_storage.cc @ 180]
01 00cff7a0 00f3c710
setup!base::internal::PlatformThreadLocalStorage::OnThreadExit(void)+0x1d
[c:\src\chromium\src\base\threading\thread_local_storage.cc @ 244]
02 00cff7a8 771de58e setup!OnThreadExit(void * module = 0x00ec0000, unsigned
long reason = 0, void * reserved = 0x00000000)+0x14
[c:\src\chromium\src\base\threading\thread_local_storage_win.cc @ 71]
03 00cff7c8 771b0e46 ntdll_77170000!LdrxCallInitRoutine+0x16
04 00cff818 771f8d20 ntdll_77170000!LdrpCallInitRoutine+0x43
05 00cff860 7719a8e8 ntdll_77170000!LdrpCallTlsInitializers+0x5dd62
06 00cff8f8 7719a416 ntdll_77170000!LdrShutdownProcess+0x198
07 00cff9cc 770badc2 ntdll_77170000!RtlExitUserProcess+0x96
08 00cff9e0 00fa9998 KERNEL32!ExitProcessImplementation+0x12
09 00cff9ec 00fa994c setup!exit_or_terminate_process(unsigned int return_code =
<Value unavailable error>)+0x41
[d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 129]
0a 00cffa08 00fa9a8f setup!common_exit(int return_code = <Value unavailable
error>, _crt_exit_cleanup_mode cleanup_mode = _crt_exit_full_cleanup (0n0),
_crt_exit_return_mode return_mode = _crt_exit_terminate_process (0n0))+0x55
[d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 265]
0b 00cffa1c 00f96622 setup!exit(int return_code = 0n0)+0x11
[d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 278]
0c 00cffa5c 770a62c4 setup!__scrt_common_main_seh(void)+0x107
[f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 262]
0d 00cffa70 771d0fd9 KERNEL32!BaseThreadInitThunk+0x24
0e 00cffab8 771d0fa4 ntdll_77170000!__RtlUserThreadStart+0x2f
0f 00cffac8 00000000 ntdll_77170000!_RtlUserThreadStart+0x1b

Ring any bells?

https://codereview.chromium.org/2559053002/diff/180001/chrome/installer/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2559053002/diff/180001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:19: #include
"base/debug/asan_invalid_access.h"
nit: put this in a #if defined(SYZYASAN) block after line 85

https://codereview.chromium.org/2559053002/diff/180001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1073: typedef VOID(WINAPI *
SyzyAsanInitializeCrashReporterFn)();
nit: prefer "using" over "typedef"

https://codereview.chromium.org/2559053002/diff/180001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1081: if (syzyasan_init_crash_reporter) {
nit: omit braces for one-liner

[grt (UTC plus 2), 2017-03-10 09:08:10]

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
File chrome/tools/build/win/create_installer_archive.py (left):

https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
chrome/tools/build/win/create_installer_archive.py:373: resources.append((file,
'BN', os.path.join(installer_dir, file)))
On 2017/03/06 21:52:26, Sébastien Marchand wrote:
> On 2016/12/09 08:30:54, grt (UTC plus 1) wrote:
> > this puts the component DLLs in as uncompressed blobs. for official
> > non-component SyzyASAN canaries, we may want the runtime to be LZX
compressed
> > with makecab.exe as is the case with setup.exe (see PrepareSetupExec above).
> is
> > the runtime so small that it won't be a gain to compress? since setup.exe is
> > compressed for both component and non-component builds, i think it's likely
> okay
> > to compress all dependent dlls always rather than having extra logic to
handle
> > component (developer-only) vs. official with SyzyASAN. i don't think there
> will
> > be a large build time cost, but maybe it's worth checking.
> > 
> > please search for COMPONENT_BUILD in mini_installer.cc. you'll see that the
> code
> > to extract these extra resources isn't included in official builds. that'll
> have
> > to change. for the sake of consistency, maybe it would be a good thing for
> setup
> > and all of its dependencies to be LZX compressed so that the handling can be
> > uniform both here and in mini_installer.cc.
> 
> Sure, if you think that all the dll dependencies should always be LZX
compressed
> then this could probably be done on a separate CL before this one.
> 
> syzyasan_rtl.dll is ~500ko, compressing it reduce its size to ~250ko.
> 
> What's your preference here? Should I split this CL in two (split the Syzyasan
> part from the LZX compression stuff).

Yeah, I think it's worth doing the compression. Do you have reason to think
250KB isn't work it?

[Sébastien Marchand, 2017-03-10 15:17:15]

On 2017/03/10 09:08:10, grt (UTC plus 1) wrote:
>
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
> File chrome/tools/build/win/create_installer_archive.py (left):
> 
>
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
> chrome/tools/build/win/create_installer_archive.py:373:
resources.append((file,
> 'BN', os.path.join(installer_dir, file)))
> On 2017/03/06 21:52:26, Sébastien Marchand wrote:
> > On 2016/12/09 08:30:54, grt (UTC plus 1) wrote:
> > > this puts the component DLLs in as uncompressed blobs. for official
> > > non-component SyzyASAN canaries, we may want the runtime to be LZX
> compressed
> > > with makecab.exe as is the case with setup.exe (see PrepareSetupExec
above).
> > is
> > > the runtime so small that it won't be a gain to compress? since setup.exe
is
> > > compressed for both component and non-component builds, i think it's
likely
> > okay
> > > to compress all dependent dlls always rather than having extra logic to
> handle
> > > component (developer-only) vs. official with SyzyASAN. i don't think there
> > will
> > > be a large build time cost, but maybe it's worth checking.
> > > 
> > > please search for COMPONENT_BUILD in mini_installer.cc. you'll see that
the
> > code
> > > to extract these extra resources isn't included in official builds.
that'll
> > have
> > > to change. for the sake of consistency, maybe it would be a good thing for
> > setup
> > > and all of its dependencies to be LZX compressed so that the handling can
be
> > > uniform both here and in mini_installer.cc.
> > 
> > Sure, if you think that all the dll dependencies should always be LZX
> compressed
> > then this could probably be done on a separate CL before this one.
> > 
> > syzyasan_rtl.dll is ~500ko, compressing it reduce its size to ~250ko.
> > 
> > What's your preference here? Should I split this CL in two (split the
Syzyasan
> > part from the LZX compression stuff).
> 
> Yeah, I think it's worth doing the compression. Do you have reason to think
> 250KB isn't work it?

Yep, my question was more: "Do you want to first make the change to always
compress
all the dependent dlls? (it won't be a SyzyAsan specific change)"

[grt (UTC plus 2), 2017-03-10 16:28:25]

On 2017/03/10 15:17:15, Sébastien Marchand wrote:
> On 2017/03/10 09:08:10, grt (UTC plus 1) wrote:
> >
>
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
> > File chrome/tools/build/win/create_installer_archive.py (left):
> > 
> >
>
https://codereview.chromium.org/2559053002/diff/40001/chrome/tools/build/win/...
> > chrome/tools/build/win/create_installer_archive.py:373:
> resources.append((file,
> > 'BN', os.path.join(installer_dir, file)))
> > On 2017/03/06 21:52:26, Sébastien Marchand wrote:
> > > On 2016/12/09 08:30:54, grt (UTC plus 1) wrote:
> > > > this puts the component DLLs in as uncompressed blobs. for official
> > > > non-component SyzyASAN canaries, we may want the runtime to be LZX
> > compressed
> > > > with makecab.exe as is the case with setup.exe (see PrepareSetupExec
> above).
> > > is
> > > > the runtime so small that it won't be a gain to compress? since
setup.exe
> is
> > > > compressed for both component and non-component builds, i think it's
> likely
> > > okay
> > > > to compress all dependent dlls always rather than having extra logic to
> > handle
> > > > component (developer-only) vs. official with SyzyASAN. i don't think
there
> > > will
> > > > be a large build time cost, but maybe it's worth checking.
> > > > 
> > > > please search for COMPONENT_BUILD in mini_installer.cc. you'll see that
> the
> > > code
> > > > to extract these extra resources isn't included in official builds.
> that'll
> > > have
> > > > to change. for the sake of consistency, maybe it would be a good thing
for
> > > setup
> > > > and all of its dependencies to be LZX compressed so that the handling
can
> be
> > > > uniform both here and in mini_installer.cc.
> > > 
> > > Sure, if you think that all the dll dependencies should always be LZX
> > compressed
> > > then this could probably be done on a separate CL before this one.
> > > 
> > > syzyasan_rtl.dll is ~500ko, compressing it reduce its size to ~250ko.
> > > 
> > > What's your preference here? Should I split this CL in two (split the
> Syzyasan
> > > part from the LZX compression stuff).
> > 
> > Yeah, I think it's worth doing the compression. Do you have reason to think
> > 250KB isn't work it?
> 
> Yep, my question was more: "Do you want to first make the change to always
> compress
> all the dependent dlls? (it won't be a SyzyAsan specific change)"

Yeah, makes sense to pull that out into its own CL. Thanks.

[Sébastien Marchand, 2017-04-05 20:57:01]

Patchset #5 (id:200001) has been deleted

[Sébastien Marchand, 2017-04-05 21:07:02]

Using Makecab on all the deps (in a component build) is really slow, so I'm only
using it on syzyasan_rtl.dll.

There's probably some minor cleanup to do but this version seems to work fine.

https://codereview.chromium.org/2559053002/diff/180001/chrome/installer/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/2559053002/diff/180001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:19: #include
"base/debug/asan_invalid_access.h"
On 2017/03/10 07:57:36, grt (UTC plus 2) wrote:
> nit: put this in a #if defined(SYZYASAN) block after line 85

Done.

https://codereview.chromium.org/2559053002/diff/180001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1073: typedef VOID(WINAPI *
SyzyAsanInitializeCrashReporterFn)();
On 2017/03/10 07:57:36, grt (UTC plus 2) wrote:
> nit: prefer "using" over "typedef"

Done.

https://codereview.chromium.org/2559053002/diff/180001/chrome/installer/setup...
chrome/installer/setup/setup_main.cc:1081: if (syzyasan_init_crash_reporter) {
On 2017/03/10 07:57:36, grt (UTC plus 2) wrote:
> nit: omit braces for one-liner

Done.

https://codereview.chromium.org/2559053002/diff/220001/chrome/installer/mini_...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/2559053002/diff/220001/chrome/installer/mini_...
chrome/installer/mini_installer/mini_installer.cc:430: success =
mini_installer::Expand(context->syzyasan_resource_path->get(),
This probably could be moved into a function, not sure if it's worth it?

[Sébastien Marchand, 2017-04-05 21:07:31]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 21:08:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 21:14:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 21:14:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Sébastien Marchand, 2017-04-05 22:34:55]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 22:35:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 01:15:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 01:15:17]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-04-06 10:38:54]

On 2017/03/10 07:57:36, grt (no reviews Apr 7-17) wrote:
> Nice! I gave it a try in my checkout to see how things worked. I see that the
> runtime is getting extracted from the mini_installer and used. Yay. I also see
> that setup.exe crashes during exit while tearing down TLS. Boo. Here's the
crash
> stack:
> 
> 1:004:x86> kp
>  # ChildEBP RetAddr  
> 00 00cff798 00f3846c setup!`anonymous namespace'::OnThreadExitInternal(struct
> `anonymous-namespace'::TlsVectorEntry * tls_data = 0x2756f5f8)+0x35
> [c:\src\chromium\src\base\threading\thread_local_storage.cc @ 180]
> 01 00cff7a0 00f3c710
> setup!base::internal::PlatformThreadLocalStorage::OnThreadExit(void)+0x1d
> [c:\src\chromium\src\base\threading\thread_local_storage.cc @ 244]
> 02 00cff7a8 771de58e setup!OnThreadExit(void * module = 0x00ec0000, unsigned
> long reason = 0, void * reserved = 0x00000000)+0x14
> [c:\src\chromium\src\base\threading\thread_local_storage_win.cc @ 71]
> 03 00cff7c8 771b0e46 ntdll_77170000!LdrxCallInitRoutine+0x16
> 04 00cff818 771f8d20 ntdll_77170000!LdrpCallInitRoutine+0x43
> 05 00cff860 7719a8e8 ntdll_77170000!LdrpCallTlsInitializers+0x5dd62
> 06 00cff8f8 7719a416 ntdll_77170000!LdrShutdownProcess+0x198
> 07 00cff9cc 770badc2 ntdll_77170000!RtlExitUserProcess+0x96
> 08 00cff9e0 00fa9998 KERNEL32!ExitProcessImplementation+0x12
> 09 00cff9ec 00fa994c setup!exit_or_terminate_process(unsigned int return_code
=
> <Value unavailable error>)+0x41
> [d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 129]
> 0a 00cffa08 00fa9a8f setup!common_exit(int return_code = <Value unavailable
> error>, _crt_exit_cleanup_mode cleanup_mode = _crt_exit_full_cleanup (0n0),
> _crt_exit_return_mode return_mode = _crt_exit_terminate_process (0n0))+0x55
> [d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 265]
> 0b 00cffa1c 00f96622 setup!exit(int return_code = 0n0)+0x11
> [d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 278]
> 0c 00cffa5c 770a62c4 setup!__scrt_common_main_seh(void)+0x107
> [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 262]
> 0d 00cffa70 771d0fd9 KERNEL32!BaseThreadInitThunk+0x24
> 0e 00cffab8 771d0fa4 ntdll_77170000!__RtlUserThreadStart+0x2f
> 0f 00cffac8 00000000 ntdll_77170000!_RtlUserThreadStart+0x1b

Do you see this TLS teardown crash? I found it by running the installer in
windbg (.childdbg 1 before letting the mini_installer run).

[Sébastien Marchand, 2017-04-06 17:35:46]

On 2017/04/06 10:38:54, grt (no reviews Apr 7-17) wrote:
> On 2017/03/10 07:57:36, grt (no reviews Apr 7-17) wrote:
> > Nice! I gave it a try in my checkout to see how things worked. I see that
the
> > runtime is getting extracted from the mini_installer and used. Yay. I also
see
> > that setup.exe crashes during exit while tearing down TLS. Boo. Here's the
> crash
> > stack:
> > 
> > 1:004:x86> kp
> >  # ChildEBP RetAddr  
> > 00 00cff798 00f3846c setup!`anonymous
namespace'::OnThreadExitInternal(struct
> > `anonymous-namespace'::TlsVectorEntry * tls_data = 0x2756f5f8)+0x35
> > [c:\src\chromium\src\base\threading\thread_local_storage.cc @ 180]
> > 01 00cff7a0 00f3c710
> > setup!base::internal::PlatformThreadLocalStorage::OnThreadExit(void)+0x1d
> > [c:\src\chromium\src\base\threading\thread_local_storage.cc @ 244]
> > 02 00cff7a8 771de58e setup!OnThreadExit(void * module = 0x00ec0000, unsigned
> > long reason = 0, void * reserved = 0x00000000)+0x14
> > [c:\src\chromium\src\base\threading\thread_local_storage_win.cc @ 71]
> > 03 00cff7c8 771b0e46 ntdll_77170000!LdrxCallInitRoutine+0x16
> > 04 00cff818 771f8d20 ntdll_77170000!LdrpCallInitRoutine+0x43
> > 05 00cff860 7719a8e8 ntdll_77170000!LdrpCallTlsInitializers+0x5dd62
> > 06 00cff8f8 7719a416 ntdll_77170000!LdrShutdownProcess+0x198
> > 07 00cff9cc 770badc2 ntdll_77170000!RtlExitUserProcess+0x96
> > 08 00cff9e0 00fa9998 KERNEL32!ExitProcessImplementation+0x12
> > 09 00cff9ec 00fa994c setup!exit_or_terminate_process(unsigned int
return_code
> =
> > <Value unavailable error>)+0x41
> > [d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 129]
> > 0a 00cffa08 00fa9a8f setup!common_exit(int return_code = <Value unavailable
> > error>, _crt_exit_cleanup_mode cleanup_mode = _crt_exit_full_cleanup (0n0),
> > _crt_exit_return_mode return_mode = _crt_exit_terminate_process (0n0))+0x55
> > [d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 265]
> > 0b 00cffa1c 00f96622 setup!exit(int return_code = 0n0)+0x11
> > [d:\rs1\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp @ 278]
> > 0c 00cffa5c 770a62c4 setup!__scrt_common_main_seh(void)+0x107
> > [f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl @ 262]
> > 0d 00cffa70 771d0fd9 KERNEL32!BaseThreadInitThunk+0x24
> > 0e 00cffab8 771d0fa4 ntdll_77170000!__RtlUserThreadStart+0x2f
> > 0f 00cffac8 00000000 ntdll_77170000!_RtlUserThreadStart+0x1b
> 
> Do you see this TLS teardown crash? I found it by running the installer in
> windbg (.childdbg 1 before letting the mini_installer run).

Ha, forgot about this but I've tried this in Windbg and haven't seen this issue
(Child
processes debugging was enabled), I've tried again and I don't hit this error.

[grt (UTC plus 2), 2017-04-19 13:17:36]

On 2017/04/06 17:35:46, Sébastien Marchand wrote:
> Ha, forgot about this but I've tried this in Windbg and haven't seen this
issue
> (Child
> processes debugging was enabled), I've tried again and I don't hit this error.

Huh. I'm rebuilding in my checkout to see if it still happens to me for some
reason. I'll take another review pass on this within the next 24h. Thanks for
your patience.

[gab, 2017-04-19 19:06:14]

gab@chromium.org changed reviewers:
- gab@chromium.org

[grt (UTC plus 2), 2017-04-20 09:30:51]

i think you're getting really close. maybe now is a good time to clean up the CL
description.

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
chrome/installer/mini_installer/mini_installer.cc:343: Context* context) {
i had thought of Context as an implementation detail of UnpackBinaryResources.
did you change it to avoid having a variable number of args here? if you think
this is cleaner, please add some additional documentation to this fn so that
callers know how to prepare |context|.

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
chrome/installer/mini_installer/mini_installer.cc:865: Context context = {
maybe it's cleaner have Context contain the output PathString instances
themselves rather than pointers to them. also, it's seems a bit awkward to pass
|base_path| in Context rather than as its own arg to UnpackBinaryResources. if
you really want to avoid the #if around the args to UBR, maybe this public
Context should become ResourcePaths with the two or three output paths,
base_bath should be passed separately, and Context should stay an impl detail of
UBR (containing a pointer to base_path and a pointer to ResourcePaths). wdyt?

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
File chrome/installer/mini_installer/mini_installer_constants.cc (right):

https://codereview.chromium.org/2559053002/diff/240001/chrome/installer/mini_...
chrome/installer/mini_installer/mini_installer_constants.cc:17: const wchar_t
kSyzyAsanRuntimePrefix[] = L"syzyasan_rtl";
if this is only used in mini_installer.cc, move it there.

https://codereview.chromium.org/2559053002/diff/240001/chrome/tools/build/win...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/2559053002/diff/240001/chrome/tools/build/win...
chrome/tools/build/win/create_installer_archive.py:338:
os.path.join(options.build_dir, SETUP_EXEC),
in my checkout, i find that this is compressing the pre-syzyasan setup.exe, not
the one in the syzygy dir. i think you need to plumb through a way for the
script invoker to pass in an optional path to setup.exe, or something, so that
the mini_installer_syzygy target uses syzygy/setup.exe rather than setup.exe. or
am i building it wrong?

C:\src\chromium\src>type out\asan\args.gn
enable_precompiled_headers = false
is_chrome_branded = true
is_debug = false
is_syzyasan = true
target_cpu = "x86"

C:\src\chromium\src>ninja -c out\asan mini_installer_syzygy

[Sébastien Marchand, 2017-04-28 20:38:30]

Description was changed from

==========
Instrument setup.exe in the SyzyAsan builds.

This is still a WIP, several things are missing:
- Comments!
- I need to do more manual tests (for the SyzyAsan installer and to make sure
that I'm not breaking anything else).
- Comments!

This does several things:
- Update the rules that generate syzygy/setup.exe so it's an instrumented copy
of setup.exe (instead of a simple copy).
- Use this instrumented version in syzygy/mini_installer.exe
- Add the SyzyAsan runtime library (syzyasan_rtl) as a runtime dependency for
setup.exe, make sure that it can be found by the installer.

BUG=557759
==========

to

==========
Instrument setup.exe in the SyzyAsan builds.


This does several things:
- Update the rules that generate syzygy/setup.exe so it's an instrumented copy
of setup.exe (instead of a simple copy).
- Use this instrumented version in syzygy/mini_installer.exe
- Add the SyzyAsan runtime library (syzyasan_rtl) as a runtime dependency for
setup.exe, make sure that it can be found by the installer.

BUG=557759
==========