Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1885593003:
Delete build/mac/change_mach_o_flags.py and friends. (Closed)
|
Created:
4 years, 8 months ago by Robert Sesek Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelete build/mac/change_mach_o_flags.py and friends.
This was only used to mark executable as non-PIE. After the non-PIE Chromium
helper was removed, the last remaining use was for Valgrind. But Valgrind
support was also recently removed, so this can go away entirely.
change_mach_o_flags.py did explicitly set the MH_NO_HEAP_EXECUTION flag on
executables, but this is not necessary on x86_64 where the architecture default
is to not have an executable heap.
BUG=520680
Committed: https://crrev.com/806f385a1f9c2814de91bd1a98a50e74c4fa9a81
Cr-Commit-Position: refs/heads/master@{#387007}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 20 (8 generated)
rsesek@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/1885593003/diff/1/build/mac/change_mach_o_fla... File build/mac/change_mach_o_flags.py (left): https://codereview.chromium.org/1885593003/diff/1/build/mac/change_mach_o_fla... build/mac/change_mach_o_flags.py:50: for non-i386 executables, including x86_64 executables. Apple's linker only I'm confused by this comment. The first paragraph says "64-bit executables always have hardware protection", but then this says "this script is useful for toggling this bit for 64-bit executables". In a test binary, the bit isn't set by default at least: thakis-macpro:src thakis$ xxd a.out | head -2 0000000: cffa edfe 0700 0001 0300 0080 0200 0000 ................ 0000010: 0e00 0000 2803 0000 8500 2000 0000 0000 ....(..... ..... The framework does have this bit: thakis-macpro:src thakis$ xxd /Applications/Google\ Chrome.app/Contents/Versions/49.0.2623.87/Google\ Chrome\ Framework.framework/Google\ Chrome\ Framework | head -2 0000000: cffa edfe 0700 0001 0300 0000 0600 0000 ................ 0000010: 2f00 0000 6819 0000 8580 1100 0000 0000 /...h........... So the CL presumably makes the MH_NO_HEAP_EXECUTION bit disappear. Is that intentional? If so, is the reasoning that this shouldn'e effectively change behavior?
rsesek@chromium.org changed reviewers: + mark@chromium.org
https://codereview.chromium.org/1885593003/diff/1/build/mac/change_mach_o_fla... File build/mac/change_mach_o_flags.py (left): https://codereview.chromium.org/1885593003/diff/1/build/mac/change_mach_o_fla... build/mac/change_mach_o_flags.py:50: for non-i386 executables, including x86_64 executables. Apple's linker only On 2016/04/12 20:10:50, Nico wrote: > I'm confused by this comment. The first paragraph says "64-bit executables > always have hardware protection", but then this says "this script is useful for > toggling this bit for 64-bit executables". The comment is correct but a little confusing. By default 64-bit apps get NX data and stack, but an executable can force NX by setting the MH_NO_HEAP_EXECUTION bit, since it is possible to override the architecture default using sysctl. On i386 only, ld has a flag -allow_heap_execute that can be used to inhibit the MH_NO_HEAP_EXECUTION bit when linking. This comment in XNU at override_nx() is maybe a little more clear: http://opensource.apple.com//source/xnu/xnu-3248.20.55/osfmk/vm/vm_map.c. You can also verify the current state of NX: rsesek@hotwire:/Users/rsesek % sysctl vm.allow_data_exec vm.allow_data_exec: 1 rsesek@hotwire:/Users/rsesek % sysctl vm.allow_stack_exec vm.allow_stack_exec: 0 Where 0x1 is VM_ABI_32 and 0x2 is VM_ABI_64 (from vm_map.h). So it is true that we are losing the forceful MH_NO_HEAP_EXECUTION bit and relying on the 64-bit architecture default for NX (that a user could override if they were crazy). But for stock installs, this shouldn't have any effect on the NX behavior. Mark may have an opinion on this, so I can let this wait till tomorrow. > In a test binary, the bit isn't set by default at least: > > thakis-macpro:src thakis$ xxd a.out | head -2 > 0000000: cffa edfe 0700 0001 0300 0080 0200 0000 ................ > 0000010: 0e00 0000 2803 0000 8500 2000 0000 0000 ....(..... ..... > > The framework does have this bit: > > thakis-macpro:src thakis$ xxd /Applications/Google\ > Chrome.app/Contents/Versions/49.0.2623.87/Google\ Chrome\ > Framework.framework/Google\ Chrome\ Framework | head -2 > 0000000: cffa edfe 0700 0001 0300 0000 0600 0000 ................ > 0000010: 2f00 0000 6819 0000 8580 1100 0000 0000 /...h........... > > So the CL presumably makes the MH_NO_HEAP_EXECUTION bit disappear. Is that > intentional? If so, is the reasoning that this shouldn'e effectively change > behavior? The framework does not have the bit. Compare without this change: rsesek@hotwire:/Volumes/Build/src(remotes/origin/HEAD) % otool -hv out/Release/Chromium.app/Contents/MacOS/Chromium out/Release/Chromium.app/Contents/MacOS/Chromium: Mach header magic cputype cpusubtype caps filetype ncmds sizeofcmds flags MH_MAGIC_64 X86_64 ALL LIB64 EXECUTE 21 2336 NOUNDEFS DYLDLINK TWOLEVEL PIE MH_NO_HEAP_EXECUTION rsesek@hotwire:/Volumes/Build/src(remotes/origin/HEAD) % otool -vh out/Release/Chromium.app/Contents/Versions/52.0.2707.0/Chromium\ Framework.framework/Chromium\ Framework out/Release/Chromium.app/Contents/Versions/52.0.2707.0/Chromium Framework.framework/Chromium Framework: Mach header magic cputype cpusubtype caps filetype ncmds sizeofcmds flags MH_MAGIC_64 X86_64 ALL 0x00 DYLIB 46 6408 NOUNDEFS DYLDLINK TWOLEVEL WEAK_DEFINES BINDS_TO_WEAK NO_REEXPORTED_DYLIBS Versus with this change: rsesek@hotwire:/Volumes/Build/src(change-macho-flags) % otool -hv out/Release/Chromium.app/Contents/MacOS/Chromium out/Release/Chromium.app/Contents/MacOS/Chromium: Mach header magic cputype cpusubtype caps filetype ncmds sizeofcmds flags MH_MAGIC_64 X86_64 ALL LIB64 EXECUTE 21 2336 NOUNDEFS DYLDLINK TWOLEVEL PIE rsesek@hotwire:/Volumes/Build/src(change-macho-flags) % otool -vh out/Release/Chromium.app/Contents/Versions/52.0.2707.0/Chromium\ Framework.framework/Chromium\ Framework out/Release/Chromium.app/Contents/Versions/52.0.2707.0/Chromium Framework.framework/Chromium Framework: Mach header magic cputype cpusubtype caps filetype ncmds sizeofcmds flags MH_MAGIC_64 X86_64 ALL 0x00 DYLIB 46 6408 NOUNDEFS DYLDLINK TWOLEVEL WEAK_DEFINES BINDS_TO_WEAK NO_REEXPORTED_DYLIBS
Cool, lgtm, but say in the CL description that this removes MH_NO_HEAP_EXECUTION from the executable and why that's ok. And let's wait for Mark too, as you suggest.
I do have a slight preference for keeping MH_NO_HEAP_EXECUTION even when the sysctl gets set badly, but that alone probably isn’t enough of a reason to keep the script around if there are competing reasons to get rid of it. LGTM, use your judgment.
On 2016/04/13 15:28:53, Mark Mentovai wrote: > I do have a slight preference for keeping MH_NO_HEAP_EXECUTION even when the > sysctl gets set badly, but that alone probably isn’t enough of a reason to keep > the script around if there are competing reasons to get rid of it. LGTM, use > your judgment. I also have a slight preference for keeping it, but not at the cost of an entire build step. I talked to jschuh@ about what Windows would do in this scenario and he agreed that getting rid of this was fine. If ld gave us an option to set this using a flag, of course we'd set it. Filed https://openradar.appspot.com/25704436 asking for it.
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1885593003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885593003/1
The CQ bit was unchecked by thakis@chromium.org
(please update cl description to mention execute bit stuff before landing)
Description was changed from ========== Delete build/mac/change_mach_o_flags.py and friends. This was only used to mark executable as non-PIE. After the non-PIE Chromium helper was removed, the last remaining use was for Valgrind. But Valgrind support was also recently removed, so this can go away entirely. BUG=520680 ========== to ========== Delete build/mac/change_mach_o_flags.py and friends. This was only used to mark executable as non-PIE. After the non-PIE Chromium helper was removed, the last remaining use was for Valgrind. But Valgrind support was also recently removed, so this can go away entirely. change_mach_o_flags.py did explicitly set the MH_NO_HEAP_EXECUTION flag on executables, but this is not necessary on x86_64 where the architecture default is to not have an executable heap. BUG=520680 ==========
On 2016/04/13 16:15:09, Nico wrote: > (please update cl description to mention execute bit stuff before landing) Ooops, sorry--forgot to do that yesterday. Thanks for catching & done.
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1885593003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885593003/1
Message was sent while issue was closed.
Description was changed from ========== Delete build/mac/change_mach_o_flags.py and friends. This was only used to mark executable as non-PIE. After the non-PIE Chromium helper was removed, the last remaining use was for Valgrind. But Valgrind support was also recently removed, so this can go away entirely. change_mach_o_flags.py did explicitly set the MH_NO_HEAP_EXECUTION flag on executables, but this is not necessary on x86_64 where the architecture default is to not have an executable heap. BUG=520680 ========== to ========== Delete build/mac/change_mach_o_flags.py and friends. This was only used to mark executable as non-PIE. After the non-PIE Chromium helper was removed, the last remaining use was for Valgrind. But Valgrind support was also recently removed, so this can go away entirely. change_mach_o_flags.py did explicitly set the MH_NO_HEAP_EXECUTION flag on executables, but this is not necessary on x86_64 where the architecture default is to not have an executable heap. BUG=520680 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Delete build/mac/change_mach_o_flags.py and friends. This was only used to mark executable as non-PIE. After the non-PIE Chromium helper was removed, the last remaining use was for Valgrind. But Valgrind support was also recently removed, so this can go away entirely. change_mach_o_flags.py did explicitly set the MH_NO_HEAP_EXECUTION flag on executables, but this is not necessary on x86_64 where the architecture default is to not have an executable heap. BUG=520680 ========== to ========== Delete build/mac/change_mach_o_flags.py and friends. This was only used to mark executable as non-PIE. After the non-PIE Chromium helper was removed, the last remaining use was for Valgrind. But Valgrind support was also recently removed, so this can go away entirely. change_mach_o_flags.py did explicitly set the MH_NO_HEAP_EXECUTION flag on executables, but this is not necessary on x86_64 where the architecture default is to not have an executable heap. BUG=520680 Committed: https://crrev.com/806f385a1f9c2814de91bd1a98a50e74c4fa9a81 Cr-Commit-Position: refs/heads/master@{#387007} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/806f385a1f9c2814de91bd1a98a50e74c4fa9a81 Cr-Commit-Position: refs/heads/master@{#387007} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
