Skip to content

[code_assets] Validate dynamic library architecture from file headers#3484

Open
Yusufihsangorgel wants to merge 12 commits into
dart-lang:mainfrom
Yusufihsangorgel:feat/code-asset-architecture-validation
Open

[code_assets] Validate dynamic library architecture from file headers#3484
Yusufihsangorgel wants to merge 12 commits into
dart-lang:mainfrom
Yusufihsangorgel:feat/code-asset-architecture-validation

Conversation

@Yusufihsangorgel

@Yusufihsangorgel Yusufihsangorgel commented Jul 21, 2026

Copy link
Copy Markdown

Description

A bundled dynamic library built for the wrong target currently reaches the
invoker and fails later with a less useful linker or loader error. This change
validates existing DynamicLoadingBundled files by reading their ELF, Mach-O,
or PE headers and comparing the recognized container family and architecture
with CodeConfig.

The built-in header representation and BinaryFormat remain private. Hook
invokers can supply NativeLibraryValidator implementations through
CodeAssetExtension. The built-in validator and every additional validator
run in registration order:

  • all rejections and validator failures are reported as validation errors;
  • otherwise, a match suppresses inconclusive or unrecognized warnings;
  • otherwise, recognized-but-inconclusive results can be logged once;
  • if no validator recognizes the file and target, validation logs a generic
    warning instead of rejecting the asset.

Warnings are emitted only when the invoker supplies a logger; validation still
rejects mismatches when no logger is present.

The extension point is compatible with the custom operating system and
architecture work in #3413: once those target values can be constructed, the
built-in validator leaves them inconclusive so an additional validator can
decide them. A Mach-O fat header declaring more than one architecture is
rejected, including when an additional validator returns a match, because
hooks produce output for one target architecture per invocation.

Cached hook output now goes through the same semantic validation as fresh
output. If cached output cannot be read or parsed, has a non-object JSON shape,
or fails current validation, hooks_runner removes both the cached output and
its dependency hash. The next invocation reruns the hook instead of reusing
the invalid cache entry.

Tests cover crafted ELF, Mach-O, and PE headers; validator aggregation and
failure behavior; multi-architecture Mach-O rejection; malformed and invalid
cached output; and a hooks_runner integration case that uses
native_toolchain_c to build a real macOS dynamic library for the opposite
architecture.

Deliberately out of scope: static archives, Mach-O cpusubtype/arm64e ABI
validation (tracked in #3379), re-exposing validators through Flutter, and
claiming complete operating-system or ABI compatibility from a container
header.

Related Issues

Fixes #2593.

PR Checklist

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
  • I've run dart tool/ci.dart --all locally with Dart 3.14.0-33.0.dev and resolved all issues identified. Dart 3.11.0 also passes dart tool/ci.dart --all --no-apitool --no-format.
  • All existing and new tests are passing. I added new tests to check the change I am making.
  • The PR is actually solving the issue. PRs that don't solve the issue will be closed. Please be respectful of the maintainers' time. If it's not clear what the issue is, feel free to ask questions on the GitHub issue before submitting a PR.
  • I have updated CHANGELOG.md for the relevant packages. (Not needed for small changes such as doc typos).
  • I have updated the pubspec package version if necessary.

@dcharkes dcharkes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet!

I just realized we need to make the validators pluggable, because we are going to support custom OSes and custom architectures soon. See the comments.

0x01000007 => Architecture.x64,
0x0000000c => Architecture.arm,
0x0100000c => Architecture.arm64,
_ => null,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the arm64e note with a TODO pointing to #3379 in 8a67fb2.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 CPU_TYPE_ARM64 and identifies its ABI in the cpusubtype, so until we read that subtype we treat it as arm64 rather than claiming to validate its ABI.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the arm64e comment with a TODO referencing #3379; it moved into the per-format Mach-O file with the split.

this.macOS,
this.sanitizer,
this.logger,
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make the LibraryHeaderRecognizer pluggable. We're adding support for custom architectures and OSes in:

If we make it pluggable here, we can support multiple third-party recognizers (provided the flutter commandline tool re-exposes the pluggability).

Maybe we can structure the code in a way that we have MachOLibraryHeaderRecognizer, ElfLibraryHeaderRecognizer, PortableExecutableLibraryHeaderRecognizer.

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 Architecture and OS and skip exposing the BinaryFormat I think.

Hm, this design doesn't enable ${expectedFormat.displayName}. in the error message though. If you have a bright idea of how to make it pluggable and have the expected format supported let me know. I think it's hard because we need to both support adding new OSes as well as new architectures. So you don't know up front which LibraryHeaderRecognizer is supposed to give you an answer. Unless you start adding something like bool canReadFor({OS os, Architecture architecture}).

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.
Buuut, then we can't give an error on that no recognizer understood the file. Which we could do with the first design.

If you have a better idea, let me know!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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
libraries and avoid publishing a header model:

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;
}

CodeAssetExtension would take an
Iterable<NativeLibraryValidator> additionalLibraryValidators = const []
and snapshot it. These are trusted extensions supplied by the invoker, not by
the package that produced the hook output. As with the current check, they
would only run for existing DynamicLoadingBundled files.
Flutter would still need a separate re-exposure point before third-party
validators could be supplied through the flutter tool.

The built-ins and all additional validators would run in registration order.
The results would be combined as follows:

  1. Any rejected result wins, with all rejection errors collected.
  2. Otherwise, a matched result passes the check.
  3. Otherwise, inconclusive reasons produce one warning.
  4. If every result is notRecognized, we keep the current generic warning.

matched means that the validator is authoritative for every dimension in
the current validation contract, not just one header field. For this PR that
contract is the expected container family for the target OS and the target
architecture family. It is not a claim of complete OS or ABI compatibility.
A future arm64e subtype or ABI check would be a contract revision and would
need an explicit capability or version gate so that an older validator cannot
silently keep returning matched.

A validator should return notRecognized outside its domain and
inconclusive when it recognizes the file but cannot cover the whole current
contract. It may return matched or rejected only within the scope it fully
understands. The factories would reject an empty reason or error list and
would snapshot returned errors.

Each validator invocation would be wrapped independently. A synchronous or
asynchronous exception becomes a validation error for that validator, and the
remaining validators still run. It is not treated as notRecognized and does
not escape the hook runner's Result path.

A multi-architecture Mach-O is separate from that aggregation. The current
protocol invokes a hook once per target architecture and leaves
universal-binary assembly to the invoker, so a Mach-O containing more than one
architecture should be rejected as a protocol invariant before a custom
matched result can affect the outcome.

Validators would return diagnostics rather than log directly. The extension
can log once after aggregation, when it knows whether an inconclusive result
was resolved by another validator. Dart and Flutter currently construct the
extension without their runner logger, so those call sites would also need to
be wired before warnings are visible there.

This keeps BinaryFormat private. It also avoids treating ELF, Mach-O, or PE
as a complete OS/ABI compatibility claim. Does this contract match what you
had in mind?

I also found that a cache hit returns before _validate. I will route cached
outputs through the same validation as fresh outputs. A cached output read or
parse failure, or a semantic-validation failure, will invalidate both
output.json and the matching dependencies.dependencies_hash_file.json, so
the next invocation reruns the hook instead of reusing a poisoned cache. I
will cover that sequence and the requested real wrong-target hooks_runner
case with regression tests.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented this as NativeLibraryValidator in 8a67fb2. The extension snapshots invoker-supplied validators, runs the built-in and every additional validator in registration order, and accumulates rejections and validator failures. If any validator rejects or throws, those errors are reported and inconclusive warnings are suppressed. Otherwise, a match suppresses inconclusive warnings. The header model and BinaryFormat remain private.

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 native_toolchain_c.

Dart 3.14.0-33.0.dev passes dart tool/ci.dart --all, and Dart 3.11.0 passes dart tool/ci.dart --all --no-apitool --no-format locally.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I like this design!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 X binary, but <os> expects Y message, which needs the OS-to-expected-format map the dispatcher owns. A tryRecognize(file) recognizer can't reconstruct it, and once recognition is fully pluggable the dispatcher can't know which recognizer should have answered, so it can't name the expected format. Your validateLibrary({file, os, architecture}) shape does get the target OS and could emit that message, but as you noted it loses the "no recognizer understood the file" error. Both are real trade-offs I'd rather resolve against a concrete caller: #3413 gives one, and the per-format split makes exposing the extension point then a small non-breaking add. So I lean lock-down now and revisit with #3413, or reserve the public shape now if you'd prefer. WDYT?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah locking it down and not exposing it in the CodeAssetsExtension for now SGTM.

