Repository navigation
fix(AutoCheckinBigGod): 任务退出时清理所属 Frida 进程及子进程 - #1951
huiyuhang1 wants to merge 1 commit into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嘿——我发现了 4 个问题
给 AI 代理的提示
请处理这次代码审查中的评论：
## 单条评论
### 评论 1
<location path="tasks/AutoCheckinBigGod/script_task.py" line_range="184" />
<code_context>
+ # The Windows CLI launcher may have a Python child process.
+ # Target only this task's Popen PID and its descendants.
+ subprocess.run(
+ ['taskkill', '/PID', str(proc.pid), '/T', '/F'],
+ capture_output=True, timeout=5, creationflags=_NO_WINDOW)
+ if proc.poll() is None:
</code_context>
<issue_to_address>
**清理过程会运行攻击者控制的可执行文件**
当任务在 Windows 上运行且 Frida 进程仍在运行时，如果攻击者能够将 `taskkill.exe` 放入一个搜索顺序早于系统工具所在目录的目录，`_stop_frida_process` 会按可执行文件名解析 `taskkill`，因此 Windows 可能会运行攻击者放置在较早搜索位置的 `taskkill.exe`，而不是系统工具；攻击者的代码将以该任务的权限运行。
请从可信的 Windows 系统目录解析 `taskkill.exe`，或者使用不会按名称搜索可执行文件的进程树终止机制。
</issue_to_address>
### 评论 2
<location path="tasks/AutoCheckinBigGod/script_task.py" line_range="55" />
<code_context>
+ try:
+ self._run_checkin()
+ finally:
+ self._cleanup(restore_game=self._restore_game_after_checkin)
+ session = getattr(self, 'session', None)
+ if session is not None:
</code_context>
<issue_to_address>
**其他 Frida 客户端会失去服务器**
当此任务退出时，如果另一个客户端正在同一 ADB 设备上使用已运行的 Frida 服务器，`_cleanup` 会执行 `killall frida-server`，却不检查该服务器是否由此任务启动，从而断开另一个客户端并导致其 Frida 操作失败。
只有在此任务启动了 Frida 服务器时，才停止该服务器。
</issue_to_address>
### 评论 3
<location path="tasks/AutoCheckinBigGod/script_task.py" line_range="220" />
<code_context>
+ if not getattr(self, '_adb_connected', False):
+ return
try:
self._adb_shell(['am', 'force-stop', GL_PACKAGE])
except Exception:
</code_context>
<issue_to_address>
**已打开的游戏会被关闭**
当 ADB 已连接，但 Frida 服务器设置在此任务启动应用之前失败，并且 BigGod 原本就在运行时，`_cleanup` 会在 `_restore_game_after_checkin` 为 false 的情况下对 `GL_PACKAGE` 执行强制停止，导致此任务未启动的应用也被关闭。
只有在此任务启动了应用时才强制停止它，并保留或恢复原本已在运行的应用状态。
另请参阅 `tasks/AutoCheckinBigGod/script_task.py:55`。
</issue_to_address>
### 评论 4
<location path="tests/test_biggod_frida_cleanup.py" line_range="66-67" />
<code_context>
+
+def test_failed_adb_check_does_not_touch_unspecified_device():
+ task = object.__new__(ScriptTask)
+ task._run_checkin = Mock(side_effect=TaskEnd('AutoCheckinBigGod'))
+ task._adb_shell = Mock()
+ with pytest.raises(TaskEnd):
+ task.run()
</code_context>
<issue_to_address>
**失败的 ADB 检查被绕过**
当实际的 `_run_checkin` 在因连接检查失败而抛出异常之前访问 ADB 时，`test_failed_adb_check_does_not_touch_unspecified_device` 将 `_run_checkin` 替换为一个立即抛出异常的 mock。因此，即使实际的 ADB 失败路径在抛出异常前调用了 `_adb_shell` 并访问了未指定的设备，该测试仍会通过。
请测试真实的连接失败路径，并断言该路径不会发出 ADB 命令。
</issue_to_address>Sourcery 评估
等待批准。 请先处理 4 个问题。
阻塞性问题：tasks/AutoCheckinBigGod/script_task.py:184、tasks/AutoCheckinBigGod/script_task.py:55、tasks/AutoCheckinBigGod/script_task.py:220、tests/test_biggod_frida_cleanup.py:67
Original comment in English
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tasks/AutoCheckinBigGod/script_task.py" line_range="184" />
<code_context>
+ # The Windows CLI launcher may have a Python child process.
+ # Target only this task's Popen PID and its descendants.
+ subprocess.run(
+ ['taskkill', '/PID', str(proc.pid), '/T', '/F'],
+ capture_output=True, timeout=5, creationflags=_NO_WINDOW)
+ if proc.poll() is None:
</code_context>
<issue_to_address>
**Cleanup runs attacker-controlled executable**
When the task runs on Windows with a live Frida process and an attacker can place `taskkill.exe` in a directory searched before the system utility, `_stop_frida_process` resolves `taskkill` by executable name, so Windows can run an attacker-planted `taskkill.exe` from an earlier search location instead of the system utility; the attacker’s code runs with the task’s privileges.
Resolve `taskkill.exe` from the trusted Windows system directory, or use a process-tree termination mechanism that does not search for an executable by name.
</issue_to_address>
### Comment 2
<location path="tasks/AutoCheckinBigGod/script_task.py" line_range="55" />
<code_context>
+ try:
+ self._run_checkin()
+ finally:
+ self._cleanup(restore_game=self._restore_game_after_checkin)
+ session = getattr(self, 'session', None)
+ if session is not None:
</code_context>
<issue_to_address>
**Other Frida clients lose their server**
When another client uses a Frida server already running on the same ADB device when this task exits, `_cleanup` runs `killall frida-server` without checking whether this task started the server, disconnecting the other client and causing its Frida operations to fail.
Only stop the Frida server when this task started it.
</issue_to_address>
### Comment 3
<location path="tasks/AutoCheckinBigGod/script_task.py" line_range="220" />
<code_context>
+ if not getattr(self, '_adb_connected', False):
+ return
try:
self._adb_shell(['am', 'force-stop', GL_PACKAGE])
except Exception:
</code_context>
<issue_to_address>
**An already-open game is closed**
When ADB connects, Frida server setup fails before this task launches the app, and BigGod was already running, `_cleanup` force-stops `GL_PACKAGE` while `_restore_game_after_checkin` is false, leaving the app this task did not launch closed.
Only force-stop the app if this task launched it, and preserve or restore the state of an app that was already running.
Also at `tasks/AutoCheckinBigGod/script_task.py:55`.
</issue_to_address>
### Comment 4
<location path="tests/test_biggod_frida_cleanup.py" line_range="66-67" />
<code_context>
+
+def test_failed_adb_check_does_not_touch_unspecified_device():
+ task = object.__new__(ScriptTask)
+ task._run_checkin = Mock(side_effect=TaskEnd('AutoCheckinBigGod'))
+ task._adb_shell = Mock()
+ with pytest.raises(TaskEnd):
+ task.run()
</code_context>
<issue_to_address>
**Failed ADB check is bypassed**
When the actual `_run_checkin` touches ADB before raising on a failed connection check, `test_failed_adb_check_does_not_touch_unspecified_device` replaces `_run_checkin` with a mock that raises immediately, so the test passes even if the real failed-ADB path calls `_adb_shell` before raising and touches an unspecified device.
Exercise the real failed-connection path and assert that it does not issue ADB commands.
</issue_to_address>Sourcery assessment
Approval pending. 4 findings to address first.
Blocking findings: tasks/AutoCheckinBigGod/script_task.py:184, tasks/AutoCheckinBigGod/script_task.py:55, tasks/AutoCheckinBigGod/script_task.py:220, tests/test_biggod_frida_cleanup.py:67
| # The Windows CLI launcher may have a Python child process. | ||
| # Target only this task's Popen PID and its descendants. | ||
| subprocess.run( | ||
| ['taskkill', '/PID', str(proc.pid), '/T', '/F'], |
There was a problem hiding this comment.
🟠 High · 清理过程会运行攻击者控制的可执行文件
当任务在 Windows 上运行且 Frida 进程仍在运行时，如果攻击者能够将 taskkill.exe 放入一个搜索顺序早于系统工具所在目录的目录，_stop_frida_process 会按可执行文件名解析 taskkill，因此 Windows 可能会运行攻击者放置在较早搜索位置的 taskkill.exe，而不是系统工具；攻击者的代码将以该任务的权限运行。
请从可信的 Windows 系统目录解析 taskkill.exe，或者使用不会按名称搜索可执行文件的进程树终止机制。
给 AI 代理的提示
在 `tasks/AutoCheckinBigGod/script_task.py` 第 184 行：
**清理过程会运行攻击者控制的可执行文件**
当任务在 Windows 上运行且 Frida 进程仍在运行时，如果攻击者能够将 `taskkill.exe` 放入一个搜索顺序早于系统工具所在目录的目录，`_stop_frida_process` 会按可执行文件名解析 `taskkill`，因此 Windows 可能会运行攻击者放置在较早搜索位置的 `taskkill.exe`，而不是系统工具；攻击者的代码将以该任务的权限运行。
请从可信的 Windows 系统目录解析 `taskkill.exe`，或者使用不会按名称搜索可执行文件的进程树终止机制。Original comment in English
🟠 High · Cleanup runs attacker-controlled executable
When the task runs on Windows with a live Frida process and an attacker can place taskkill.exe in a directory searched before the system utility, _stop_frida_process resolves taskkill by executable name, so Windows can run an attacker-planted taskkill.exe from an earlier search location instead of the system utility; the attacker’s code runs with the task’s privileges.
Resolve taskkill.exe from the trusted Windows system directory, or use a process-tree termination mechanism that does not search for an executable by name.
Prompt for AI agents
In `tasks/AutoCheckinBigGod/script_task.py` at line 184:
**Cleanup runs attacker-controlled executable**
When the task runs on Windows with a live Frida process and an attacker can place `taskkill.exe` in a directory searched before the system utility, `_stop_frida_process` resolves `taskkill` by executable name, so Windows can run an attacker-planted `taskkill.exe` from an earlier search location instead of the system utility; the attacker’s code runs with the task’s privileges.
Resolve `taskkill.exe` from the trusted Windows system directory, or use a process-tree termination mechanism that does not search for an executable by name.| try: | ||
| self._run_checkin() | ||
| finally: | ||
| self._cleanup(restore_game=self._restore_game_after_checkin) |
There was a problem hiding this comment.
🟡 Medium · 其他 Frida 客户端会失去服务器
当此任务退出时，如果另一个客户端正在同一 ADB 设备上使用已运行的 Frida 服务器，_cleanup 会执行 killall frida-server，却不检查该服务器是否由此任务启动，从而断开另一个客户端并导致其 Frida 操作失败。
只有在此任务启动了 Frida 服务器时，才停止该服务器。
给 AI 代理的提示
在 `tasks/AutoCheckinBigGod/script_task.py` 第 55 行：
**其他 Frida 客户端会失去服务器**
当此任务退出时，如果另一个客户端正在同一 ADB 设备上使用已运行的 Frida 服务器，`_cleanup` 会执行 `killall frida-server`，却不检查该服务器是否由此任务启动，从而断开另一个客户端并导致其 Frida 操作失败。
只有在此任务启动了 Frida 服务器时，才停止该服务器。Original comment in English
🟡 Medium · Other Frida clients lose their server
When another client uses a Frida server already running on the same ADB device when this task exits, _cleanup runs killall frida-server without checking whether this task started the server, disconnecting the other client and causing its Frida operations to fail.
Only stop the Frida server when this task started it.
Prompt for AI agents
In `tasks/AutoCheckinBigGod/script_task.py` at line 55:
**Other Frida clients lose their server**
When another client uses a Frida server already running on the same ADB device when this task exits, `_cleanup` runs `killall frida-server` without checking whether this task started the server, disconnecting the other client and causing its Frida operations to fail.
Only stop the Frida server when this task started it.| if not getattr(self, '_adb_connected', False): | ||
| return | ||
| try: | ||
| self._adb_shell(['am', 'force-stop', GL_PACKAGE]) |
There was a problem hiding this comment.
🟡 Medium · 已打开的游戏会被关闭
当 ADB 已连接，但 Frida 服务器设置在此任务启动应用之前失败，并且 BigGod 原本就在运行时，_cleanup 会在 _restore_game_after_checkin 为 false 的情况下对 GL_PACKAGE 执行强制停止，导致此任务未启动的应用也被关闭。
只有在此任务启动了应用时才强制停止它，并保留或恢复原本已在运行的应用状态。
另请参阅 tasks/AutoCheckinBigGod/script_task.py:55。
给 AI 代理的提示
在 `tasks/AutoCheckinBigGod/script_task.py` 第 220 行：
**已打开的游戏会被关闭**
当 ADB 已连接，但 Frida 服务器设置在此任务启动应用之前失败，并且 BigGod 原本就在运行时，`_cleanup` 会在 `_restore_game_after_checkin` 为 false 的情况下对 `GL_PACKAGE` 执行强制停止，导致此任务未启动的应用也被关闭。
只有在此任务启动了应用时才强制停止它，并保留或恢复原本已在运行的应用状态。
另请参阅 `tasks/AutoCheckinBigGod/script_task.py:55`。Original comment in English
🟡 Medium · An already-open game is closed
When ADB connects, Frida server setup fails before this task launches the app, and BigGod was already running, _cleanup force-stops GL_PACKAGE while _restore_game_after_checkin is false, leaving the app this task did not launch closed.
Only force-stop the app if this task launched it, and preserve or restore the state of an app that was already running.
Also at tasks/AutoCheckinBigGod/script_task.py:55.
Prompt for AI agents
In `tasks/AutoCheckinBigGod/script_task.py` at line 220:
**An already-open game is closed**
When ADB connects, Frida server setup fails before this task launches the app, and BigGod was already running, `_cleanup` force-stops `GL_PACKAGE` while `_restore_game_after_checkin` is false, leaving the app this task did not launch closed.
Only force-stop the app if this task launched it, and preserve or restore the state of an app that was already running.
Also at `tasks/AutoCheckinBigGod/script_task.py:55`.| task._run_checkin = Mock(side_effect=TaskEnd('AutoCheckinBigGod')) | ||
| task._adb_shell = Mock() |
There was a problem hiding this comment.
🟡 Medium · 失败的 ADB 检查被绕过
当实际的 _run_checkin 在因连接检查失败而抛出异常之前访问 ADB 时，test_failed_adb_check_does_not_touch_unspecified_device 将 _run_checkin 替换为一个立即抛出异常的 mock。因此，即使实际的 ADB 失败路径在抛出异常前调用了 _adb_shell 并访问了未指定的设备，该测试仍会通过。
请测试真实的连接失败路径，并断言该路径不会发出 ADB 命令。
给 AI 代理的提示
在 `tests/test_biggod_frida_cleanup.py` 第 66-67 行：
**失败的 ADB 检查被绕过**
当实际的 `_run_checkin` 在因连接检查失败而抛出异常之前访问 ADB 时，`test_failed_adb_check_does_not_touch_unspecified_device` 将 `_run_checkin` 替换为一个立即抛出异常的 mock。因此，即使实际的 ADB 失败路径在抛出异常前调用了 `_adb_shell` 并访问了未指定的设备，该测试仍会通过。
请测试真实的连接失败路径，并断言该路径不会发出 ADB 命令。Original comment in English
🟡 Medium · Failed ADB check is bypassed
When the actual _run_checkin touches ADB before raising on a failed connection check, test_failed_adb_check_does_not_touch_unspecified_device replaces _run_checkin with a mock that raises immediately, so the test passes even if the real failed-ADB path calls _adb_shell before raising and touches an unspecified device.
Exercise the real failed-connection path and assert that it does not issue ADB commands.
Prompt for AI agents
In `tests/test_biggod_frida_cleanup.py` at lines 66-67:
**Failed ADB check is bypassed**
When the actual `_run_checkin` touches ADB before raising on a failed connection check, `test_failed_adb_check_does_not_touch_unspecified_device` replaces `_run_checkin` with a mock that raises immediately, so the test passes even if the real failed-ADB path calls `_adb_shell` before raising and touches an unspecified device.
Exercise the real failed-connection path and assert that it does not issue ADB commands.
Findings
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
1 participant
frida不清理的话，会被腾讯的安全检测识别成作弊工具。。。
因为不涉及游戏流程，使用gpt修复并且测试了一下，没有问题。
Sourcery 摘要
确保 AutoCheckinBigGod 在任务的所有退出路径中，都会终止其拥有的 Frida 进程并释放资源。
Bug Fixes:
Enhancements:
Tests:
Original summary in English
Summary by Sourcery
Guarantee that AutoCheckinBigGod terminates its owned Frida processes and releases resources on every task exit path.
Bug Fixes:
Enhancements:
Tests: