From 19ae87e5cec71912c7e7ecec5dc8ff18d18c60ee Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Mon, 17 Jun 2024 23:47:51 -0700 Subject: [PATCH 1/4] tree-wide: add support for asan! What if you could find memory bugs in Lix without really trying very hard? I've had variously scuffed patches to do this, but this is blocked on boost coroutines removal at this point tbh. Change-Id: Id762af076aa06ad51e77a6c17ed10275929ed578 --- .clang-tidy | 3 +++ meson.build | 15 +++++++++++++-- src/asan-options/asan-options.cc | 17 +++++++++++++++++ src/meson.build | 11 ++++++++++- src/nix/meson.build | 1 + .../repl_characterization/meson.build | 1 + .../test-libstoreconsumer/meson.build | 1 + tests/unit/meson.build | 17 +++++++++++++---- 8 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 src/asan-options/asan-options.cc diff --git a/.clang-tidy b/.clang-tidy index 3b5dcd91a..0cc1f2520 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -16,3 +16,6 @@ Checks: - -bugprone-unchecked-optional-access # many warnings, seems like a questionable lint - -bugprone-branch-clone + +CheckOptions: + bugprone-reserved-identifier.AllowedIdentifiers: '__asan_default_options' diff --git a/meson.build b/meson.build index 56f447501..ed50dff78 100644 --- a/meson.build +++ b/meson.build @@ -199,7 +199,11 @@ configdata = { } # Dependencies # -boehm = dependency('bdw-gc', required : get_option('gc'), version : '>=8.2.6') +gc_opt = get_option('gc').disable_if( + 'address' in get_option('b_sanitize'), + error_message: 'gc does far too many memory crimes for ASan' +) +boehm = dependency('bdw-gc', required : gc_opt, version : '>=8.2.6') configdata += { 'HAVE_BOEHMGC': boehm.found().to_int(), } @@ -482,7 +486,14 @@ if cxx.get_id() == 'clang' and get_option('b_sanitize') != '' add_project_link_arguments('-shared-libsan', language : 'cpp') endif +# Clang gets grumpy about missing libasan symbols if -shared-libasan is not +# passed when building shared libs, at least on Linux +if cxx.get_id() == 'clang' and 'address' in get_option('b_sanitize') + add_project_link_arguments('-shared-libasan', language : 'cpp') +endif + add_project_link_arguments('-pthread', language : 'cpp') + if cxx.get_linker_id() in ['ld.bfd', 'ld.gold'] add_project_link_arguments('-Wl,--no-copy-dt-needed-entries', language : 'cpp') endif @@ -497,7 +508,7 @@ endif # maintainers/buildtime_report.sh BUILD-DIR to simply work in clang builds. # # They can also be manually viewed at https://ui.perfetto.dev -if get_option('profile-build').require(meson.get_compiler('cpp').get_id() == 'clang').enabled() +if get_option('profile-build').require(cxx.get_id() == 'clang').enabled() add_project_arguments('-ftime-trace', language: 'cpp') endif diff --git a/src/asan-options/asan-options.cc b/src/asan-options/asan-options.cc new file mode 100644 index 000000000..c4cf360af --- /dev/null +++ b/src/asan-options/asan-options.cc @@ -0,0 +1,17 @@ +/// @file This is very bothersome code that has to be included in every +/// executable to get the correct default ASan options. I am so sorry. + +extern "C" [[gnu::retain]] const char *__asan_default_options() +{ + // We leak a bunch of memory knowingly on purpose. It's not worthwhile to + // diagnose that memory being leaked for now. + // + // Instruction bytes are useful for finding the actual code that + // corresponds to an ASan report. + // + // TODO: setting log_path=asan.log or not: neither works, since you can't + // write to the fs in certain places in the testsuite, but you also cannot + // write arbitrarily to stderr in other places so the reports get eaten. + // pain 🥖 + return "halt_on_error=1:abort_on_error=1:detect_leaks=0:print_summary=1:dump_instruction_bytes=1"; +} diff --git a/src/meson.build b/src/meson.build index 3fc5595b8..e918ae392 100644 --- a/src/meson.build +++ b/src/meson.build @@ -12,10 +12,19 @@ subdir('libmain') # libcmd depends on everything subdir('libcmd') - # The rest of the subdirectories aren't separate components, # just source files in another directory, so we process them here. +# Static library that just sets default ASan options. It needs to be included +# in every executable. +asanoptions = static_library( + 'libasanoptions', + files('asan-options/asan-options.cc'), +) +libasanoptions = declare_dependency( + link_whole: asanoptions +) + build_remote_sources = files( 'build-remote/build-remote.cc', ) diff --git a/src/nix/meson.build b/src/nix/meson.build index 22f148fcb..97387e402 100644 --- a/src/nix/meson.build +++ b/src/nix/meson.build @@ -80,6 +80,7 @@ nix = executable( profiles_md_gen, nix2_commands_sources, dependencies : [ + libasanoptions, liblixcmd, liblixutil_mstatic, liblixstore_mstatic, diff --git a/tests/functional/repl_characterization/meson.build b/tests/functional/repl_characterization/meson.build index 56410cfd2..79de9a5f5 100644 --- a/tests/functional/repl_characterization/meson.build +++ b/tests/functional/repl_characterization/meson.build @@ -7,6 +7,7 @@ repl_characterization_tester = executable( 'test-repl-characterization', repl_characterization_tester_sources, dependencies : [ + libasanoptions, liblixutil, liblixutil_test_support, sodium, diff --git a/tests/functional/test-libstoreconsumer/meson.build b/tests/functional/test-libstoreconsumer/meson.build index ad96aac12..63d0c97ac 100644 --- a/tests/functional/test-libstoreconsumer/meson.build +++ b/tests/functional/test-libstoreconsumer/meson.build @@ -2,6 +2,7 @@ libstoreconsumer_tester = executable( 'test-libstoreconsumer', 'main.cc', dependencies : [ + libasanoptions, liblixutil, liblixstore, sodium, diff --git a/tests/unit/meson.build b/tests/unit/meson.build index c449b2276..55c7566bd 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -11,6 +11,10 @@ # functions, the result would be way less readable than just a bit of copypasta. # It's only ~200 lines; better to just refactor the tests themselves which we'll want to do anyway. +default_test_env = { + 'ASAN_OPTIONS': 'detect_leaks=0:halt_on_error=1:abort_on_error=1:print_summary=1:dump_instruction_bytes=1' +} + libutil_test_support_sources = files( 'libutil-support/tests/cli-literate-parser.cc', 'libutil-support/tests/hash.cc', @@ -63,6 +67,7 @@ libutil_tester = executable( 'liblixutil-tests', libutil_tests_sources, dependencies : [ + libasanoptions, rapidcheck, gtest, boehm, @@ -78,7 +83,7 @@ test( 'libutil-unit-tests', libutil_tester, args : tests_args, - env : { + env : default_test_env + { '_NIX_TEST_UNIT_DATA': meson.project_source_root() / 'tests/unit/libutil/data', }, suite : 'check', @@ -132,6 +137,7 @@ libstore_tester = executable( 'liblixstore-tests', libstore_tests_sources, dependencies : [ + libasanoptions, liblixstore_test_support, liblixutil_test_support, liblixstore_mstatic, @@ -147,7 +153,7 @@ test( 'libstore-unit-tests', libstore_tester, args : tests_args, - env : { + env : default_test_env + { '_NIX_TEST_UNIT_DATA': meson.project_source_root() / 'tests/unit/libstore/data', }, suite : 'check', @@ -196,6 +202,7 @@ libexpr_tester = executable( 'liblixexpr-tests', libexpr_tests_sources, dependencies : [ + libasanoptions, liblixexpr_test_support, liblixstore_test_support, liblixstore_mstatic, @@ -214,7 +221,7 @@ test( 'libexpr-unit-tests', libexpr_tester, args : tests_args, - env : { + env : default_test_env + { '_NIX_TEST_UNIT_DATA': meson.project_source_root() / 'tests/unit/libexpr/data', }, suite : 'check', @@ -226,6 +233,7 @@ libcmd_tester = executable( 'liblixcmd-tests', files('libcmd/args.cc'), dependencies : [ + libasanoptions, liblixcmd, liblixutil, liblixmain, @@ -241,7 +249,7 @@ test( 'libcmd-unit-tests', libcmd_tester, args : tests_args, - env : { + env : default_test_env + { # No special meaning here, it's just a file laying around that is unlikely to go anywhere # any time soon. '_NIX_TEST_UNIT_DATA': meson.project_source_root() / 'src/nix-env/buildenv.nix', @@ -272,6 +280,7 @@ test( 'libmain-unit-tests', libmain_tester, args : tests_args, + env : default_test_env, suite : 'check', protocol : 'gtest', ) From e51263057f21e77602481d6a6d826ff8cc0c1db0 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Tue, 23 Jul 2024 22:43:38 +0200 Subject: [PATCH 2/4] ci: add a asan+ubsan test run on x86_64-linux This should at least catch out blatantly bad patches that don't pass the test suite with ASan. We don't do this to the integration tests since they run on relatively limited-memory VMs and so it may not be super safe to run an evaluator with leak driven garbage collection for them. Fixes: https://git.lix.systems/lix-project/lix/issues/403 Fixes: https://git.lix.systems/lix-project/lix/issues/319 Change-Id: I5267b02626866fd33e8b4d8794344531af679f78 --- flake.nix | 9 +++++++++ package.nix | 13 ++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/flake.nix b/flake.nix index cec970974..9a3087c4b 100644 --- a/flake.nix +++ b/flake.nix @@ -275,6 +275,15 @@ # System tests. tests = import ./tests/nixos { inherit lib nixpkgs nixpkgsFor; } // { + # This is x86_64-linux only, just because we have significantly + # cheaper x86_64-linux compute in CI. + # It is clangStdenv because clang's sanitizers are nicer. + asanBuild = self.packages.x86_64-linux.nix-clangStdenv.override { + sanitize = [ + "address" + "undefined" + ]; + }; # Make sure that nix-env still produces the exact same result # on a particular version of Nixpkgs. diff --git a/package.nix b/package.nix index 61015bac9..0807ec1de 100644 --- a/package.nix +++ b/package.nix @@ -57,6 +57,10 @@ buildUnreleasedNotes ? true, internalApiDocs ? false, + # List of Meson sanitize options. Accepts values of b_sanitize, e.g. + # "address", "undefined", "thread". + sanitize ? null, + # Not a real argument, just the only way to approximate let-binding some # stuff for argument defaults. __forDefaults ? { @@ -166,6 +170,12 @@ stdenv.mkDerivation (finalAttrs: { dontBuild = false; mesonFlags = + let + sanitizeOpts = lib.optionals (sanitize != null) ( + [ "-Db_sanitize=${builtins.concatStringsSep "," sanitize}" ] + ++ lib.optional (builtins.elem "address" sanitize) "-Dgc=disabled" + ); + in lib.optionals hostPlatform.isLinux [ # You'd think meson could just find this in PATH, but busybox is in buildInputs, # which don't actually get added to PATH. And buildInputs is correct over @@ -182,7 +192,8 @@ stdenv.mkDerivation (finalAttrs: { (lib.mesonBool "enable-tests" finalAttrs.finalPackage.doCheck) (lib.mesonBool "enable-docs" canRunInstalled) ] - ++ lib.optional (hostPlatform != buildPlatform) "--cross-file=${mesonCrossFile}"; + ++ lib.optional (hostPlatform != buildPlatform) "--cross-file=${mesonCrossFile}" + ++ sanitizeOpts; # We only include CMake so that Meson can locate toml11, which only ships CMake dependency metadata. dontUseCmakeConfigure = true; From b5c6ce7a537d91ee22d6876ba0166259da2ac3c0 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Tue, 23 Jul 2024 22:53:56 +0200 Subject: [PATCH 3/4] Add -Werror CI job We should cause CLs that introduce compiler warnings to fail CI. Sadly this will only cover Clang, but it will cover Clang for free, so it's truly impossible to say if it's bad or not. Change-Id: I45ca20d77251af9671d5cbe0d29cb08c5f1d03c2 --- flake.nix | 4 ++++ package.nix | 3 +++ 2 files changed, 7 insertions(+) diff --git a/flake.nix b/flake.nix index 9a3087c4b..440320afa 100644 --- a/flake.nix +++ b/flake.nix @@ -283,6 +283,10 @@ "address" "undefined" ]; + # it is very hard to make *every* CI build use this option such + # that we don't wind up building Lix twice, so we do it here where + # we are already doing so. + werror = true; }; # Make sure that nix-env still produces the exact same result diff --git a/package.nix b/package.nix index 0807ec1de..9f983f66b 100644 --- a/package.nix +++ b/package.nix @@ -60,6 +60,8 @@ # List of Meson sanitize options. Accepts values of b_sanitize, e.g. # "address", "undefined", "thread". sanitize ? null, + # Turn compiler warnings into errors. + werror ? false, # Not a real argument, just the only way to approximate let-binding some # stuff for argument defaults. @@ -191,6 +193,7 @@ stdenv.mkDerivation (finalAttrs: { (lib.mesonEnable "internal-api-docs" internalApiDocs) (lib.mesonBool "enable-tests" finalAttrs.finalPackage.doCheck) (lib.mesonBool "enable-docs" canRunInstalled) + (lib.mesonBool "werror" werror) ] ++ lib.optional (hostPlatform != buildPlatform) "--cross-file=${mesonCrossFile}" ++ sanitizeOpts; From 5eecdd3ae9f47b1aaac22134eced318ff3e4bc41 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Tue, 23 Jul 2024 23:25:18 +0200 Subject: [PATCH 4/4] releng: move officialRelease to version.json This was causing a few bits of suffering downstream, in particular, in the NixOS module, which, after this change, can have the `officialRelease` stuff in *it* completely deleted since we now have correct defaulting in package.nix for it. It also eliminates some automated editing of Nix files, which is certainly always welcome to eliminate. Fixes: https://git.lix.systems/lix-project/lix/issues/406 Change-Id: Id12f3018cff4633e379dbfcbe26b7bc84922bdaf --- flake.nix | 5 +++-- package.nix | 9 ++++----- releng/README.md | 2 +- releng/create_release.xsh | 19 ++++++++++++------- releng/version.py | 1 + version.json | 1 + 6 files changed, 22 insertions(+), 15 deletions(-) diff --git a/flake.nix b/flake.nix index 440320afa..d2173cf47 100644 --- a/flake.nix +++ b/flake.nix @@ -59,7 +59,8 @@ (Run `touch .nocontribmsg` to hide this message.) ''; - officialRelease = false; + versionJson = builtins.fromJSON (builtins.readFile ./version.json); + officialRelease = versionJson.official_release; # Set to true to build the release notes for the next release. buildUnreleasedNotes = true; @@ -419,7 +420,7 @@ pkgs: stdenv: let nix = pkgs.callPackage ./package.nix { - inherit stdenv officialRelease versionSuffix; + inherit stdenv versionSuffix; busybox-sandbox-shell = pkgs.busybox-sandbox-shell or pkgs.default-busybox-sandbox; internalApiDocs = false; }; diff --git a/package.nix b/package.nix index 9f983f66b..1b711585d 100644 --- a/package.nix +++ b/package.nix @@ -52,7 +52,7 @@ pname ? "lix", versionSuffix ? "", - officialRelease ? false, + officialRelease ? __forDefaults.versionJson.official_release, # Set to true to build the release notes for the next release. buildUnreleasedNotes ? true, internalApiDocs ? false, @@ -68,6 +68,8 @@ __forDefaults ? { canRunInstalled = stdenv.buildPlatform.canExecute stdenv.hostPlatform; + versionJson = builtins.fromJSON (builtins.readFile ./version.json); + boehmgc-nix = boehmgc.override { enableLargeConfig = true; }; editline-lix = editline.overrideAttrs (prev: { @@ -83,8 +85,7 @@ let inherit (lib) fileset; inherit (stdenv) hostPlatform buildPlatform; - versionJson = builtins.fromJSON (builtins.readFile ./version.json); - version = versionJson.version + versionSuffix; + version = __forDefaults.versionJson.version + versionSuffix; aws-sdk-cpp-nix = aws-sdk-cpp.override { apis = [ @@ -381,8 +382,6 @@ stdenv.mkDerivation (finalAttrs: { pegtl ; - inherit officialRelease; - # The collection of dependency logic for this derivation is complicated enough that # it's easier to parameterize the devShell off an already called package.nix. mkDevShell = diff --git a/releng/README.md b/releng/README.md index cfacf4b8e..2aa3b959f 100644 --- a/releng/README.md +++ b/releng/README.md @@ -30,7 +30,7 @@ First, we prepare the release. `python -m releng prepare` is used for this. Then we tag the release with `python -m releng tag`: * Git HEAD is detached. -* `officialRelease = true` is set in `flake.nix`, this is committed, and a +* `"official_release": true` is set in `version.json`, this is committed, and a release is tagged. * The tag is merged back into the last branch (either `main` for new releases or `release-MAJOR` for maintenance releases) with `git merge -s ours VERSION` diff --git a/releng/create_release.xsh b/releng/create_release.xsh index 358124359..62114350b 100644 --- a/releng/create_release.xsh +++ b/releng/create_release.xsh @@ -11,7 +11,7 @@ from . import environment from .environment import RelengEnvironment from . import keys from . import docker -from .version import VERSION, RELEASE_NAME, MAJOR +from .version import VERSION, RELEASE_NAME, MAJOR, OFFICIAL_RELEASE from .gitutils import verify_are_on_tag, git_preconditions from . import release_notes @@ -39,12 +39,18 @@ def setup_creds(env: RelengEnvironment): def official_release_commit_tag(force_tag=False): - print('[+] Setting officialRelease in flake.nix and tagging') + print('[+] Setting officialRelease in version.json and tagging') prev_branch = $(git symbolic-ref --short HEAD).strip() git switch --detach - sed -i 's/officialRelease = false/officialRelease = true/' flake.nix - git add flake.nix + + # Must be done in two parts due to buffering (opening the file immediately + # would truncate it). + new_version_json = $(jq --indent 4 '.official_release = true' version.json) + with open('version.json', 'w') as fh: + fh.write(new_version_json) + git add version.json + message = f'release: {VERSION} "{RELEASE_NAME}"\n\nRelease produced with releng/create_release.xsh' git commit -m @(message) git tag @(['-f'] if force_tag else []) -a -m @(message) @(VERSION) @@ -250,15 +256,14 @@ def build_manual(eval_result): def upload_manual(env: RelengEnvironment): - stable = json.loads($(nix eval --json '.#nix.officialRelease')) - if stable: + if OFFICIAL_RELEASE: version = MAJOR else: version = 'nightly' print('[+] aws s3 sync manual') aws s3 sync @(MANUAL)/ @(env.docs_bucket)/manual/lix/@(version)/ - if stable: + if OFFICIAL_RELEASE: aws s3 sync @(MANUAL)/ @(env.docs_bucket)/manual/lix/stable/ diff --git a/releng/version.py b/releng/version.py index 47ef23504..4ad188d46 100644 --- a/releng/version.py +++ b/releng/version.py @@ -4,3 +4,4 @@ version_json = json.load(open('version.json')) VERSION = version_json['version'] MAJOR = '.'.join(VERSION.split('.')[:2]) RELEASE_NAME = version_json['release_name'] +OFFICIAL_RELEASE = version_json['official_release'] diff --git a/version.json b/version.json index 48db2994f..809358e6d 100644 --- a/version.json +++ b/version.json @@ -1,4 +1,5 @@ { "version": "2.91.0-dev", + "official_release": false, "release_name": "TBA" }