dart compile exe fails on macOS versions older than macOS 12

[athomas]

The crash was first noticed here:
https://logs.chromium.org/logs/dart-internal/buildbucket/cr-buildbucket/8805124329898895137/+/u/compile_script/stdout

 Info: Compiling with sound null safety
 ===== CRASH =====
 si_signo=Illegal instruction: 4(4), si_code=2, si_addr=0x1031fd454
 version=2.18.0-271.0.dev (dev) (Sat Jul 9 12:06:18 2022 -0700) on "macos_arm64"
 pid=53165, thread=-1, isolate_group=isolate(0x14481b800), isolate=(nil)(0x0)
 isolate_instructions=0, vm_instructions=0
   pc 0x00000001031fd454 fp 0x000000016cee49f0 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0xc3cc8
   pc 0x00000001031fa298 fp 0x000000016cee4a70 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0xc0b0c
   pc 0x00000001031f6fa8 fp 0x000000016cee4b90 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0xbd81c
   pc 0x00000001031f5f68 fp 0x000000016cee4c30 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0xbc7dc
   pc 0x00000001031ffb98 fp 0x000000016cee4c80 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0xc640c
   pc 0x0000000103236c8c fp 0x000000016cee51c0 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0xfd500
   pc 0x0000000103235d58 fp 0x000000016cee5280 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0xfc5cc
   pc 0x000000010323637c fp 0x000000016cee52b0 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0xfcbf0
   pc 0x000000010316eb50 fp 0x000000016cee59b0 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0x353c4
   pc 0x000000010316f42c fp 0x000000016cee6080 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0x35ca0
   pc 0x000000010316ac78 fp 0x000000016cee6150 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0x314ec
   pc 0x0000000103065380 fp 0x000000016cee61b0 Cr_z_armv8_crc32_little+0x10c98c
   pc 0x00000001030650b4 fp 0x000000016cee62f0 Cr_z_armv8_crc32_little+0x10c6c0
   pc 0x000000010315ff68 fp 0x000000016cee6380 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0x267dc
   pc 0x000000010315e01c fp 0x000000016cee6ae0 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0x24890
   pc 0x000000010315de10 fp 0x000000016cee6f80 std::__2::__call_once(unsigned long volatile&, void*, void (*)(void*))+0x24684
   pc 0x00000001032cbc0c fp 0x000000016cee70c0 Dart_Precompile+0x138
   pc 0x0000000102f401a0 fp 0x000000016cee7270 std::__2::align(unsigned long, unsigned long, void*&, unsigned long&)+0xa88
   pc 0x0000000199c25430 fp 0x000000016cee7290 start+0x4
 -- End of DumpStackTrace
 === Crash occured when compiling dart:core_StateError_StateError. in AOT mode in AllocateRegisters pass
 *** BEGIN CFG
 AllocateRegisters
 ==== dart:core_StateError_StateError. (Constructor)
   0: B0[graph]:0 {
       v0 <- Constant(#null) T{Null?}
 }
   2: B1[function entry]:2 {
       v2 <- Parameter(0) T{StateError}
       v3 <- Parameter(1) T{String}
 }
   4:     StoreInstanceField(v2 . message = v3)
   6:     Return:12(v0)
 *** END CFG
 Error: AOT compilation failed
 Generating AOT snapshot failed!

The program being compiled was bin/find_base_commit.dart from https://dart.googlesource.com/dart_ci/+/refs/heads/main/builder/.

The crash can also be reproduced with a "Hello World" program.

[athomas]

On the latest beta, this crash doesn't seem to happen, instead we get the same failure as on x64 (see also #49275).
https://logs.chromium.org/logs/dart-internal/buildbucket/cr-buildbucket/8805115124636981169/+/u/compile_script/stdout

[a-siva]

@athomas can you confirm on which beta range you saw the crash (I presume beta 2.18.0-271.7,beta does not exhibit the crash but 2.18.0-271.4.beta exhibits the crash)

[a-siva]

Also does clang upgrades done in the main branch get carried over to the beta branch too ?

[athomas]

Crash confirmed on 2.18.0-271.0.dev (the branch point).
Crash seems fixed on 2.18.0-271.7.beta (assuming it's not a flaky crash).

Clang updates are changes to DEPS, and don't carry over unless CP'ed or landed before the branch point.

[mkustermann]

To summarize, there's two issues here

• There's a crash in our AOT compiler (invocation of gen_snapshot while compiling a function). This crash has disappeared, it may have been fixed. So it's no longer an issue?

• There's a failure to dart compile exe bin/find_base_commit.dart: This issue isn't reproducible locally (according to @sstrickl), but may still exist on the bots, the failure is

Failed to add a linker signed signature, adding a regular signature instead.
Failed to replace the dartaotruntime signature, subcommand terminated with exit code 1.
Subcommand stderr:
/opt/s/w/ir/x/w/recipe_cleanup/tmpmb2rhhen/find_base_commit: replacing existing signature
/usr/libexec/DeveloperTools/codesign_allocate: object: /opt/s/w/ir/x/w/recipe_cleanup/tmpmb2rhhen/find_base_commit malformed object (offset field of section 0 in LC_SEGMENT command 1 not past the headers of the file)
/opt/s/w/ir/x/w/recipe_cleanup/tmpmb2rhhen/find_base_commit: the codesign_allocate helper tool cannot be found or used

This has been reported in the past (see e.g. #49275, #49326)

@athomas says this issue seems to be a regression in 2.18.

If this happens on our CI, we should be able to bisect the issue on one of the VMs?

[mkustermann]

/usr/libexec/DeveloperTools/codesign_allocate: object:
... malformed object (offset field of section 0 in LC_SEGMENT command 1 not past the headers of the file

The corresponding code for this error message can be found in llvm/lib/Object/MachOObjectFile.cpp which may be helpful for debuging the issue. /cc @sstrickl

[athomas]

Assuming the crash wasn't flaky, it seems to have disappeared. It only occured on arm64 so we have relatively few datapoints on CI, however it was reproducible locally as well.

The signing issue seems to be tied to macOS version:
https://github.com/athomas/github-actions-playground/runs/7969362964?check_suite_focus=true

[sstrickl]

So looking at the dartaotruntime executable we use to create the merged executable, the toolchain generates it with the first _TEXT load command being at file offset 0, which means that its range includes the headers (hence the error). We just copy that over when creating the merged executable without changing it. The first section within that load command starts at a file offset after the headers. My guess is that at one point that check was done, and then later, if kept in any form, they changed it to check the section offsets instead (and thus the error goes away on later MacOS versions because no section is defined to be located within the headers).

We could try to patch this up as we generate the merged binary, changing the file offset of any load sections with non-zero size that start at an offset in the headers to the file offset of the first section within it, adjusting the vm address, size, and file size of the load command accordingly.

However, if the vm address of a load segment needs to, say, be on a page boundary for mmap reasons, that might be a problem, given that in the example build on my machine, the header ends at 3216 and the first section in that load command starts at 3280 (and there's no page boundary between those two values).

That is, it's a possibly very brittle change that isn't even needed for later MacOS versions that have an updated codesign. So I suppose we need to decide if those MacOS versions need supporting, and if they do, then we could just fall back to the old appended executable way of generating things, which means that those executables couldn't be signed on those MacOS versions. Thoughts?

[a-siva]

Could you state which MacOS versions would be considered later which have the updated codesign ?
Our website claims we support 10.15, 11 and 12 versions.

Flutter claims to support (12 - google tested) and best effort (10.11 and 11.

[athomas]

dart compile exe only works on macOS 12 in the current release candidate, 10.15, 11 don't work. See the GitHub Actions link I posted earlier for details.

[mkustermann]

So I suppose we need to decide if those MacOS versions need supporting, and if they do, then we could just fall back to the old appended executable way of generating things, which means that those executables couldn't be signed on those MacOS versions. Thoughts?

We support several older MachOS versions, not just the oldest. I don't think we can just drop older version support now.

The ability to sign dart compile exeed binaries was afaik introduced in cl/228080 which was included in 2.17.0 where it worked afaik. So I don't think we need to fall back to append snapshot approach, since it used to work. The regression seems to have been introduced since then.

@sstrickl Is there something I'm overlooking?

So looking at the dartaotruntime executable we use to create the merged executable, the toolchain generates it with the first _TEXT load command being at file offset 0, which means that its range includes the headers (hence the error).

Phrased in another way: Are you saying that the dartaotruntime binary itself - as produced with our toolchain / xcodebuild - already has this problem (i.e. cannot be signed) irrespective of using dart compile exe?

If so, why wasn't this an issue before?

[sstrickl]

Phrased in another way: Are you saying that the dartaotruntime binary itself - as produced with our toolchain / xcodebuild - already has this problem (i.e. cannot be signed) irrespective of using dart compile exe?

If so, why wasn't this an issue before?

Cannot be signed on those MacOS versions. I'd guess that the automated process for signing the SDK uses a version of the codesign utility which doesn't have this issue.

[athomas]

I don't see a change in our signing infra that fits in that timeframe:

On 2022-02-17 we upgrade the signing flow to big sur (but that also affects the 2.17.6 release), then there were changes recently to upgrade it to monterey but that happened after the branchpoint.

[a-siva]

Since a revert has landed I am going to change the priority of this issue to p2.

[sstrickl]

Okay, so here's the lowdown of what's behind this and the current plan for the future:

We currently create signable MachO standalone executables by adding a new segment load command with a section pointing to the snapshot data in a copy of the dartaotruntime MachO executable. Notably, currently we add the load command/section to the header in the padding between the header and the first actual post-header data, which avoids having to go through all the header and section data and change offsets for all the contents of the original executable that we're just copying over.

However, this approach assumes there's enough space in the padding between the header and the contents of the MachO executable to put this information. Prior to the clang change above, there was (e.g., the headers ended at offset 3064, and the first content started at offset 17280). Afterwards, there wasn't (e.g., the headers ended at offset 3128, and the first content offset was at 3200). Unfortunately, the current code for adding the new header information doesn't check that the available padding can fit the new information, which is why we didn't get an error when this started happening.

Instead, we saw codesign failures because it did verification checks on the resulting MachO file. Unfortunately, these verification checks seem to only happen when the -f argument to force signature overwriting happens, and we only use that flag as a fallback in MacOS <12 when we can't use the linker-signed option to codesign. Thus, we only saw this error on older MacOS versions, like those used by some of the trybots, which muddled the importance of this failure at the time.

Now that we know what the issue is, we've rolled back clang (which as a side effect restores the extra padding) as a temporary fix to unblock the stable release. The longer term fix is to change our approach to combining the snapshot into the MachO executable.

There are three possibilities I currently have in mind, though really only the first and third should be considered:

1. Adjust the MachO parsing and editing code in pkg:dart2native to adjust all offsets in the copied version of dartaotruntime appropriately for the addition of the new section, if the padding in the file isn't enough. This means much more parsing of the MachO format than we currently do in pkg:dart2native, and will likely be very brittle when new information is added by future compiler versions/MachO format updates that we don't handle.

2. Instead of trying to cram the snapshot into a copy of the dartaotruntime MachO executable, create a universal binary where the dartaotruntime executable is used for the appropriate architecture, and add a separate section that just contains the snapshot data directly. Thus, we no longer need MachO parsing/writing in pkg:dart2native or the Dart runtime, but instead MacOS universal binary parsing/writing.

   However, I believe this approach means that only the dartaotruntime will be signed by any uses of codesign, and thus the snapshot data could be replaced wholesale without affecting the signature. Thus, I don't suggest this method.

3. Combine the two approaches: create a universal binary as suggested, but instead of adding the raw snapshot data as a second section, add a MachO wrapper that can be signed. This means we don't need full MachO parsing or writing in pkg:dart2native or the Dart runtime, just enough to wrap the snapshot and to parse what we output. (We'll still need universal binary parsing/writing, of course, but that's a simple format.) This means verifying the signatures on the universal binary will reveal if someone attempts to replace only the snapshot data.

Thus, my plans are to implement the third approach, after which we can reland the clang changes without concern.

[athomas]

We've CP'ed the revert of the clang roll to beta.

The crash that started this issue was investigated by @mraleph and turned out to be an infra issue (the crash occurs if you run dart compile exe with an x64 SDK on rosetta).

We still need a long term solution such as those outlined by @sstrickl

[sstrickl]

I'm also making a quick CL to add a check that we're not overwriting the section contents with the expanded header, so that we'll get proper failures if this case ends up happening again while the long-term solution is being worked on.

[mkustermann]

/cc @mraleph see suggested fix in #49783 (comment).

[mkustermann]

@sstrickl Thanks for writing up the issue & solution space!

Adjust the MachO parsing and editing code in pkg:dart2native to adjust all offsets in the copied version of dartaotruntime appropriately for the addition of the new section, if the padding in the file isn't enough. This means much more parsing of the MachO format than we currently do in pkg:dart2native, and will likely be very brittle when new information is added by future compiler versions/MachO format updates that we don't handle.

Do we have good understanding of which parts use file offsets that would need to be modified? (Many things may use offsets into sections and not absolute file offset). It would be good to understand before making the implementation decision.

Instead of trying to cram the snapshot into a copy of the dartaotruntime MachO executable, create a universal binary where the dartaotruntime executable is used for the appropriate architecture, and add a separate section that just contains the snapshot data directly.

By piggy-bagging on universal binaries, we rely on tools (strip, signing, ...) to preserve all parts. It may be unusual to have universal binaries with several parts with same architecture. I can imagine there's tools that take universal binaries (containing code for different architectures) and pulling out / stripping to only whats needed - to get smaller binaries. That would be the risk with this approach.

[sstrickl]

That's true, that would be a risk. That, and I think we'd have to use a different (non-supported) architecture for the Dart snapshot MachO, though if we mark it as a dsym or something, that might be enough for it not to be chosen when executing, but that doesn't help against stripping universal binaries back to MachO executables.

The main thing I'm worried about are file offsets inside sections. Handling load segments and section headers is relatively easy, though whether a load command contains file offsets depends on the specific load segment type and so we'd have to make sure we handle what we know about and hope none get added.

However, but if any section contents themselves include file offsets, then it gets even trickier. I'll page back in the MachO specifics I know and see if I have any obvious examples of such to point to.

[mkustermann]

Another question: If a universal binary is executed, I assume only the specific architecture's MachO is loaded into memory. So if we add another part, would it get loaded by the dynamic loader or we'd need to manually mmap() it into memory ourselves / find file handle / filename and read from the file?

[mraleph]

Maybe we could return back to the idea of shipping clang's assembler+linker+linkable-AOT-runtime-library in the SDK instead? This has larger footprint but is guaranteed to work.

[sstrickl]

Yeah, that'd probably be the best in the long run, just to treat the AOT runtime as something linked together with the snapshot to create an executabler rather than trying to do the merging mess we currently do. I should have listed that as a possible solution as well, thanks for mentioning it!

[sstrickl]

So if we add another part, would it get loaded by the dynamic loader or we'd need to manually mmap() it into memory ourselves / find file handle / filename and read from the file?

I might be misremembering but I'm pretty sure we already do this approach, opening the file, finding the snapshot offset, and then using the ELF loader which will mmap() the right part of it.