It's nice that we already set the code up in a mostly extensible way 👍

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locked it down. The public export and the CodeAssetExtension constructor parameter are both gone, along with the now-dead runtime injection plumbing. The per-format interface stays, so adding a validator once a concrete caller like #3413 arrives is a small non-breaking change.

}
}

bool _hasElfMagic(Uint8List head) =>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added source links for these constants in 8a67fb2 and updated the System V, fat-header, and machine-header links in 7560bd4.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 e_machine table, Apple's loader.h/fat.h/machine.h, and the Microsoft PE spec).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 MachOValidator with the split.

);
return;
}
if (architectures.contains(codeConfig.targetArchitecture)) return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, does Flutter support getting fat binaries? Flutter might actually crash on receiving a fat binary instead of a thin one. Maybe having a fat binary should be an error. We should check.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that hooks produce output for one target architecture per invocation. 7560bd4 rejects a multi-architecture Mach-O before target-OS and custom-validator decisions, so an additional match cannot override it. 136e3f2 also rejects a structurally valid fat header declaring more than 32 slices without materializing the slice table.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check what Flutter actually does with a fat binary before making it an error, and report back. If it can't consume one, rejecting a multi-slice Mach-O is the right call and I'll keep it; one thin binary per invocation is the rule either way.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked. CodeConfig.targetArchitecture documents that build and link hooks run once per targetArchitecture and that the invoker, not the hook, combines the per-arch CodeAssets into a universal binary. In flutter_tools, lipoDylibs in native_assets_host.dart runs lipo -create to build the universal binary from the thin per-arch outputs. So a hook emitting a fat Mach-O is not the expected shape, and keeping the multi-slice rejection is correct.

);
return;
}
errors.add(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@github-actions

Copy link
Copy Markdown

Package publishing

If you have publishing permissions, you can use the links below to publish the changes after merging this PR.

Package Version Status Publish tag (post-merge)
package:code_assets 1.3.0 ready to publish code_assets-v1.3.0
package:data_assets 0.20.0 already published at pub.dev
package:ffi 2.2.0 already published at pub.dev
package:hooks 2.1.0 already published at pub.dev
package:hooks_runner 1.6.1 already published at pub.dev
package:jni_flutter 1.0.1 already published at pub.dev
package:jni_util 1.0.0-wip WIP (no publish necessary)
package:native_toolchain_c 0.19.3 already published at pub.dev
package:record_use 1.0.0 already published at pub.dev
package:swift2objc 0.2.0 already published at pub.dev
package:swiftgen 0.1.3-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown

PR Health

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
code_assets Non-Breaking 1.2.1 1.3.0-wip 1.3.0-wip ✔️
hooks_runner Non-Breaking 1.6.1 1.6.2-wip 1.6.2-wip ✔️

This check can be disabled by tagging the PR with skip-breaking-check.

}

/// A minimal 64-byte ELF header with [machine] at offset 0x12.
Uint8List elfHeader({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this risks us having the wrong magic numbers both in the package code and in the tests.

We should have tests against real dylibs.

I believe we already cross-compile against every OS/arch combination in native_toolchain_c.
And most of the integration tests actually running hooks and are in the hooks_runner package. So we already cover all the success cases most likely.

Maybe it's worth it to add a single integration test in the hooks_runner package that has a test project that builds against the wrong architecture or wrong OS on purpose with native_toolchain_c.

The integration test can be built like pkgs/hooks_runner/test/build_runner/build_runner_asset_id_test.dart which also has a test project and gives an error on validation.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a macOS integration test in 23f70e0. It uses native_toolchain_c to build a real dylib for the opposite architecture and verifies that hooks_runner rejects the output.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The wrong-architecture test already builds a real library for the opposite architecture and asserts the mismatch rejection against the bytes the real compiler emits, so a wrong constant in the prod tables would fail it. I'll lean on that as the source of truth and cut the hand-built headers down to a couple of minimal negative fixtures for the unrecognized cases. I'll also turn its macOS-only early-return into a visible skip and run it wherever the cross-architecture build is available.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduced the hand-built header fixtures and lean on the real wrong-architecture build test as the source of truth. It now carries a visible skip off macOS instead of registering no tests.

@dcharkes dcharkes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch with the caching, that's something we should think about and document properly.

And I'd organize the 3 binary formats into 3 different validators.

var matched = false;

for (final validator in <NativeLibraryValidator>[
const _NativeLibraryHeaderValidator(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code org feedback:

How about we make a MachOValidator, PortableExecutionFormatValidator, and ElfValidator in 3 different files. These 3 files all import the native_library_validation.dart interface file. And then this file imports those 3 files. (Instead of having a single _NativeLibraryHeaderValidator in this file and all 3 file formats in native_library_headers.dart). It feels cleaner to have a validator per file type. And then if someone wants to add a custom validator it's trivial to look at the existing 3 ones.

The top-level consts with magic constants would then also fit cleanly as private static consts inside the 3 classes.

If there's too much code duplication between the 3 validators we could share some code in a fourth file. We should have some duplicated code for reading some files and some error messages, maybe that's enough to want to do code sharing, maybe not.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on one format per file. One tweak I'd propose: keep readNativeLibraryHeader as the single reader and split only the per-format parsing (ELF, Mach-O, PE) into three files, each with its magic numbers as private static consts. The shared endian readers, BinaryFormat, and the OS-to-expected-format map go in a fourth file the three import. I'd keep one validator on top for the cross-format OS and architecture check, since "wrong format for this OS" isn't something a single parser owns, and three separate NativeLibraryValidators would each re-open the file. Does that match what you had in mind, or would you rather three full validators?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, if each validator only knows about its own binary format, it can't really give an error message containing what format it did find. But maybe that's okay, we can just give an error like we expected a Mach-O file but it wasn't.

and three separate NativeLibraryValidators would each re-open the file

I think we'd skip opening. From the context we know what the target OS is. If the validator is not for that target OS, the validate function will just return early without opening the file?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather have 3 separate validators. Because then when we make it pluggable, it works exactly like that but we'd have 4 or 5.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split into MachOValidator, PortableExecutableValidator, and ElfValidator, each in its own file behind the NativeLibraryValidator interface. The magic numbers are now private static consts on each class. A validator whose format is not the one expected for the target OS returns early without opening the file.

One design note on the unrecognized case: a file whose magic matches no known format is treated as notRecognized, which produces a warning rather than a hard error. That matches the previous behavior and the local_asset example, which bundles an asset.txt CodeAsset with a TODO to move it to a DataAsset. If you would rather have the hard expected a Mach-O file but it wasn't error, that example needs its asset moved to a DataAsset first. Which do you prefer?

);
expect(await outputFile.exists(), isTrue);
expect(await dependenciesHashFile.exists(), isTrue);
await outputFile.writeAsString('{"status":"unknown"}');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this testing? Would we ever have this? Is this an LLM trying to come up with more corner cases?
This seems unrelated to this PR. If it's a useful test, consider putting it up in a separate PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it's unrelated. Removed.


/// Receives warnings from validations which are skipped rather than failed,
/// such as an unrecognized native library header.
final Logger? logger;

Copy link
Copy Markdown
Collaborator

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 validateXxx methods 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?

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Author

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 ProtocolExtension subclass that doesn't also declare it would fail invalid_override, so I won't touch those signatures. The additive way to your ValidationContext idea: add a mutable Logger? logger to ProtocolExtension, have NativeAssetsBuildRunner set e.logger before the override call in its existing for (final e in extensions) loop, and CodeAssetExtension forwards 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.

Copy link
Copy Markdown
Author

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 existing final Logger? logger alongside a new logger on ProtocolExtension does not work: the subclass field shadows the base getter, so an e.logger assignment 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: put Logger? logger; on ProtocolExtension, drop the field from CodeAssetExtension, and set this.logger in its constructor body so existing callers still pass it. If you would rather the runner not mutate a caller-owned field, a void setupValidationLogger(Logger logger) {} default hook does the same job.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a method to ProtocolExtension like setupValidationLogger is then not breaking because we made it a base class.

Then we could also consider something along the lines of:

base class ProtocolExtension {
  // ..
  late Logger _logger;

  Logger get logger => _logger;

  /// Sets up context by the hooks_runner that uses the protocol extensions.
  ///
  /// Guaranteed to be run before any other methods are called on this object.
  void setupLogger(Logger logger) {
    this._logger = logger;
  }
}

We might get passed the same protocol extension object in multiple build/link methods 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_runner might resolve to an older version while package:code_assets might resolve to a newer version, causing the setupLogger never to be called. However, I don't think this will happen in Dart or Flutter because all packages are rolled at the same time.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added setupLogger on ProtocolExtension, a non-final late field with a logger getter, since the same extension object can be reused across a run. The hooks_runner calls it before any other method in both build and link. CodeAssetExtension's own logger field is removed so nothing shadows the base getter.

const Timeout longTimeout = Timeout(Duration(minutes: 5));

void main() async {
test('cached output is revalidated', timeout: longTimeout, () async {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so this can happen if you update flutter or dart and they contain new validators. (Before it could only happen if you add new extensions, but those new extensions would have always resulted in a new input.json, which would have automatically reinvoked hooks.)

Nice catch!

Did we have to change anything the hooks_runner to make this scenario work correctly?

P.S. I'm not entirely sure this is water tight by itself. The Flutter tool also does caching of the complete hooks_runner package before it even invokes hooks_runner by hashing all input and output files. So in order for Flutter to reinvoke the hooks_runner when it added a plugin with a new validator, it would need to invalidate its cache.

So, alternatively, we could also specify that we don't run validators on cache hits. Because running the validators might slow us down. A comment on the protocol extension class https://pub.dev/documentation/hooks/latest/hooks/ProtocolExtension-class.html and on https://pub.dev/documentation/hooks_runner/latest/hooks_runner/NativeAssetsBuildRunner/build.html and link that protocol validations do not re-run and do not invalidate caching.

The hooks for single package caching probably skips the validators. but we might run the global validateApplicationAssets in hooks_runner. Though we shouldn't need to validate the arch/os there, we only need to in validateBuildOutput and validateLinkOutput. So I think it should go unnoticed that we changed the validators?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've talked me out of it. Flutter already caches the whole hooks_runner invocation by hashing its inputs and outputs, so a validator arriving through an SDK update only reaches a fresh run once that outer cache misses; re-validating every cached output isn't worth it. I'll drop the cache-hit revalidation and the output-shape and status check together, which returns build_runner.dart to its current behavior, since that check also runs on the fresh-run path. One caveat worth flagging: it also turned a latent CastError on non-Map output into a graceful FormatException, so reverting restores the CastError; I'd rather land that hardening as its own small PR. In its place I'll document on ProtocolExtension and on NativeAssetsBuildRunner.build and .link that protocol validations run only on fresh hook output and neither re-run on cache hits nor invalidate the cache.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with not running validators on cache hits. Reverted the cache-hit revalidation along with the output shape and status checks. build and link now document that validations run only on freshly produced output, so a new validator in an updated dart or flutter needs no special hooks_runner handling. The non-Map CastError hardening will go in a separate PR.

this.macOS,
this.sanitizer,
this.logger,
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I like this design!

The cached link output with unknown status test exercised generic
cached-output-shape rejection, which is unrelated to architecture
validation. Removed as requested in review.
Replace the single header-model validator that parsed ELF, Mach-O, and PE in
one class with three focused validators, one per container format, each in its
own file: ElfValidator, MachOValidator, and PortableExecutableValidator.

Each validator returns early without opening the file when its format is not
the one expected for the target operating system, and only claims a match when
it fully covers the current contract. This keeps the format-specific parsing
and its edge cases (for example the Mach-O fat-header entry-count cap) isolated
and readable, and removes the shared native_library_headers model that grew
hard to reason about.
Give ProtocolExtension a logger that the hooks runner sets through the new
setupLogger, which it calls on every extension before invoking any other
method during a build or link. testBuildHook upholds the same contract with a
detached logger, matching the previous behavior of not surfacing skipped-
validation warnings from an isolated hook test.

With the logger available on the base class, CodeAssetExtension no longer
carries its own logger field, and its invoker-supplied additionalLibraryValidators
extension point is removed along with the now-internal NativeLibraryValidator
export. Bump the hooks dependency of code_assets and hooks_runner to the
2.2.0-wip that provides setupLogger.
Revert re-validating a cached hook result on a cache hit: output validations
contributed by extensions run once, on freshly produced hook output, and a
cache hit reuses an already accepted result. Drop the on-cache-hit _validate
call and the _invalidateCachedHookResult helper it depended on, and simplify
reading a hook output back to a plain JSON-object decode. Document on build and
link that a validation is never re-run against a cache hit.

Turn the wrong-architecture build_runner test's silent macOS-only return into a
visible skip so the test reports why it did not run instead of registering no
tests, and remove the caching test that only covered the reverted behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[code_assets] Add validator that inspects the reported dylib architectures

2 participants