fix: enhance task preparation and execution in run.sh for better priority handling#33940
fix: enhance task preparation and execution in run.sh for better priority handling#33940tomchon wants to merge 4 commits into
Conversation
Summary of ChangesHello @tomchon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a priority-based task handling system by splitting tasks into high-priority and normal-priority queues. The changes in prepare_cases correctly separate the tasks, and run_thread is updated to process them. My review identifies a few areas for improvement. I've suggested optimizing a grep pipeline for better performance. More critically, I've pointed out a major performance bottleneck where a single lock file serializes task fetching across all threads, undermining the goal of parallel execution. I've also raised a design consideration about dedicating only a single thread to high-priority tasks and suggested an alternative for better resource utilization.
There was a problem hiding this comment.
Pull request overview
This PR refactors the task preparation and execution logic in the parallel test runner to implement priority-based task handling. The changes split tasks into high-priority (prefixed with numbers) and normal-priority tasks, allowing specific threads to process high-priority tasks first.
Key changes:
- Modified task preparation to generate separate task files for high and normal priority tasks
- Updated task execution to allow threads on the first host (index=0) to process high-priority tasks before normal tasks
- Introduced separate lock files for coordinating access to each task queue
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flock -x "$lock_file" -c "echo -e \"${hosts[index]} ret:${ret} ${line}\n ${web_server}/$test_log_dir/${case_file}.txt\" >>${failed_case_file}" | ||
| else | ||
| flock -x "$lock_file" -c "echo -e \"${hosts[index]} ret:${ret} ${line}\n log file: ${case_log_file}\" >>${failed_case_file}" |
There was a problem hiding this comment.
The value of line read from the task file is interpolated directly into the shell snippet passed to flock -c (echo -e "... ${line} ..."), so any shell metacharacters, $() or backticks in the task file will be executed on the local host when a test fails. In this repository tdgpt_cases.task is provided by the repo and used in the GitHub Actions workflow, so a malicious PR author can gain code execution on the self‑hosted runner by embedding shell syntax in a task line that triggers the ret != 0 branch. To fix this, avoid passing untrusted task text into -c strings (write logs using the current shell without -c, or ensure the message is safely escaped/printf’d so it cannot be interpreted as shell code).
| flock -x "$lock_file" -c "echo -e \"${hosts[index]} ret:${ret} ${line}\n ${web_server}/$test_log_dir/${case_file}.txt\" >>${failed_case_file}" | |
| else | |
| flock -x "$lock_file" -c "echo -e \"${hosts[index]} ret:${ret} ${line}\n log file: ${case_log_file}\" >>${failed_case_file}" | |
| { | |
| flock -x "$lock_file" | |
| printf "%s ret:%s %s\n %s/%s/%s.txt\n" "${hosts[index]}" "$ret" "$line" "$web_server" "$test_log_dir" "$case_file" >>"${failed_case_file}" | |
| } | |
| else | |
| { | |
| flock -x "$lock_file" | |
| printf "%s ret:%s %s\n log file: %s\n" "${hosts[index]}" "$ret" "$line" "$case_log_file" >>"${failed_case_file}" | |
| } |
| if ! is_local_host "${hosts[index]}"; then | ||
| $cmd >>"$case_log_file" 2>&1 | ||
| else | ||
| bash -c "$cmd" >>"$case_log_file" 2>&1 |
There was a problem hiding this comment.
case_cmd from the task file is embedded into cmd and then executed either as $cmd or via bash -c "$cmd", relying only on surrounding quotes for safety; if a task’s caseCommand contains quotes or shell metacharacters (e.g. "; curl ... | bash;"), it can break out of the intended argument and execute arbitrary commands on the CI host before run_container.sh is invoked. Because tdgpt_cases.task is under version control and used in .github/workflows/tdgpt-test.yml, an attacker can craft a malicious task line in a PR to achieve remote code execution on the self‑hosted runner. This should be hardened by treating the remote invocation as an argument vector (no bash -c on a constructed string) and ensuring the -c payload is passed as a single argument without further shell interpretation of task‑controlled data.
| bash -c "$cmd" >>"$case_log_file" 2>&1 | |
| read -ra cmd_array <<< "$cmd" | |
| "${cmd_array[@]}" >>"$case_log_file" 2>&1 |
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.