diff --git a/.github/actions/bench/action.yml b/.github/actions/bench/action.yml index bdc18266cb..48461ce751 100644 --- a/.github/actions/bench/action.yml +++ b/.github/actions/bench/action.yml @@ -26,7 +26,7 @@ inputs: description: opt flag to set for tests script default: "true" bench_extra_args: - description: Further arguments to be appended to command line for `bench` script + description: Newline-delimited additional arguments for the `bench` script default: "" store_results: description: Whether to push results to GH pages @@ -56,6 +56,8 @@ runs: using: composite steps: - uses: ./.github/actions/setup-shell + env: + BENCH_CROSS_PREFIX: ${{ inputs.cross_prefix }} with: nix-shell: ${{ inputs.nix-shell }} nix-cache: ${{ inputs.nix-cache }} @@ -63,13 +65,18 @@ runs: gh_token: ${{ inputs.gh_token }} custom_shell: ${{ inputs.custom_shell }} script: | + safe_cross_prefix_re='^[A-Za-z0-9_./+-]*$' + if [[ ! "$BENCH_CROSS_PREFIX" =~ $safe_cross_prefix_re ]]; then + printf 'cross_prefix contains unsupported characters\n' >&2 + exit 1 + fi ARCH=$(uname -m) cat >> $GITHUB_STEP_SUMMARY <<-EOF ## Setup Architecture: $ARCH - $(uname -a) - $(nix --version) - - $(${{ matrix.target.cross_prefix }}gcc --version | grep -m1 "") + - $("${BENCH_CROSS_PREFIX}gcc" --version | grep -m1 "") - $(bash --version | grep -m1 "") ## CPU Info @@ -77,18 +84,60 @@ runs: EOF - name: Run benchmark shell: ${{ env.SHELL }} + env: + BENCH_ARCHFLAGS: ${{ inputs.archflags }} + BENCH_CFLAGS: ${{ inputs.cflags }} + BENCH_CROSS_PREFIX: ${{ inputs.cross_prefix }} + BENCH_EXTRA_ARGS: ${{ inputs.bench_extra_args }} + BENCH_LDFLAGS: ${{ inputs.ldflags }} + BENCH_OPT: ${{ inputs.opt }} + BENCH_PERF: ${{ inputs.perf }} run: | - ./scripts/tests bench -c ${{ inputs.perf }} --cross-prefix="${{ inputs.cross_prefix }}" \ - --cflags="${{ inputs.cflags }} ${{ inputs.archflags }}" \ - --ldflags="${{ inputs.ldflags }}" \ - --opt=$([[ ${{ inputs.opt }} == "false" ]] && echo "no_opt" || echo "opt") \ - -v --output=output.json ${{ inputs.bench_extra_args }} + validate_value() { + local name=$1 + local value=$2 + local pattern=$3 + if [[ ! "$value" =~ $pattern ]]; then + printf '%s contains unsupported characters\n' "$name" >&2 + exit 1 + fi + } - ./scripts/tests bench --components -c ${{ inputs.perf }} --cross-prefix="${{ inputs.cross_prefix }}" \ - --cflags="${{ inputs.cflags }} ${{ inputs.archflags }}" \ - --ldflags="${{ inputs.ldflags }}" \ - --opt=$([[ ${{ inputs.opt }} == "false" ]] && echo "no_opt" || echo "opt") \ - -v ${{ inputs.bench_extra_args }} + validate_value perf "$BENCH_PERF" '^(NO|PMU|PERF|MAC)$' + validate_value cross_prefix "$BENCH_CROSS_PREFIX" '^[A-Za-z0-9_./+-]*$' + validate_value cflags "$BENCH_CFLAGS" '^[A-Za-z0-9_.,:+=/@% -]*$' + validate_value archflags "$BENCH_ARCHFLAGS" '^[A-Za-z0-9_.,:+=/@% -]*$' + validate_value ldflags "$BENCH_LDFLAGS" '^[A-Za-z0-9_.,:+=/@% -]*$' + + if [[ "$BENCH_OPT" == "false" ]]; then + bench_opt=no_opt + elif [[ "$BENCH_OPT" == "true" ]]; then + bench_opt=opt + else + printf 'opt must be true or false\n' >&2 + exit 1 + fi + + extra_args=() + if [[ -n "$BENCH_EXTRA_ARGS" ]]; then + while IFS= read -r arg; do + [[ -z "$arg" ]] && continue + extra_args+=("$arg") + done <<< "$BENCH_EXTRA_ARGS" + fi + + common_args=( + ./scripts/tests bench + -c "$BENCH_PERF" + "--cross-prefix=$BENCH_CROSS_PREFIX" + "--cflags=$BENCH_CFLAGS $BENCH_ARCHFLAGS" + "--ldflags=$BENCH_LDFLAGS" + "--opt=$bench_opt" + -v + ) + + "${common_args[@]}" --output=output.json "${extra_args[@]}" + "${common_args[@]}" --components "${extra_args[@]}" - name: Check namespace shell: ${{ env.SHELL }} run: | diff --git a/.github/workflows/bench_ec2_any.yml b/.github/workflows/bench_ec2_any.yml index 22ef13604f..67fb95318d 100644 --- a/.github/workflows/bench_ec2_any.yml +++ b/.github/workflows/bench_ec2_any.yml @@ -43,7 +43,7 @@ on: - opt - no_opt bench_extra_args: - description: Additional command line to be appended to `tests bench` script + description: Newline-delimited additional arguments for `tests bench` default: '' compiler: description: Compiler to use. When unset, default nix shell is used. diff --git a/.github/workflows/bench_ec2_reusable.yml b/.github/workflows/bench_ec2_reusable.yml index af30ed8230..be5bed1bd4 100644 --- a/.github/workflows/bench_ec2_reusable.yml +++ b/.github/workflows/bench_ec2_reusable.yml @@ -52,7 +52,7 @@ on: default: false bench_extra_args: type: string - description: Additional command line to be appended to `bench` script + description: Newline-delimited additional arguments for the `bench` script default: '' compiler: type: string @@ -97,18 +97,35 @@ jobs: ec2-instance-id: ${{ steps.start-ec2-runner.outputs.ec2-instance-id }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false - name: Determine AMI ID id: det_ami_id + env: + EC2_AMI: ${{ inputs.ec2_ami }} + EC2_AMI_ID: ${{ inputs.ec2_ami_id }} run: | - if [[ "${{ inputs.ec2_ami }}" == "ubuntu-latest (x86_64)" ]]; then - AMI_ID=${{ env.AMI_UBUNTU_LATEST_X86_64 }} - elif [[ "${{ inputs.ec2_ami }}" == "ubuntu-latest (aarch64)" ]]; then - AMI_ID=${{ env.AMI_UBUNTU_LATEST_AARCH64 }} - elif [[ "${{ inputs.ec2_ami }}" == "ubuntu-latest (custom AMI)" ]]; then - AMI_ID=${{ inputs.ec2_ami_id }} - fi - echo "Using AMI ID: $AMI_ID" - echo "AMI_ID=$AMI_ID" >> "$GITHUB_OUTPUT" + case "$EC2_AMI" in + "ubuntu-latest (x86_64)") + AMI_ID="$AMI_UBUNTU_LATEST_X86_64" + ;; + "ubuntu-latest (aarch64)") + AMI_ID="$AMI_UBUNTU_LATEST_AARCH64" + ;; + "ubuntu-latest (custom AMI)") + if [[ ! "$EC2_AMI_ID" =~ ^ami-[0-9a-f]{8,17}$ ]]; then + printf 'Invalid custom AMI ID: %s\n' "$EC2_AMI_ID" >&2 + exit 1 + fi + AMI_ID="$EC2_AMI_ID" + ;; + *) + printf 'Unsupported AMI label: %s\n' "$EC2_AMI" >&2 + exit 1 + ;; + esac + printf 'Using AMI ID: %s\n' "$AMI_ID" + printf 'AMI_ID=%s\n' "$AMI_ID" >> "$GITHUB_OUTPUT" - name: Configure AWS credentials uses: aws-actions/configure-aws-credentials@e7f100cf4c008499ea8adda475de1042d6975c7b # v6.2.0 with: @@ -139,6 +156,8 @@ jobs: if: ${{ inputs.compiler == '' }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false - uses: ./.github/actions/bench if: ${{ inputs.opt == 'all' || inputs.opt == 'opt' }} with: @@ -176,12 +195,20 @@ jobs: if: ${{ inputs.compiler != '' }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false - uses: ./.github/actions/setup-apt with: packages: ${{ inputs.additional_packages }} - name: Set compiler + env: + BENCH_COMPILER: ${{ inputs.compiler }} run: | - echo "CC=${{ inputs.compiler }}" >> "$GITHUB_ENV" + if [[ ! "$BENCH_COMPILER" =~ ^[A-Za-z0-9_./+-]+$ ]]; then + printf 'compiler contains unsupported characters\n' >&2 + exit 1 + fi + printf 'CC=%s\n' "$BENCH_COMPILER" >> "$GITHUB_ENV" - uses: ./.github/actions/bench if: ${{ inputs.opt == 'all' || inputs.opt == 'opt' }} with: diff --git a/scripts/lint b/scripts/lint index 27d4a91e34..bfb78fe64f 100755 --- a/scripts/lint +++ b/scripts/lint @@ -109,6 +109,25 @@ gh_error_simple() fi } +list-shell-scripts() +{ + git ls-files -z -- ":/" | xargs -0 shfmt -f +} + +lint-shell-format() +{ + local file out="" result + while IFS= read -r file; do + [[ -z $file ]] && continue + if ! result=$(shfmt -s -l -i 2 -ci -fn "$file" 2>&1); then + out+="$result"$'\n' + elif [[ -n $result ]]; then + out+="$result"$'\n' + fi + done <<<"$SHELL_SCRIPTS" + printf "%s" "$out" +} + run-shellcheck() { if ! command -v shellcheck >/dev/null; then @@ -119,21 +138,30 @@ run-shellcheck() return 0 fi - checkerr "Lint shellcheck" "$(echo $SHELL_SCRIPTS | xargs shellcheck --severity=warning)" + local file out="" result + while IFS= read -r file; do + [[ -z $file ]] && continue + if ! result=$(shellcheck --severity=warning "$file" 2>&1); then + out+="$result"$'\n' + elif [[ -n $result ]]; then + out+="$result"$'\n' + fi + done <<<"$SHELL_SCRIPTS" + checkerr "Lint shellcheck" "$out" } # Formatting SUCCESS=true # Get list of shell scripts for linting -SHELL_SCRIPTS=$(git grep -l '' :/ | xargs printf "'%s' " | xargs -L 1 shfmt -f) +SHELL_SCRIPTS=$(list-shell-scripts) gh_group_start "Linting nix files with nixpkgs-fmt" checkerr "Lint nix" "$(nixpkgs-fmt --check "$ROOT")" gh_group_end gh_group_start "Linting shell scripts with shfmt" -checkerr "Lint shell" "$(echo $SHELL_SCRIPTS | xargs -L 1 shfmt -s -l -i 2 -ci -fn)" +checkerr "Lint shell" "$(lint-shell-format)" gh_group_end gh_group_start "Linting shell scripts with shellcheck" @@ -185,12 +213,13 @@ gh_group_end gh_group_start "Linting c files with clang-format" lint-c-files() { - for file in $(git ls-files -- ":/*.c" ":/*.h"); do + local file out + while IFS= read -r -d '' file; do # Ignore symlinks - if [[ ! -L $file ]]; then - clang-format --Werror --dry-run "$file" 2>&1 | grep "error:" | cut -d ':' -f 1,2 | tr ':' ' ' + if [[ ! -L $file ]] && ! out=$(clang-format --Werror --dry-run "$file" 2>&1); then + printf "%s\n" "$out" | grep "error:" | cut -d ':' -f 1,2 | tr ':' ' ' || true fi - done + done < <(git ls-files -z -- ":/*.c" ":/*.h") } checkerr "Lint C" "$(lint-c-files)" gh_group_end @@ -214,17 +243,23 @@ lint-clang-tidy() local nproc success=true nproc=$(getconf _NPROCESSORS_ONLN 2>/dev/null || echo 1) - # Run clang-tidy over a set of files. $1 is a label for diagnostics, $2 is a - # space-separated list of git pathspecs, and the rest are passed verbatim to - # clang-tidy's compiler invocation (after `--`). Note: not invoked via a pipe - # so that `success=false` propagates to the enclosing function. + # Run clang-tidy over a set of files. $1 is a label for diagnostics, followed + # by git pathspecs, `--`, and clang-tidy compiler invocation arguments. Note: + # not invoked via a pipe so that `success=false` propagates to the enclosing + # function. run-clang-tidy() { - local label="$1" pathspecs="$2" out - shift 2 - if ! out=$(git ls-files -- $pathspecs | - xargs -P "$nproc" -I {} \ - clang-tidy --quiet {} -- -I"$ROOT"/mldsa -std=c90 "$@" 2>&1); then + local label="$1" out + local pathspecs=() + shift + while [[ $# -gt 0 && $1 != "--" ]]; do + pathspecs+=("$1") + shift + done + shift + if ! out=$(git ls-files -z -- "${pathspecs[@]}" | + xargs -0 -P "$nproc" -I {} \ + clang-tidy --quiet "{}" -- -I"$ROOT"/mldsa -std=c90 "$@" 2>&1); then echo "$out" gh_error_simple "clang-tidy error" "clang-tidy reported findings ($label)" success=false @@ -233,9 +268,9 @@ lint-clang-tidy() for params in 44 65 87; do # Headers are passed with `-x c` so clang-tidy treats them as C, not C++. - run-clang-tidy "ML-DSA-$params .c" "${c_files[*]}" \ + run-clang-tidy "ML-DSA-$params .c" "${c_files[@]}" -- \ -DMLD_CONFIG_PARAMETER_SET="$params" - run-clang-tidy "ML-DSA-$params .h" "${h_files[*]}" \ + run-clang-tidy "ML-DSA-$params .h" "${h_files[@]}" -- \ -x c -DMLD_CONFIG_PARAMETER_SET="$params" done @@ -255,13 +290,14 @@ gh_group_end check-eol-dry-run() { - for file in $(git ls-files -- ":/" ":/!:*.png"); do + local file l + while IFS= read -r -d '' file; do # Ignore symlinks if [[ ! -L $file && $(tail -c1 "$file" | wc -l) == 0 ]]; then l=$(wc -l <"$file") echo "$file $l" fi - done + done < <(git ls-files -z -- ":/" ":/!:*.png") } gh_group_start "Checking eol" checkerr "Check eol" "$(check-eol-dry-run)" @@ -269,29 +305,61 @@ gh_group_end check-spdx() { - local success=true - for file in $(git ls-files -- ":/" ":/!:*.json" ":/!:*.png" ":/!:*LICENSE*" ":/!:.git*" ":/!:flake.lock"); do + local file success=true + local license_pathspecs=( + ":/" + ":/!:*.json" + ":/!:*.png" + ":/!:*LICENSE*" + ":/!:.git*" + ":/!:flake.lock" + ) + local copyright_pathspecs=( + "*.[chsS]" + "*.py" + "*.mk" + "*.yml" + "**/Makefile*" + ":/!proofs/cbmc/*.py" + ":/!examples/bring_your_own_fips202/custom_fips202/tiny_sha3/*" + ":/!examples/custom_backend/mldsa_native/src/fips202/native/custom/src/*" + ) + while IFS= read -r -d '' file; do # Ignore symlinks - if [[ ! -L $file && $(grep "SPDX-License-Identifier:" "$file" | wc -l) == 0 ]]; then - gh_error "$file" "${line:-1}" "Missing license header error" "$file is missing SPDX License header" + if [[ ! -L $file ]] && ! grep -q "SPDX-License-Identifier:" "$file"; then + gh_error \ + "$file" \ + "${line:-1}" \ + "Missing license header error" \ + "$file is missing SPDX License header" success=false fi - done - for file in $(git ls-files -- "*.[chsS]" "*.py" "*.mk" "*.yml" "**/Makefile*" ":/!proofs/cbmc/*.py" ":/!examples/bring_your_own_fips202/custom_fips202/tiny_sha3/*" ":/!examples/custom_backend/mldsa_native/src/fips202/native/custom/src/*"); do + done < <(git ls-files -z -- "${license_pathspecs[@]}") + while IFS= read -r -d '' file; do # Ignore symlinks - if [[ ! -L $file && $(grep "Copyright (c) The mldsa-native project authors" "$file" | wc -l) == 0 ]]; then - gh_error "$file" "${line:-1}" "Missing copyright header error" "$file is missing copyright header" + if [[ ! -L $file ]] && ! grep -q "Copyright (c) The mldsa-native project authors" "$file"; then + gh_error \ + "$file" \ + "${line:-1}" \ + "Missing copyright header error" \ + "$file is missing copyright header" success=false fi - done + done < <(git ls-files -z -- "${copyright_pathspecs[@]}") # For source files in dev/* and mldsa/*, we enforce `Apache-2.0 OR ISC OR MIT` - for file in $(git ls-files -- "*.[chsSi]" | grep "^dev/\|^mldsa/"); do + while IFS= read -r -d '' file; do + [[ $file == dev/* || $file == mldsa/* ]] || continue # Ignore symlinks - if [[ ! -L $file && $(grep "SPDX-License-Identifier: Apache-2.0 OR ISC OR MIT" "$file" | wc -l) == 0 ]]; then - gh_error "$file" "${line:-1}" "Missing license header error" "$file is not licensed under 'Apache-2.0 OR ISC OR MIT'" + if [[ ! -L $file ]] && + ! grep -q "SPDX-License-Identifier: Apache-2.0 OR ISC OR MIT" "$file"; then + gh_error \ + "$file" \ + "${line:-1}" \ + "Missing license header error" \ + "$file is not licensed under 'Apache-2.0 OR ISC OR MIT'" success=false fi - done + done < <(git ls-files -z -- "*.[chsSi]") if $success; then info "Check SPDX + Copyright" diff --git a/scripts/tests b/scripts/tests index 14360b2cf5..aa65cb7193 100755 --- a/scripts/tests +++ b/scripts/tests @@ -33,6 +33,25 @@ def dict2str(dict): return s +# CI passes escaped quotes for configuration-file defines such as +# -DMLD_CONFIG_FILE=\"../test/configs/custom_heap_alloc_config.h\". +# Keep shell metacharacters such as ;, $, `, |, &, <, >, and newlines out. +SAFE_MAKE_FLAG_RE = re.compile(r'^[A-Za-z0-9_.,:+=/@% "\\-]*$') +SAFE_CROSS_PREFIX_RE = re.compile(r"^[A-Za-z0-9_./+-]*$") + + +def validate_cli_inputs(parser, args): + checks = [ + ("cflags", SAFE_MAKE_FLAG_RE), + ("ldflags", SAFE_MAKE_FLAG_RE), + ("cross_prefix", SAFE_CROSS_PREFIX_RE), + ] + for name, pattern in checks: + value = getattr(args, name, None) + if value is not None and not pattern.fullmatch(value): + parser.error(f"--{name.replace('_', '-')} contains unsupported characters") + + def github_log(msg): if os.environ.get("GITHUB_ENV") is None: return @@ -1603,6 +1622,7 @@ def cli(): ) args = main_parser.parse_args() + validate_cli_inputs(main_parser, args) if not hasattr(args, "mac_taskpolicy"): args.mac_taskpolicy = None