search

monogon-dev / monogon

The Monogon Monorepo. May contain traces of peanuts and a ✨pure Go Linux userland✨. Work in progress!
https://monogon.tech
Apache License 2.0
378 stars 8 forks source link

EFI binaries get built with materially different configurations #282

Closed lorenz closed 7 months ago

lorenz commented 7 months ago

In https://review.monogon.dev/c/monogon/+/2413 we ran into an issue where //metropolis/node/core/abloader gets built with two different configurations if included in the OS executable (via //metropolis/node/core/update) versus what's being included in mkimage. Both work, but because the hashese are different this causes spurious repair actions by the update code. diffoscope shows non-trivial differences including machine code.

q3k commented 7 months ago

Seems like the intended transition path is taken, but just that the initial configuration for both reverse dependencies is different, thus the transitioned configuration is also different:

$ bazel cquery 'somepath(//metropolis/node/core/update, //metropolis/node/core/abloader:abloader_bin)' --universe_scope=//metropolis/node/core/update 
//metropolis/node/core/update:update (401e648)
//metropolis/node/core/abloader:abloader (401e648)
//metropolis/node/core/abloader:abloader_bin (9e3ced5)
$ bazel cquery 'somepath(//metropolis/node/core, //metropolis/node/core/abloader:abloader_bin)' --universe_scope=//metropolis/node/core
//metropolis/node/core:core (05d50f5)
//metropolis/node/core:core_lib (4fa4f3b)
//metropolis/node/core/update:update (4fa4f3b)
//metropolis/node/core/abloader:abloader (adb7102)
//metropolis/node/core/abloader:abloader_bin (471d70e)

The difference between the two configs is pretty insignificant, so I would rather classify this issue as the build being not hermetic enough (and thus resulting in different artifacts for only slightly different configs) rather than an issue with the configurations being different - although that also isn't optimal.

$ diff -u <(bazel config 9e3ced5) <(bazel config 471d70e)
-BuildConfiguration 9e3ced5c778355e02bed68759a44e0261f79d0c2b8168b8d4d645a549dd96b2a:
-Skyframe Key: BuildConfigurationValue.Key[9e3ced5c778355e02bed68759a44e0261f79d0c2b8168b8d4d645a549dd96b2a]
+BuildConfiguration 471d70e341e24a6a3b088f52046e8abc17a7cb057ad7b44ae81ac18e0830cba1:
+Skyframe Key: BuildConfigurationValue.Key[471d70e341e24a6a3b088f52046e8abc17a7cb057ad7b44ae81ac18e0830cba1]
 Fragments: com.google.devtools.build.lib.analysis.PlatformConfiguration: [com.google.devtools.build.lib.analysis.PlatformOptions], com.google.devtools.build.lib.analysis.ShellConfiguration: [com.google.devtools.build.lib.analysis.ShellConfiguration$Options], com.google.devtools.build.lib.analysis.test.CoverageConfiguration: [com.google.devtools.build.lib.analysis.config.CoreOptions,com.google.devtools.build.lib.analysis.test.CoverageConfiguration$CoverageOptions], com.google.devtools.build.lib.analysis.test.TestConfiguration: [com.google.devtools.build.lib.analysis.test.TestConfiguration$TestOptions], com.google.devtools.build.lib.bazel.rules.BazelRuleClassProvider$StrictActionEnvConfiguration: [com.google.devtools.build.lib.bazel.rules.BazelRuleClassProvider$StrictActionEnvOptions], com.google.devtools.build.lib.bazel.rules.python.BazelPythonConfiguration: [com.google.devtools.build.lib.bazel.rules.python.BazelPythonConfiguration$Options,com.google.devtools.build.lib.rules.python.PythonOptions], com.google.devtools.build.lib.rules.android.AndroidConfiguration: [com.google.devtools.build.lib.rules.android.AndroidConfiguration$Options], com.google.devtools.build.lib.rules.android.AndroidLocalTestConfiguration: [com.google.devtools.build.lib.rules.android.AndroidLocalTestConfiguration$Options], com.google.devtools.build.lib.rules.android.BazelAndroidConfiguration: [com.google.devtools.build.lib.rules.android.BazelAndroidConfiguration$Options], com.google.devtools.build.lib.rules.apple.AppleConfiguration: [com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions], com.google.devtools.build.lib.rules.apple.swift.SwiftConfiguration: [com.google.devtools.build.lib.rules.apple.swift.SwiftCommandLineOptions], com.google.devtools.build.lib.rules.config.ConfigFeatureFlagConfiguration: [com.google.devtools.build.lib.rules.config.ConfigFeatureFlagOptions], com.google.devtools.build.lib.rules.cpp.CppConfiguration: [com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions,com.google.devtools.build.lib.rules.cpp.CppOptions], com.google.devtools.build.lib.rules.genquery.GenQueryConfiguration: [com.google.devtools.build.lib.rules.genquery.GenQueryConfiguration$GenQueryOptions], com.google.devtools.build.lib.rules.java.JavaConfiguration: [com.google.devtools.build.lib.analysis.PlatformOptions,com.google.devtools.build.lib.rules.java.JavaOptions], com.google.devtools.build.lib.rules.objc.J2ObjcConfiguration: [com.google.devtools.build.lib.rules.objc.J2ObjcCommandLineOptions], com.google.devtools.build.lib.rules.objc.ObjcConfiguration: [com.google.devtools.build.lib.rules.cpp.CppOptions,com.google.devtools.build.lib.rules.objc.ObjcCommandLineOptions], com.google.devtools.build.lib.rules.proto.ProtoConfiguration: [com.google.devtools.build.lib.rules.proto.ProtoConfiguration$Options], com.google.devtools.build.lib.rules.python.PythonConfiguration: [com.google.devtools.build.lib.rules.python.PythonOptions], 
 FragmentOptions com.google.devtools.build.lib.analysis.PlatformOptions {
   experimental_add_exec_constraints_to_targets: []
@@ -21,7 +21,7 @@
 }
 FragmentOptions com.google.devtools.build.lib.analysis.config.CoreOptions {
   action_env: [MONOGON_SANDBOX_DIGEST=null, PATH=/usr/bin, BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1]
-  affected by starlark transition: [//command_line_option:platforms, @io_bazel_rules_go//go/config:pure, @io_bazel_rules_go//go/config:static]
+  affected by starlark transition: [//command_line_option:platforms, @io_bazel_rules_go//go/config:pure, @io_bazel_rules_go//go/private/rules:original_pure]
   allow_analysis_failures: false
   analysis_testing_deps_limit: 800
   archived_tree_artifact_mnemonics_filter: -(?:(?>.*))
@@ -389,7 +389,5 @@
   python_version: PY3
 }
 FragmentOptions user-defined {
-  @io_bazel_rules_go//go/config:pure: true
-  @io_bazel_rules_go//go/config:static: true
   @io_bazel_rules_go//go/config:tags: [selinux, seccomp, no_zfs, no_aufs, no_devicemapper, providerless, dockerless, nowasm, netgo, osusergo, nobtrfs, nozfs, notapestats, norapl]
 }

This is confirmed by retriggering the build and checking the resulting SHA256:

q3k@sizeableunit ~/Work/monogon $ rm -f bazel-bin/metropolis/node/core/abloader/abloader_bin.efi
q3k@sizeableunit ~/Work/monogon $ bazel build //metropolis/node/core/abloader:abloader_bin --@io_bazel_rules_go//go/config:static=true --platforms //build/platforms:efi_amd64
q3k@sizeableunit ~/Work/monogon $ sha256sum bazel-bin/metropolis/node/core/abloader/abloader_bin.efi
8c61602bba4bcb179e517bcbba594572cbefeca00fd2c7f51b379bac7d70c180  bazel-bin/metropolis/node/core/abloader/abloader_bin.efi
q3k@sizeableunit ~/Work/monogon $ rm -f bazel-bin/metropolis/node/core/abloader/abloader_bin.efi
q3k@sizeableunit ~/Work/monogon $ bazel build //metropolis/node/core/abloader:abloader_bin --
q3k@sizeableunit ~/Work/monogon $ sha256sum bazel-bin/metropolis/node/core/abloader/abloader_bin.efi
6b34aedb137a5f70523c5e10491c075a414be7270cfc2bb4a60adaaf21b74ccf  bazel-bin/metropolis/node/core/abloader/abloader_bin.efi
q3k commented 7 months ago 
q3k@sizeableunit ~/Work/monogon $ cp bazel-bin/metropolis/node/core/abloader/abloader_bin.efi abloader_bin_a.efi
q3k@sizeableunit ~/Work/monogon $ rm -f bazel-bin/metropolis/node/core/abloader/abloader_bin.efi
q3k@sizeableunit ~/Work/monogon $ bazel build //metropolis/node/core/abloader:abloader_bin --
q3k@sizeableunit ~/Work/monogon $ cp bazel-bin/metropolis/node/core/abloader/abloader_bin.efi abloader_bin_b.efi
q3k@sizeableunit ~/Work/monogon $ sha256sum abloader_bin_a.efi 
6b34aedb137a5f70523c5e10491c075a414be7270cfc2bb4a60adaaf21b74ccf  abloader_bin_a.efi
q3k@sizeableunit ~/Work/monogon $ sha256sum abloader_bin_b.efi 
e3b9816f76472ab6e8cd638b89f5e2b8e15948a2a066f7c90042e6308b076b35  abloader_bin_b.efi
q3k@sizeableunit ~/Work/monogon $ diff -u <(xxd abloader_bin_a.efi) <(xxd abloader_bin_b.efi)
--- /dev/fd/63  2023-12-18 15:02:19.541138952 +0100
+++ /dev/fd/62  2023-12-18 15:02:19.542138961 +0100
@@ -6,7 +6,7 @@
 00000050: 6973 2070 726f 6772 616d 2063 616e 6e6f  is program canno
 00000060: 7420 6265 2072 756e 2069 6e20 444f 5320  t be run in DOS 
 00000070: 6d6f 6465 2e24 0000 5045 0000 6486 0500  mode.$..PE..d...
-00000080: 7850 8065 0000 0000 0000 0000 f000 2200  xP.e..........".
+00000080: da50 8065 0000 0000 0000 0000 f000 2200  .P.e..........".
 00000090: 0b02 0e00 00e4 0100 0062 0000 0000 0000  .........b......
 000000a0: 4073 0000 0010 0000 0000 0000 0000 0000  @s..............
 000000b0: 0010 0000 0002 0000 0600 0000 0000 0000  ................

This single byte difference lives in the TimeDateStamp in the COFF Header of the resulting PE file.

q3k commented 7 months ago

@lorenz Pretty sure this would be fixed by

diff --git a/build/toolchain/llvm-efi/cc_toolchain_config.bzl b/build/toolchain/llvm-efi/cc_toolchain_config.bzl
index 697b6750..f9fd239a 100644
--- a/build/toolchain/llvm-efi/cc_toolchain_config.bzl
+++ b/build/toolchain/llvm-efi/cc_toolchain_config.bzl
@@ -126,6 +126,7 @@ def _efi_k8_cc_toolchain_impl(ctx):
                             "-Wl,-entry:efi_main",
                             "-Wl,-subsystem:efi_application",
                             "-Wl,/BASE:0x0",
+                            "-Wl,/Brepro",
                             "-nostdlib",
                             "build/toolchain/llvm-efi/fltused.o",
                         ],

Could you check if applying this patch fixes spurious abloader updates?

q3k commented 7 months ago

I found a few more reproducibility issues and fixed them in https://review.monogon.dev/c/monogon/+/2583

@lorenz Please verify.

I've tested it in the following way (as per our chat):

$ bazel test //metropolis/node/core/update/e2e:e2e_test --test_arg=-test.v --test_output=all --runs_per_test=2
$ grep 'Replacing A/B' bazel-testlogs/metropolis/node/core/update/e2e/e2e_test/run_{1,2}_of_2/test.log
lorenz commented 7 months ago

Fixed in https://review.monogon.dev/c/monogon/+/2583