-
Notifications
You must be signed in to change notification settings - Fork 129
[code_assets] Validate dynamic library architecture from file headers #3484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
695d62f
8a67fb2
23f70e0
24218e4
7560bd4
933b5ef
32f5f07
136e3f2
ccf2f0a
97a40d6
550ff1b
18dad6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| import 'package:hooks/hooks.dart'; | ||
| import 'package:logging/logging.dart'; | ||
|
|
||
| import 'architecture.dart'; | ||
| import 'c_compiler_config.dart'; | ||
|
|
@@ -55,6 +56,10 @@ final class CodeAssetExtension extends ProtocolExtension { | |
| /// runtime libraries and memory allocators across the FFI boundary. | ||
| final Sanitizer? sanitizer; | ||
|
|
||
| /// Receives warnings from validations which are skipped rather than failed, | ||
| /// such as an unrecognized native library header. | ||
| final Logger? logger; | ||
|
|
||
| /// Constructs a [CodeAssetExtension]. | ||
| CodeAssetExtension({ | ||
| required this.targetArchitecture, | ||
|
|
@@ -65,6 +70,7 @@ final class CodeAssetExtension extends ProtocolExtension { | |
| this.iOS, | ||
| this.macOS, | ||
| this.sanitizer, | ||
| this.logger, | ||
| }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to make the If we make it pluggable here, we can support multiple third-party recognizers (provided the Maybe we can structure the code in a way that we have And then interface LibraryHeaderRecognizer {
Future<LibraryHeader?> tryRecognize(Uri file);
}
final class LibraryHeader {
final List<Architecture> architectures;
final OS? os;
// (for arm64 we might need to add an abi version field later as well here)
// we can add a nullable field here later without it being a breaking change.
LibraryHeader({required this.architectures, this.os});
}As this API is part of the public API in order to make it pluggable/extensible, we should only use a Hm, this design doesn't enable Maybe the API should be interface LibraryHeaderRecognizer {
Future<List<String>> validateLibrary({Uri file, OS os, Architecture architecture});
}Or it should simply be a list of functions CodeAssetExtension({
libraryValidators List<Future<List<String>> Function({Uri file, OS os, Architecture architecture, Logger logger})>
});And then if you recognize the magic bytes, and they are wrong, you can give an error. If no recognizer gives errors, then that's good. If you have a better idea, let me know!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A validator API seems like the cleaner fit. I would keep it specific to native final class NativeLibraryValidationContext {
final Uri file;
final CodeConfig config;
const NativeLibraryValidationContext({
required this.file,
required this.config,
});
}
abstract interface class NativeLibraryValidator {
Future<NativeLibraryValidation> validate(
NativeLibraryValidationContext context,
);
}
sealed class NativeLibraryValidation {
const NativeLibraryValidation._();
const factory NativeLibraryValidation.notRecognized() = _NotRecognized;
factory NativeLibraryValidation.inconclusive(String reason) = _Inconclusive;
const factory NativeLibraryValidation.matched() = _Matched;
factory NativeLibraryValidation.rejected(Iterable<String> errors) = _Rejected;
}
The built-ins and all additional validators would run in registration order.
A validator should return Each validator invocation would be wrapped independently. A synchronous or A multi-architecture Mach-O is separate from that aggregation. The current Validators would return diagnostics rather than log directly. The extension This keeps I also found that a cache hit returns before
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented this as 7560bd4 makes multi-architecture Mach-O rejection independent of target OS and validator aggregation, so an additional match cannot override it. 136e3f2 also rejects a structurally valid fat header declaring more than 32 slices without reading or allocating the slice table. For a custom target architecture, the built-in validator remains inconclusive, preserving the future extension path for #3413. 23f70e0 routes cache hits through the same validation as fresh output. 933b5ef invalidates malformed or non-object cached JSON, and 136e3f2 does the same for an unknown cached status, for both build and link hooks. Read, parse, and semantic-validation failures remove both cache files, so the next invocation reruns the hook. The real wrong-architecture integration test builds an opposite-architecture dylib through Dart 3.14.0-33.0.dev passes
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I like this design!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd keep the recognizers internal for this PR. The built-in path's value is the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah locking it down and not exposing it in the It's nice that we already set the code up in a mostly extensible way 👍
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Locked it down. The public export and the |
||
|
|
||
| @override | ||
|
|
@@ -103,13 +109,13 @@ final class CodeAssetExtension extends ProtocolExtension { | |
| Future<ValidationErrors> validateBuildOutput( | ||
| BuildInput input, | ||
| BuildOutput output, | ||
| ) => validateCodeAssetBuildOutput(input, output); | ||
| ) => validateCodeAssetBuildOutput(input, output, logger: logger); | ||
|
|
||
| @override | ||
| Future<ValidationErrors> validateLinkOutput( | ||
| LinkInput input, | ||
| LinkOutput output, | ||
| ) => validateCodeAssetLinkOutput(input, output); | ||
| ) => validateCodeAssetLinkOutput(input, output, logger: logger); | ||
|
|
||
| @override | ||
| Future<ValidationErrors> validateApplicationAssets( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| // Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file | ||
| // for details. All rights reserved. Use of this source code is governed by a | ||
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| import 'dart:io'; | ||
| import 'dart:typed_data'; | ||
|
|
||
| import 'architecture.dart'; | ||
|
|
||
| /// The binary format of a native library file. | ||
| enum BinaryFormat { | ||
| /// The Executable and Linkable Format used on Linux, Android, and Fuchsia. | ||
| elf('ELF'), | ||
|
|
||
| /// The Mach object format used on macOS and iOS. | ||
| machO('Mach-O'), | ||
|
|
||
| /// The Portable Executable format used on Windows. | ||
| pe('PE'); | ||
|
|
||
| const BinaryFormat(this.displayName); | ||
|
|
||
| /// The name of this format in messages. | ||
| final String displayName; | ||
| } | ||
|
|
||
| /// The result of reading a native library's header. | ||
| sealed class NativeLibraryHeader { | ||
| const NativeLibraryHeader(); | ||
| } | ||
|
|
||
| /// A header whose binary format was recognized. | ||
| final class RecognizedHeader extends NativeLibraryHeader { | ||
| /// Constructs a [RecognizedHeader]. | ||
| const RecognizedHeader(this.format, this.architectures, this.machineValues); | ||
|
|
||
| /// The binary format of the file. | ||
| final BinaryFormat format; | ||
|
|
||
| /// The architectures the file was built for, aligned with [machineValues]. | ||
| /// | ||
| /// A `null` entry means the format was recognized but its machine value is | ||
| /// not one of the [Architecture]s. Fat Mach-O files contain one entry per | ||
| /// slice; the other formats always contain exactly one. | ||
| final List<Architecture?> architectures; | ||
|
|
||
| /// The raw machine values from the header, for diagnostics. | ||
| final List<int> machineValues; | ||
| } | ||
|
|
||
| /// A file whose binary format was not recognized. | ||
| final class UnrecognizedHeader extends NativeLibraryHeader { | ||
| /// Constructs an [UnrecognizedHeader]. | ||
| const UnrecognizedHeader(); | ||
| } | ||
|
|
||
| /// Reads just enough of [file] to identify its binary format and the | ||
| /// architectures it was built for. | ||
| /// | ||
| /// Never throws for file contents: anything that cannot be identified, | ||
| /// including short and empty files, is an [UnrecognizedHeader]. | ||
| NativeLibraryHeader readNativeLibraryHeader(File file) { | ||
| final RandomAccessFile raf; | ||
| try { | ||
| raf = file.openSync(); | ||
| } on FileSystemException { | ||
| return const UnrecognizedHeader(); | ||
| } | ||
| try { | ||
| final length = raf.lengthSync(); | ||
| final head = raf.readSync(64); | ||
| if (head.length >= 20 && _hasElfMagic(head)) return _readElf(head); | ||
| if (head.length >= 8) { | ||
| final bigEndianMagic = _u32(head, 0, littleEndian: false); | ||
| if (bigEndianMagic == 0xcafebabe || bigEndianMagic == 0xcafebabf) { | ||
| return _readFatMachO(raf, head, is64: bigEndianMagic == 0xcafebabf); | ||
| } | ||
| final thin = _readThinMachO(head); | ||
| if (thin != null) return thin; | ||
| } | ||
| if (head.length >= 0x40 && head[0] == 0x4d && head[1] == 0x5a) { | ||
| return _readPe(raf, head, length); | ||
| } | ||
| return const UnrecognizedHeader(); | ||
| } on FileSystemException { | ||
| return const UnrecognizedHeader(); | ||
| } finally { | ||
| raf.closeSync(); | ||
| } | ||
| } | ||
|
|
||
| bool _hasElfMagic(Uint8List head) => | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for all of the magic numbers everywhere in this code, if we have sources lets add links to the sources in doc comments.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. Each format's magic numbers become private static consts in its own file with a doc-comment link to the format spec (the ELF
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the source links for the magic numbers in doc comments; they moved into |
||
| head[0] == 0x7f && head[1] == 0x45 && head[2] == 0x4c && head[3] == 0x46; | ||
|
|
||
| NativeLibraryHeader _readElf(Uint8List head) { | ||
| final elfClass = head[4]; | ||
| final elfData = head[5]; | ||
| if ((elfClass != 1 && elfClass != 2) || (elfData != 1 && elfData != 2)) { | ||
| return const UnrecognizedHeader(); | ||
| } | ||
| final machine = _u16(head, 18, littleEndian: elfData == 1); | ||
| final architecture = switch (machine) { | ||
| 3 => Architecture.ia32, | ||
| 40 => Architecture.arm, | ||
| 62 => Architecture.x64, | ||
| 183 => Architecture.arm64, | ||
| 243 => elfClass == 1 ? Architecture.riscv32 : Architecture.riscv64, | ||
| _ => null, | ||
| }; | ||
| return RecognizedHeader(BinaryFormat.elf, [architecture], [machine]); | ||
| } | ||
|
|
||
| Architecture? _machOArchitecture(int cpuType) => switch (cpuType) { | ||
| 0x00000007 => Architecture.ia32, | ||
| 0x01000007 => Architecture.x64, | ||
| 0x0000000c => Architecture.arm, | ||
| 0x0100000c => Architecture.arm64, | ||
| _ => null, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please leave a comment what you'd expect for arm64e with a TODO referring to:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. I'll add a TODO in the Mach-O parser referencing #3379: arm64e uses the same
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the arm64e comment with a |
||
| }; | ||
|
|
||
| NativeLibraryHeader? _readThinMachO(Uint8List head) { | ||
| final littleEndianMagic = _u32(head, 0, littleEndian: true); | ||
| final bool littleEndian; | ||
| if (littleEndianMagic == 0xfeedface || littleEndianMagic == 0xfeedfacf) { | ||
| littleEndian = true; | ||
| } else if (littleEndianMagic == 0xcefaedfe || | ||
| littleEndianMagic == 0xcffaedfe) { | ||
| littleEndian = false; | ||
| } else { | ||
| return null; | ||
| } | ||
| final cpuType = _u32(head, 4, littleEndian: littleEndian); | ||
| return RecognizedHeader( | ||
| BinaryFormat.machO, | ||
| [_machOArchitecture(cpuType)], | ||
| [cpuType], | ||
| ); | ||
| } | ||
|
|
||
| NativeLibraryHeader _readFatMachO( | ||
| RandomAccessFile raf, | ||
| Uint8List head, { | ||
| required bool is64, | ||
| }) { | ||
| // The fat header and arch entries are always big-endian. The number of | ||
| // slices is tiny in practice; the bound also rejects Java class files, | ||
| // which share the 0xcafebabe magic but carry a version number here. | ||
| final sliceCount = _u32(head, 4, littleEndian: false); | ||
| if (sliceCount == 0 || sliceCount > 32) return const UnrecognizedHeader(); | ||
| final entrySize = is64 ? 32 : 20; | ||
| raf.setPositionSync(8); | ||
| final entries = raf.readSync(sliceCount * entrySize); | ||
| if (entries.length < sliceCount * entrySize) { | ||
| return const UnrecognizedHeader(); | ||
| } | ||
| final machineValues = [ | ||
| for (var slice = 0; slice < sliceCount; slice++) | ||
| _u32(entries, slice * entrySize, littleEndian: false), | ||
| ]; | ||
| return RecognizedHeader(BinaryFormat.machO, [ | ||
| for (final cpuType in machineValues) _machOArchitecture(cpuType), | ||
| ], machineValues); | ||
| } | ||
|
|
||
| NativeLibraryHeader _readPe(RandomAccessFile raf, Uint8List head, int length) { | ||
| final peOffset = _u32(head, 0x3c, littleEndian: true); | ||
| if (peOffset + 6 > length) return const UnrecognizedHeader(); | ||
| raf.setPositionSync(peOffset); | ||
| final signature = raf.readSync(6); | ||
| if (signature.length < 6 || | ||
| signature[0] != 0x50 || | ||
| signature[1] != 0x45 || | ||
| signature[2] != 0 || | ||
| signature[3] != 0) { | ||
| return const UnrecognizedHeader(); | ||
| } | ||
| final machine = _u16(signature, 4, littleEndian: true); | ||
| final architecture = switch (machine) { | ||
| 0x014c => Architecture.ia32, | ||
| 0x01c4 => Architecture.arm, | ||
| 0x5064 => Architecture.riscv64, | ||
| 0x8664 => Architecture.x64, | ||
| 0xaa64 => Architecture.arm64, | ||
| _ => null, | ||
| }; | ||
| return RecognizedHeader(BinaryFormat.pe, [architecture], [machine]); | ||
| } | ||
|
|
||
| int _u16(Uint8List bytes, int offset, {required bool littleEndian}) => | ||
| littleEndian | ||
| ? bytes[offset] | (bytes[offset + 1] << 8) | ||
| : (bytes[offset] << 8) | bytes[offset + 1]; | ||
|
|
||
| int _u32(Uint8List bytes, int offset, {required bool littleEndian}) => | ||
| littleEndian | ||
| ? bytes[offset] | | ||
| (bytes[offset + 1] << 8) | | ||
| (bytes[offset + 2] << 16) | | ||
| (bytes[offset + 3] << 24) | ||
| : (bytes[offset] << 24) | | ||
| (bytes[offset + 1] << 16) | | ||
| (bytes[offset + 2] << 8) | | ||
| bytes[offset + 3]; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have the logger from hooks_runner be used instead of a logger here in the constructor, but adding a logger argument to the
validateXxxmethods would be a breaking change.See https://github.com/flutter/flutter/blob/e69e703dce29fda392f8024109c1961306cb365a/packages/flutter_tools/lib/src/isolated/native_assets/targets.dart#L237. We don't want to have to construct the same logger there as we did in https://github.com/flutter/flutter/blob/e69e703dce29fda392f8024109c1961306cb365a/packages/flutter_tools/lib/src/isolated/native_assets/native_assets.dart#L561
Maybe we can add the logger to the
ValidationContext?WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm wait the code assets extension creates the validation context, so it also doesn't have access to the logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it's breaking even as an optional named param, since an external
ProtocolExtensionsubclass that doesn't also declare it would failinvalid_override, so I won't touch those signatures. The additive way to yourValidationContextidea: add a mutableLogger? loggertoProtocolExtension, haveNativeAssetsBuildRunnersete.loggerbefore the override call in its existingfor (final e in extensions)loop, andCodeAssetExtensionforwards it into the context it builds. That covers your follow-up: the extension still creates the context, but now has the runner's logger to put in it, so Flutter never builds a second one. I'll leave the existing constructor logger as a fallback so the change stays additive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction on the mechanism I proposed. Keeping
CodeAssetExtension's existingfinal Logger? loggeralongside a newloggeronProtocolExtensiondoes not work: the subclass field shadows the base getter, so ane.loggerassignment from the runner lands on the base field while every read dispatches to the subclass getter, a silent no-op (confirmed with a quick repro). The workable shape is a single storage: putLogger? logger;onProtocolExtension, drop the field fromCodeAssetExtension, and setthis.loggerin its constructor body so existing callers still pass it. If you would rather the runner not mutate a caller-owned field, avoid setupValidationLogger(Logger logger) {}default hook does the same job.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a method to
ProtocolExtensionlikesetupValidationLoggeris then not breaking because we made it a base class.Then we could also consider something along the lines of:
We might get passed the same protocol extension object in multiple
build/linkmethods in one run. So we do end up setting the logger more than once potentially. So the field can't be final.I think technically this is still a breaking change, because
package:hooks_runnermight resolve to an older version whilepackage:code_assetsmight resolve to a newer version, causing thesetupLoggernever to be called. However, I don't think this will happen in Dart or Flutter because all packages are rolled at the same time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
setupLoggeronProtocolExtension, a non-finallatefield with aloggergetter, since the same extension object can be reused across a run. The hooks_runner calls it before any other method in bothbuildandlink.CodeAssetExtension's ownloggerfield is removed so nothing shadows the base getter.