Repository navigation
fix(tools): validate output directory in relinker_fuzz_seeds - #12406
pushkarreddyy wants to merge 5 commits into
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the exact head 73e9920c13. The focused regression suite passes locally (4/4), covering successful seed generation, an existing file, a directory-creation failure, and the CLI subprocess path. git diff --check and tools/check_conventions.py --base <current main> pass. Error reporting uses argparse exit code 2 without a traceback in the tested failures. Hosted checks are still pending, so I am not approving yet.
|
Proactively closing to avoid file collision with approved PR #12328. |
rooklz
left a comment
There was a problem hiding this comment.
Reviewed @73e9920c13 on macOS (relinker-only lane). AI-assisted review.
The fix works. On py3.9.6, 3.13 and 3.14 the head passes 4/4. With the old main source (only argv plumbing added so the tests can call it), all 4 new tests fail (2 FAIL, 2 ERROR), so the tests do exercise the change. Merged on main 750cf6c: build 270/270, conventions clean.
Blocker: CI never runs the new test. tools/tests/test_relinker_fuzz_seeds.py gets no add_test. python3 tools/check_test_registration.py --scripts build on the head prints:
Error: tools/tests/test_relinker_fuzz_seeds.py is a test script that no add_test or workflow runs
ctest -N | grep fuzz returns nothing.
Overlap. #12328 (PrivacyOnion, approved) adds the same file plus its add_test(NAME relinker_fuzz_seeds ...). pr_overlap reports an add/add conflict on tools/tests/test_relinker_fuzz_seeds.py. Whichever lands second has to merge its tests into the other's file. Listing #12328 under Depends on and adding these cases to its file after it merges would avoid the conflict. Or, if this one goes first, register it as below.
Mutants (on the source):
| mutant | result |
|---|---|
drop the is_file() check |
killed |
| drop the try around mkdir | killed |
| drop the selector prefix byte | killed |
| drop the try around the seed writes | survives: no test makes a write fail |
main() instead of sys.exit(main()) |
equivalent (rc 0 or parser.error exits 2 either way) |
This diff kills the write-try survivor (it fails with a traceback ERROR when that try is removed) and registers the test. With it: 5/5 OK on 3 Pythons, merged on main ctest 379/379 (relinker_fuzz_seeds 0.23 s), and check_test_registration --scripts gives "212 test script(s), each run by an add_test or a workflow".
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9ccab61e1b..4c5b4c4c94 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -274,6 +274,8 @@ if(BUILD_TESTING)
${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_debt_split.py)
add_test(NAME stubgen_stdout_newlines COMMAND ${Python3_EXECUTABLE}
${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_stubgen_stdout_newlines.py)
+ add_test(NAME relinker_fuzz_seeds COMMAND ${Python3_EXECUTABLE}
+ ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_relinker_fuzz_seeds.py)
add_test(NAME hardware_oracle COMMAND ${Python3_EXECUTABLE}
${CMAKE_CURRENT_SOURCE_DIR}/tools/hw-oracle/test_hw_oracle.py)
add_test(NAME hardware_oracle_compare COMMAND ${Python3_EXECUTABLE}
diff --git a/tools/tests/test_relinker_fuzz_seeds.py b/tools/tests/test_relinker_fuzz_seeds.py
index 342d1afd35..9565624f60 100644
--- a/tools/tests/test_relinker_fuzz_seeds.py
+++ b/tools/tests/test_relinker_fuzz_seeds.py
@@ -55,6 +55,16 @@ class RelinkerFuzzSeedsTests(unittest.TestCase):
self.assertNotIn("Traceback", stderr)
self.assertEqual(stdout, "")
+ def test_rejects_unwritable_seed_without_traceback(self):
+ with tempfile.TemporaryDirectory() as directory:
+ output = Path(directory) / "seeds"
+ name, _ = next(relinker_fuzz_seeds.images())
+ (output / f"{name}-{relinker_fuzz_seeds.SELECTORS[0]}").mkdir(parents=True)
+ code, stdout, stderr = self.run_main(output)
+ self.assertEqual(code, 2)
+ self.assertNotIn("Traceback", stderr)
+ self.assertEqual(stdout, "")
+
def test_cli_subprocess_invocation(self):
with tempfile.TemporaryDirectory() as directory:
existing = Path(directory) / "file.txt"|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@pushkarreddyy the tool change reads right to me.
|
|
@oneandonlydean Thanks for the review! Allowing #12328 to land first makes total sense. Once #12328 merges with the test suite and CMake registration in place, I will rebase and submit a clean follow-up adding the four error-handling test cases to its class. |
adriadam10
left a comment
There was a problem hiding this comment.
The change to tools/relinker_fuzz_seeds.py reads right: existing file and mkdir/write OSErrors now give a usage error, and the new tests fail on main without it. Two things block the merge:
tools/tests/test_relinker_fuzz_seeds.pyhas noadd_testin the rootCMakeLists.txt, so CI never runs it (python3 tools/check_test_registration.py --scripts buildreports it).- #12328 (open) creates the same file and registers it, so the two cannot both land. As you said in the thread, let #12328 merge first, then rebase and add these four cases to its test class (no new file, no second registration). Keep
main(argv=None)and the error handling as they are.
Also, the thread says this PR is being closed but it is still open; either close it or push the rebased version here.
|
@adriadam10 @oneandonlydean Closing this PR for now so it doesn't block the queue. As soon as #12328 lands on |
|
Reopening: registered |
adriadam10
left a comment
There was a problem hiding this comment.
The add_test registration in the root CMakeLists.txt is there now, thanks. The remaining blocker is the same file in #12328:
- #12328 (open, CI green) also creates
tools/tests/test_relinker_fuzz_seeds.pyand registers it, so both cannot land. Once #12328 is merged, rebase, drop this PR's copy of the test file and theadd_test, and add the four cases to its test class. Keepmain(argv=None)and the error handling intools/relinker_fuzz_seeds.pyas they are.
|
@adriadam10 Understood, thank you! I will keep this PR open as-is. Once #12328 lands on |
MarcZX10
left a comment
There was a problem hiding this comment.
The added CMake registration now makes the focused suite discoverable, and the exact-head tests pass 4/4. check_test_registration.py --scripts reports all 212 scripts registered; current-main conventions and git diff --check pass.
One repository checklist item still contradicts the agreed merge plan. The PR declares Depends on: none, but you and the reviewers have agreed that #12328 must land first, after which this PR will be rebased and its tests folded into #12328's registered test file. I fetched both current heads and git merge-tree --write-tree upstream/pr/12328 upstream/pr/12406 confirms an add/add conflict in tools/tests/test_relinker_fuzz_seeds.py. Please list #12328 on this PR's Depends on checklist line while it remains the prerequisite. I am requesting only that checklist correction; the registration and test fix itself now passes locally. The hosted macOS relinker jobs are still pending, so this is not an approval.
|
@MarcZX10 Updated! I have updated the checklist line to |
ab76879 to
5d4fe30
Compare
|
The directory check is right; the failure paths around it are not separated from it.
|
5d4fe30 to
d720e12
Compare
|
@ebosido444-ai Thanks for the observations!
Branch is rebased on latest |
adriadam10
left a comment
There was a problem hiding this comment.
The new nested-parent-directory test is fine, but the blocker is unchanged: #12328 is still open and creates the same tools/tests/test_relinker_fuzz_seeds.py plus its add_test, so both cannot land.
- Once #12328 is merged, rebase, drop this PR's copy of the test file and the
add_test, and add the five cases to its test class. Keepmain(argv=None)and the error handling intools/relinker_fuzz_seeds.pyas they are.
|
@adriadam10 Understood! |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
I reviewed exact head d720e12 against current main 3a2b919; the merge preview is clean, the previously missing dependency declaration is now present, and the five focused tests pass locally.
What I ran
Environment: Linux x86_64, kernel 6.1.0-54-amd64, Python 3.11.2. Tests ran in an isolated worktree at the exact PR head.
git diff --check upstream/main...refs/remotes/review/12406
python3 tools/check_conventions.py --base upstream/main --head refs/remotes/review/12406
git merge-tree --write-tree upstream/main refs/remotes/review/12406
python3 tools/tests/test_relinker_fuzz_seeds.pyI also temporarily removed the explicit existing-file check, reran the focused test file, and restored the implementation. The current PR description declares Depends on: #12328; that dependency remains open.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean |
| Diff check | Passed |
| Conventions | Passed |
| relinker_fuzz_seeds focused tests | 5 passed |
| Negative control without the existing-file check | 2 expected failures |
| Hosted Windows job | Passed |
| Hosted relinker Ubuntu and Windows jobs | Passed |
| Hosted relinker ASan | Passed |
| Hosted macOS relinker jobs | Pending |
| Hosted Linux and specialized jobs | Skipped |
Raw output
Merge preview:
51da2126da4e8ad9681ce841454d79eff8737887
Diff check:
(no output; exit status 0)
Convention checker:
All conventions checks passed.
Focused tests:
.....
----------------------------------------------------------------------
Ran 5 tests in 0.788s
OK
Negative control:
F.F..
======================================================================
FAIL: test_cli_subprocess_invocation (__main__.RelinkerFuzzSeedsTests.test_cli_subprocess_invocation)
...
AssertionError: 'is an existing file, not a directory' not found in "usage: relinker_fuzz_seeds.py [-h] output\nrelinker_fuzz_seeds.py: error: [Errno 17] File exists: '/tmp/tmp_19frr5z/file.txt'\n"
======================================================================
FAIL: test_rejects_existing_file_output (__main__.RelinkerFuzzSeedsTests.test_rejects_existing_file_output)
...
AssertionError: 'is an existing file, not a directory' not found in "usage: test_relinker_fuzz_seeds.py [-h] output\ntest_relinker_fuzz_seeds.py: error: [Errno 17] File exists: '/tmp/tmppzyn65w9/existing.bin'\n"
----------------------------------------------------------------------
Ran 5 tests in 0.284s
FAILED (failures=2)
Hosted checks:
conventions pass
labels pass
relinker (ubuntu-latest, g++) pass
relinker (windows-latest, g++) pass
relinker-asan pass
windows pass
linux skipping
register-context skipping
write-watch skipping
relinker (macos-latest, clang++) pending
relinker (macos-latest, clang++, x86_64) pending
eligibility pass
Negative control
| Assertion | Change | Result |
|---|---|---|
| Existing-file output is rejected with a clear non-traceback diagnostic | Temporarily removed the is_file guard | The in-process and CLI regression tests both failed on the expected diagnostic; the full test file was restored and passed 5/5 |
Findings
Blocking
None found on this head. The earlier dependency-checklist issue is resolved: the PR now declares #12328, which remains open.
Non-blocking
The hosted macOS relinker jobs are still pending. The hosted Windows workflow passed, and the local focused tests cover successful generation, nested directories, existing-file rejection, directory-creation errors, and CLI behavior.
Sources
- Output validation and error handling at the reviewed head
- Focused tests at the reviewed head
- CTest registration at the reviewed head
Not verified
I did not run the full project build, the macOS relinker jobs, or console behavior. The hosted Linux project job was skipped; this is a small Python tool change and its focused unit tests passed locally.
To get this to a good state
No code change is needed on the reviewed head. After the declared dependency #12328 lands, follow the described plan to integrate the test with that suite and rerun the focused tests on the resulting head; I will recheck that revision.
035783d to
c27f7f0
Compare
116ed9f to
06c3414
Compare
✅ AMD GPU test: passedPR head
Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (12, not counted against this PR)Validation IDs already present on main (8 IDs in 801 tests)Skipped (62)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Thanks for the review. Here's a summary of the fix in this PR: What it does
This PR:
Verification
Status
Ready for review whenever #12328 is merged. |
06c3414 to
fb791f6
Compare
|
Hi pushkarreddyy, Thanks for the summary and for the rebase onto the current The description lists Could you update the description so it states what is true now? If this PR no longer needs #12328, please untick or reword the One smaller point: the AMD GPU test comment from 14:05 UTC ran head |
WhatUpdated pull request dependencies and confirmed standalone verification. Context
StatusReady for review and queueing. |
|
Hi Thanks for the rebase. The new head On main the Python tool tests end with Putting the set_tests_properties(title_run PROPERTIES TIMEOUT 60)
add_test(NAME relinker_fuzz_seeds COMMAND ${Python3_EXECUTABLE}
${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_relinker_fuzz_seeds.py)
endif()
endif() |
WhatRestored the nested Fix
Status
|
|
Hi Thanks, that was quick. |
✅ AMD GPU test: passedPR head
Unstable on main (6, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (14, not counted against this PR)Passed on retry (2, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 811 tests)Skipped (62)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
439bdf2 to
a408190
Compare
|
Added est_rejects_unwritable_seed_without_traceback to ools/tests/test_relinker_fuzz_seeds.py to cover unwritable seed write rejection as suggested. All tests pass locally and conventions are clean. |
What
In
tools/relinker_fuzz_seeds.py,outputwas passed directly toPath.mkdir(parents=True, exist_ok=True). Ifoutputreferenced an existing regular file or uncreatable directory,mkdirraised an unhandledFileExistsErrororOSErrortraceback and exited with code 1.parser.error(f"{arguments.output} is an existing file, not a directory").OSErrorerror handling viaparser.error(str(error)).argvinmain(argv=None)to support testing.tools/tests/test_relinker_fuzz_seeds.py.relinker_fuzz_seedstest in rootCMakeLists.txtunderBUILD_TESTING.Tested
tools/tests/test_relinker_fuzz_seeds.pycovering seed creation, existing-file rejection, uncreatable directories, nested parent directory creation, and CLI subprocess invocation (5/5 passed).relinker_fuzz_seedstest registration viapython tools/check_test_registration.py --scripts build.python tools/check_conventions.py --base origin/mainwith 0 findings.Checklist