|
|
Created:
4 years ago by Sébastien Marchand Modified:
3 years, 7 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, wfh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionInstrument 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
Patch Set 1 #Patch Set 2 : create_installer_archive changes. #Patch Set 3 : create_installer_archive changes. #
Total comments: 11
Patch Set 4 : Rebase #
Total comments: 6
Patch Set 5 : BL compress syzyasan_rtl.dll #
Total comments: 1
Patch Set 6 : Fix the component build #
Total comments: 4
Messages
Total messages: 48 (29 generated)
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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@chromium.org changed reviewers: + grt@chromium.org
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
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 :)
Description was changed from ========== Instrument setup.exe in the SyzyAsan builds. BUG=557759 ========== to ========== Instrument setup.exe in the SyzyAsan builds. BUG=557759 ==========
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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).
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@chromium.org changed reviewers: + gab@chromium.org
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!
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 ==========
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? :)
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.
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/ :)
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!
Patchset #4 (id:60001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:160001) has been deleted
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.
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
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?
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)"
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.
Patchset #5 (id:200001) has been deleted
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?
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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).
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.
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@chromium.org changed reviewers: - gab@chromium.org
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
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 ========== |