Repository navigation
Conversation
A cols/rows value that the PTY layer cannot represent raised out of the receive
loop (struct.error for anything outside an unsigned short, ValueError/TypeError
for strings and arrays, OverflowError for the Infinity literal json.loads
accepts). The only handler there is except WebSocketDisconnect, so the exception
reached the finally, which closes the master fd and SIGTERMs adb shell: one
malformed frame cost the user the whole terminal.
Validate the frame against the same bounds the handshake already enforces and
ignore it otherwise, so the previously applied size survives a bad frame.
The regression tests drive the handler with the PTY and subprocess layers
stubbed, which lets them run on a non-POSIX host; the set_winsize stub contains
the same struct.pack("HHHH", rows, cols, 0, 0) statement posix_compat runs in
production, so an unvalidated size fails the way it fails there.
Fixes TencentCloud#1475
Full non-live unit suite read-out (promised in the PR body)
All 23 are pre-existing, and the way this PR proves it is a set comparison rather than an assertion: Two known lanes, matching those node ids:
The 19 were re-read on this tree rather than carried over from the earlier run: selecting exactly those Neither lane shares a file with this PR, measured rather than argued: Targeted lanes on this branch, for completeness:
|
2 participants
What this fixes
One malformed
resizeframe cost the user the whole remote Android shell./api/mobile/adb/shell/wsvalidates the handshakecols/rows(ge=20, le=500/ge=5, le=200) butconverts and applies a runtime
resizeframe with no guard at all, and_set_winsizethere has notry— unlike the same-named helper inrouters/terminal.py, which logs "Errors are non-fatal". Thereceive loop's only handler is
except WebSocketDisconnect, so anything else escapes the handler, andthe
finallythen closes the master fd and SIGTERMs theadb shellprocess group. uvicorn answers1011; the terminal is gone.
This is #1317's request applied to the second PTY-over-WebSocket surface in the repo. #1318 implements
exactly this shape for
terminal.py; this PR mirrors that diff for the mobile route, and changesnothing in
terminal.py, which #1318 owns.Found from the shipped code, filed as #1475.
Changes
src/octop/api/routers/mobile/shell_ws.py_MIN_COLS/_MAX_COLS/_MIN_ROWS/_MAX_ROWS, reference them from theQuerydeclarations, and in theresizebranch drop a frame that fails to convert or falls outside those bounds — samecontinueshape as #1318tests/unit/mobile/test_shell_ws_resize.pyCHANGELOG.md### 修复bulletOverflowErroris in theexceptclause in addition to #1318's(TypeError, ValueError), becausejson.loadsaccepts the non-standardInfinityliteral andint(float("inf"))raises it. Mutation M3below shows that clause is load-bearing.
Red on
develop@a02e3e187 failed, 4 passed in 4.29s— the 4 passing cases are the frames that should be applied, so thesuite is not uniformly red by construction. Verbatim:
develop{"cols":70000,"rows":32}struct.error: 'H' format requires 0 <= number <= 65535{"cols":-1,"rows":32}struct.error: 'H' format requires 0 <= number <= 65535{"cols":"abc","rows":32}ValueError: invalid literal for int() with base 10: 'abc'{"cols":[1],"rows":32}TypeError: int() argument must be a string, a bytes-like object or a real number, not 'list'{"cols":Infinity,"rows":32}OverflowError: cannot convert float infinity to integer{"cols":NaN,"rows":32}ValueError: cannot convert float NaN to integer{"cols":100,"rows":1}then{"cols":100,"rows":9999}assert [(41, 120, 32), (41, 100, 1), (41, 100, 9999), (41, 80, 24)] == [(41, 120, 32), (41, 80, 24)]The last row is a second, quieter defect that the range check also closes:
1and9999fit in anunsigned short, so those frames do not kill the session, they resize the PTY to a geometry noterminal can use.
Note on
NaN:float('nan') or 120evaluates tonanbecauseNaNis truthy, so theorfallbackdoes not rescue it. Recorded on #1475.
Green
11 passed in 0.40sNeighbourhood lanes on this branch:
tests/unit/mobile tests/unit/api→366 passed, 10 skipped in 37.52s.Full non-live unit suite (
pytest tests/unit -m "not live" -n 4) is still running as this PR opens;its read-out, including how any failure relates to this diff, follows in a comment on this PR.
How each clause was shown to be necessary
Each mutation was applied to the fixed file, run, and reverted; the source was then confirmed
byte-identical to the measured-green copy (
sha1 61684fd9…):try/except4 failed—test_string_cols_is_ignored,test_array_cols_is_ignored,test_infinity_cols_is_ignored,test_nan_cols_is_ignored3 failed—test_oversized_cols_is_ignored_and_the_session_survives,test_negative_cols_is_ignored_and_the_previous_size_is_kept,test_out_of_range_rows_is_ignored(TypeError, ValueError)1 failed—test_infinity_cols_is_ignored_MAX_COLS = 500→1002 failed—test_resize_without_dimensions_falls_back_to_the_handshake_size,test_endpoints_of_the_handshake_range_are_still_acceptedNo test is a tautology: M1/M2/M3/M4 each kill a different subset, and the two clauses cover different
rows of the table above.
Gates
ruff check src tests→ All checks passedruff format --check src tests→ 1120 files already formattedmypy --strict src/octop→ Success: no issues found in 544 source filesWhat is not measured
The route refuses non-POSIX at
shell_ws.py:69and needs a realadb, a connected device and a realPTY, so this box cannot produce a live 1011 capture. The tests instead drive the handler with the PTY,
subprocess and auth layers stubbed, and the
set_winsizestub contains the samestruct.pack("HHHH", rows, cols, 0, 0)statement thatposix_compat.set_winsize:103runs inproduction — so the exception under test is the one production raises, while the teardown consequence
(the
finallykilling the process group) is read from the code, not observed.CI
These checks run
make install && make lint && make typecheck && make teston Linux and Windows.make testispytest -n $(PYTEST_JOBS) -m "not live", so it does cover the new test file on bothplatforms; the new tests are not platform-gated, unlike
terminal.py's@posix_onlyones.