Skip to content

Conversation

boomanaiden154
Copy link
Contributor

There was one action dependency that was not hash pinned and this workflow also allowed code injection as the input might not be properly escaped when dumped into the run script.

There was one action dependency that was not hash pinned and this
workflow also allowed code injection as the input might not be properly
escaped when dumped into the run script.
@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. github:workflow labels Sep 14, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 14, 2025

@llvm/pr-subscribers-github-workflow

@llvm/pr-subscribers-libcxx

Author: Aiden Grossman (boomanaiden154)

Changes

There was one action dependency that was not hash pinned and this workflow also allowed code injection as the input might not be properly escaped when dumped into the run script.


Full diff: https://github.com/llvm/llvm-project/pull/158467.diff

1 Files Affected:

  • (modified) .github/workflows/libcxx-run-benchmarks.yml (+4-2)
diff --git a/.github/workflows/libcxx-run-benchmarks.yml b/.github/workflows/libcxx-run-benchmarks.yml
index 5714600b63a5e..17a97df029ba5 100644
--- a/.github/workflows/libcxx-run-benchmarks.yml
+++ b/.github/workflows/libcxx-run-benchmarks.yml
@@ -33,12 +33,14 @@ jobs:
 
     runs-on: llvm-premerge-libcxx-next-runners # TODO: This should run on a dedicated set of machines
     steps:
-      - uses: actions/setup-python@v6
+      - uses: actions/setup-python@e797f83bcb11b83ae66e0230d6156d7c80228e7c # v6.0.0
         with:
           python-version: '3.10'
 
       - name: Extract information from the PR
         id: vars
+        env:
+          COMMENT_BODY: ${{ github.event.comment.body }}
         run: |
           python3 -m venv .venv
           source .venv/bin/activate
@@ -51,7 +53,7 @@ jobs:
           print(f"pr_base={pr.base.sha}")
           print(f"pr_head={pr.head.sha}")
           EOF
-          BENCHMARKS=$(echo "${{ github.event.comment.body }}" | sed -nE 's/\/libcxx-bot benchmark (.+)/\1/p')
+          BENCHMARKS=$(echo "$COMMENT_BODY" | sed -nE 's/\/libcxx-bot benchmark (.+)/\1/p')
           echo "benchmarks=${BENCHMARKS}" >> ${GITHUB_OUTPUT}
 
       - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0

@philnik777
Copy link
Contributor

It looks like the libc++ CI didn't actually run. Could you make a dummy change to ensure that it runs correctly?

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot.

FWIW, I would have much much rather gone through a PR with this workflow, but getting it to work was a matter of 10s of trial and error. I even created my own dummy repo to try out the workflow but there are a few things that could only be "tested" on the real monorepo (without incurring a lot of overhead in the feedback loop).

So the result is that I basically committed to main directly, which is a shame. I might just be unaware of better options, but as far as I can tell there is no good way to develop such workflows besides shipping directly to main. If there's a better way, we could probably document it cause I would have loved to get everything ready and then request your review before enabling the workflow.

@ldionne
Copy link
Member

ldionne commented Sep 15, 2025

/libcxx-bot benchmark libcxx/test/benchmarks/hash.bench.cpp

Benchmark results:
Benchmark                              Baseline    Candidate    Difference    % Difference
-----------------------------------  ----------  -----------  ------------  --------------
BM_Hash/uint32_random_std_hash/1024      364.11       362.51         -1.60           -0.44
BM_Hash/uint32_top_std_hash/1024         364.06       363.23         -0.83           -0.23

@ldionne ldionne merged commit 1d27e66 into llvm:main Sep 15, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github:workflow libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants