Skip to content

fix(tools): ignore invalid timelines when advancing pad sequences - #10578

Merged
1 commit merged into
boykopovar:mainfrom
xTRACKGOD:fix/pad-invalid-timeline-sequence
Nov 2, 2026
Merged

1 commit merged into
boykopovar:mainfrom
xTRACKGOD:fix/pad-invalid-timeline-sequence

Conversation

@xTRACKGOD

@xTRACKGOD xTRACKGOD commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

What

pad_input.py and pad_timeline.py trust an existing timeline's header sequence without reading its events. A truncated APS5_PAD_TIMELINE 1 18446744073709551615 1 file blocks both tools with "controller command sequence is exhausted", although InputCommandFile rejects that timeline and will accept a subsequent sequence-1 command.

Validate the declared events and trailing data before retaining a timeline's sequence. Match the native reader's count, line length, ordering, duration, button-mask and text-format rules using bounded reads. Malformed timelines can be replaced; valid timelines still advance the sequence, and a valid maximum sequence still reports exhaustion without modifying the file. Pulse/stick parsing keeps its behavior.

Tested

Windows x86-64, Python 3.12.14, CMake 4.3.1, Ninja 1.13.2, WinLibs GCC 15.2.0; baseline main e512cd51e892625139101f1042129b6c8eef1c36:

  • Tests first: 25 new malformed-body subcases fail against unchanged main. An independent three-test baseline reproduction also fails for an ordinary high sequence and both real CLI exhaustion cases; all three pass with this fix.
  • cmake -S . -B build-relinker -G Ninja -DCMAKE_BUILD_TYPE=Release -DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON and cmake --build build-relinker --target pad_command_file_tests --parallel 2 succeed.
  • ctest --test-dir build-relinker -R '^(pad_command_file|pad_input|pad_timeline|test_script_registration)$' --output-on-failure -V: 4/4 passed in 3.03 seconds. The two Python suites ran 42 tests: 39 passed and 3 POSIX-only tests skipped. The existing hardlink alias-protection test ran successfully outside sandbox restrictions. All 175 Python test scripts are registered.
  • Independent 31-case comparison against the actual unchanged C++ InputCommandFile reader and both CLIs passes. Additional native differential checks cover 105 lexical cases, the Windows NUL/Ctrl-Z header case and nine pulse/stick/touch controls, with no mismatches.
  • Regressions cover missing/extra/malformed rows, unsigned bounds, invalid masks, zero/oversized duration, overlap/order, one-hour limits, trailing data, 127/128-byte line boundaries, final EOF, CRLF, native numeric adjacency, NUL, Windows text EOF, 4,096 events, valid sequence advancement and genuine exhaustion/file preservation.
  • python tools/check_conventions.py --base origin/main, PR title/body validation and git diff --check origin/main...HEAD pass.
  • tools/pr_overlap.py --branch fix/pad-invalid-timeline-sequence checked 1,268 open PRs. The sole base-conflicted comparison, fix(tools): normalize pad input output encoding and validate target path #6531 (head be8b84b), was reviewed with paired exact-blob merges. It changes exception handling and CLI output/path validation, without validating timeline bodies. This branch adds one mechanical conflict beside its expanded exception tuple; its pre-existing main() conflict is unchanged, and the shared test file merges cleanly in either comparison. The behaviors can coexist by retaining this parser and its expanded exception tuple; whichever lands second needs that small reconciliation. fix(tools): wrap entrypoint in main with stream reconfigure in pad_input #10206 and fix(tools): decode timeline JSON with utf-8-sig in pad_timeline #9258 change separate CLI encoding/entry-point behavior. No dependency. The repository checker ran unchanged through a local adapter supplying public GitHub pagination because gh is unavailable.

No title, console hardware, GPU or full system-library build was tested. Hosted relinker checks for this head pass on Linux, Windows and macOS (arm64/x86_64), together with ASan; full runtime builds were skipped by eligibility.

Native timeline compatibility

Fresh Windows x86-64 / GCC 15.2 checks used the unchanged native header from both main 8ec6397b37 and this PR. All 13 cases agree with the Python reader, including the exact embedded-NUL example raised in review.

The native reader constructs std::string(line) before stream parsing in both the header path and event path. That construction ends at the first NUL; the remaining bytes do not reach istringstream.

Input within a timeline Native result Python prior sequence
0 100 16384\0-1\n Accepted; buttons 16384 2000
Header with a complete valid prefix, then NUL and junk Accepted 2000
Extra token without NUL, or incomplete prefix before NUL Rejected 0
NUL after the declared event rows, or overlong event line Rejected 0

A constructor-latching check also confirms that an accepted NUL timeline suppresses a subsequent sequence-1 pulse, whereas rejected controls allow it. Rejecting those otherwise valid NUL prefixes only in Python would reset their sequence despite native acceptance.

Minimal native reproduction (prints 16384):

#include "core/libs/prx/libScePad/include/InputCommandFile.hpp"
#include <fstream>
#include <iostream>
int main(int argc, char** argv) {
    if (argc != 2) return 2;
    const std::filesystem::path path(argv[1]);
    { std::ofstream file(path); file << "1 0 1\n"; }
    Pad::InputCommandFile input(path);
    constexpr char text[] = "APS5_PAD_TIMELINE 1 2000 1\n0 100 16384\0-1\n";
    { std::ofstream file(path); file.write(text, sizeof(text) - 1); }
    std::cout << input.Buttons(0) << '\n';
}

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The timeline reader currently accepts embedded NUL bytes as if they ended a record, but the native InputCommandFile does not. For example, the new “valid” test case 0 100 16384\0-1\n is accepted here because timeline_sequence splits the line at NUL; the native reader sees the remaining \0-1 as an extra token and rejects the command. The same mismatch applies to a header with a valid prefix followed by NUL and trailing data, since this code truncates the header before parsing while the native stream parser rejects the extra token. Please reject embedded NULs in the header and each event line (or otherwise match the native parser), and change the valid-boundary test to an input accepted by InputCommandFile. The two focused Python suites pass locally (42 tests); this is a parser-compatibility gap not covered by them.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 9258 tools/tests/test_pad_timeline.py
PR 10206 tools/pad_input.py
tools/tests/test_pad_input.py
PR 11360 tools/pad_input.py
tools/tests/test_pad_input.py
PR 11885 tools/pad_input.py
tools/tests/test_pad_input.py
PR 14457 tools/pad_input.py
tools/tests/test_pad_input.py
tools/tests/test_pad_timeline.py

@roboterspinne roboterspinne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed head f036afe merged onto upstream/main befe264 (2026-10-24); the merge is clean, check_conventions.py passes and pr_overlap.py lists only #6531, which conflicts with main itself and could not be compared (it touches pad_input.py and its test for a different topic, output encoding and target path, so whichever lands second rebases).

Environment: Windows 11 Home 10.0.26200, Python 3.13.9 (Anaconda); for the native comparison below the unchanged core/libs/prx/libScePad/include/InputCommandFile.hpp from this tree was compiled into a small console probe with WinLibs GCC 15.2.0 UCRT (-std=c++20 -static); no relinker build needed.

Results

  • python -m unittest tools.tests.test_pad_input tools.tests.test_pad_timeline on the merged tree: 42 tests, 39 pass, 3 skipped (the pre-existing POSIX-only ones), 2 s. Red/green: against main's pad_input.py thirty subcases fail (25 in test_malformed_timeline_bodies_do_not_exhaust_sequence, 4 in test_invalid_existing_timeline_can_be_replaced_after_sequence_exhaustion, 1 in test_timeline_header_numeric_adjacency_preserves_sequence); all pass on the branch.
  • Differential against the runtime reader: the same files were fed to the native InputCommandFile::read (text-mode std::ifstream, as at runtime) and to pad_input.previous_sequence. Over 6,059 hand-written and mutated cases plus 28,000 generated timeline files (1,036 accepted natively, among them 21 with NUL, 15 with Ctrl-Z and 375 with CRLF) the branch agrees with the native result on every file; main disagrees on 1,134 of the first 6,059. A second, independent harness with 10,113 inputs agrees as well, and so does its rerun with the header compiled in binary mode and os.name forced to posix, standing in for Linux (2,524 accepted, 0 mismatches). Pulse files: 0 differences between main and the branch over 3,004 inputs, as the body says.
  • The boundaries agree one by one: 127 stored bytes per line with NUL counting toward the limit (getline(line, 128) against readline(129) plus rstrip(b"\n"), checked at 122 and 123 spaces with LF, CRLF and EOF endings), count 1..4096, duration 1..5000, start + duration > 3600000 (the native start > 3600000 - duration), the mask 0x70fffe = 0x10fffe | TouchLeftBit | TouchRightBit (lines 95 and 137-138), a leading + and the 1+2000+1 adjacency (num_get stops at +, which (?=\+) mirrors) with ++2, + 2, 2+ and 0x2 rejected on both sides, 2^64-1 accepted and 2^64 rejected on both (overflow sets failbit), trailing content rejected on both (file >> std::ws; peek(), lines 104-105) with \v, \f, \r, \t accepted and \x85, \xa0 rejected, also past 9,000 bytes of whitespace.
  • On the open change request: the native reader does cut each line at the first NUL. getline stores the bytes, but const std::string text(line) (line 73) and eventText(line) (line 89) are built from the char* and stop at the first NUL, so the - check, the istringstream and the >> extra check never see what follows. 0 100 16384\0-1\n is accepted natively with buttons 16384 (the probe confirms it, sequence 2000 as the test expects), and so is a valid header followed by NUL and junk. The case on line 98 of the test is right, and rejecting embedded NULs in Python would create the mismatch this PR removes: 0 for a file the runtime has latched, then a sequence at or below the latched one, which Poll ignores silently (line 40). The body's "Native timeline compatibility" section says the same, but it was added by editing the description after the review, and the review thread has no reply, so @MarcZX10 may not have seen it.

Nits

  • pad_input.py:36 (also :21, :23, :28, :45): five checks match the native parser but no test pins them. A mutation run on Windows passes the full suite with the seek after Ctrl-Z deleted, the header limit at 128, the upper sequence bound removed, the mask widened to 0x7ffffe, or the header NUL split deleted (that last one is covered on POSIX by line 118). Cases that kill them, each checked against the probe: (1, b"0 100 65536\n") and a header sequence 18446744073709551616 in the malformed list; a 128-byte header; on Windows APS5_PAD_TIMELINE 1 2000 2\n0 100 16384\x1a\n100 100 64\n (native 0, the seek mutant 2000); and APS5_PAD_TIMELINE 1 2000 1\0junk\n0 100 16384\n, which the native reader accepts, so it covers the header NUL split on every OS.
  • Body, "Tests first": it names only the 25 malformed-body subcases. Against unchanged main 30 new subcases fail here on Windows (the 25, plus the 4 and 1 listed above), and with POSIX semantics the four host-text-mode subcases (lines 110, 117, 122) fail too, about 34; that figure comes from forcing os.name, not from a Linux run. The full per-platform count would make the claim checkable.

Notes, nothing to change here: the plain pulse path (previous_sequence, lines 66-78) still neither cuts at NUL nor treats Ctrl-Z as end of file on Windows, while the native reader does both (line 73 and the pulse branch from line 108), so 5 64 300\0junk latches 5 natively and gives 0 here; pre-existing and untouched by this PR, low impact because next_sequence takes the max with the clock, a follow-up candidate. One consequence of returning 0 for an invalid timeline: main returned header+1 for any timeline header, so a runtime that had latched a sequence above the monotonic clock (a file carried over from an earlier boot, or hand-written) kept being exceeded even after the file turned invalid; now the next command gets monotonic_ns, below the latch, and is ignored silently. That is the trade-off the fix is about and fine by me, just worth knowing. The walrus in line 48 fits the 3.9 floor in BUILD.md:5 and is already used in build_web.py and pr_overlap.py.

Not checked: Linux and macOS runs (the POSIX branch was checked only against a binary-mode build of the header with os.name forced; libstdc++ and libc++ text mode were not run), the runtime itself (only the header compiled into a probe), the body's CI runs.

I found no blocking issue, and I could not reproduce the embedded-NUL mismatch from the change request: with the unchanged runtime header compiled into a probe, the branch agrees with the native reader on every generated file, NUL cases included, where main disagreed on about a fifth of them. The five unpinned checks are coverage, not behaviour. For disclosure: I have open PRs (#10667 on tools/tests/test_suggest_patches.py, and relinker PRs #10679, #10658, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390); none touches the pad tools.

Written with AI assistance (Claude Code). I ran the tests and the native probe above on my Windows 11 machine.

b"+0 +5000 +7405566\n5000 1 0\n3599999 1 16384\n \t\r\n\v\f",
b" " * 118 + b"0 1 16384\n1 1 0\n2 1 0",
b"0+100+16384\n100 100 64\n200 1 0\n",
b"0 100 16384\x00-1\n100 100 64\n200 1 0\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checked against the unchanged InputCommandFile.hpp compiled into a probe (WinLibs g++ 15.2): the runtime reads this line with getline(line, 128) but builds std::string eventText(line) from the char*, which stops at the first NUL, so the - check, the istringstream and the >> extra check never see -1. It accepts the file with buttons 16384 and sequence 2000, as this case expects, and the same holds for a valid header followed by NUL and junk. Rejecting embedded NULs here would make Python return 0 for a file the runtime has latched. The description's "Native timeline compatibility" section says the same; a reply in the review thread would make sure @MarcZX10 sees it, since editing the description sends no notification.

Comment thread tools/pad_input.py
if os.name == "nt":
if b"\x1a" in line:
line = line.split(b"\x1a", 1)[0]
command.seek(0, os.SEEK_END)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this seek and four other checks match the native parser but no test pins them; a mutation run on Windows passes the full suite with this line deleted, the header limit in line 21 at 128, the upper sequence bound in line 28 removed, the mask in line 45 widened to 0x7ffffe, or the header NUL split in line 23 deleted (that one is covered on POSIX by test line 118). Cases that kill them, each checked against the probe: (1, b"0 100 65536\n") and a header sequence 18446744073709551616 in the malformed list; a 128-byte header; on Windows APS5_PAD_TIMELINE 1 2000 2\n0 100 16384\x1a\n100 100 64\n (native 0, the seek mutant 2000); and APS5_PAD_TIMELINE 1 2000 1\0junk\n0 100 16384\n, which the runtime accepts, so it covers the header split on every OS.

Comment thread tools/pad_input.py
return 0
if os.name == "nt" and line.endswith(b"\r\n"):
line = line[:-2] + b"\n"
tokens = line.split()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note, nothing to change here: the pulse path still neither cuts at NUL nor treats Ctrl-Z as end of file on Windows, while the runtime does both (InputCommandFile.hpp:73 and the pulse branch from line 108), so 5 64 300\0junk latches 5 natively and gives 0 here. Pre-existing and untouched by this PR (0 differences to main over 3,004 pulse inputs), low impact because next_sequence takes the max with the clock; a follow-up candidate.

@THEMANnt THEMANnt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed f036afe0c92399c9c7972d5145f37f9dc1c8e6c3 against e512cd51e892625139101f1042129b6c8eef1c36. No actionable findings in this focused review.

Source review: Inspected the complete patch and the native InputCommandFile reader. Timeline sequencing now validates header, event count, button mask, duration, ordering, total duration and trailing content before trusting an existing sequence. Reviewed the intended correspondence with bounded native lines, NUL termination, adjacent signed-positive numeric fields and Windows CRLF/text EOF handling. Valid exhausted sequences still refuse replacement; malformed timelines can be replaced.

Validation: Windows Python 3.11: test_pad_input.py 20 passed, 2 skipped; test_pad_timeline.py 19 passed, 1 skipped (42 total). Baseline: 12+17 passed, 3 skipped. Grafted baseline controls produced one expected sequence assertion failure and 29 expected sequence-exhaustion SystemExit subcase errors across both suites. These demonstrate the malformed-body and numeric-adjacency behavior fixed by head. Conventions, whitespace and script registration passed. Effective hosted relinker matrix, ASan and conventions succeeded; full-product jobs were skipped.

Scope: Python command generation and source comparison with the native parser. POSIX permission checks were skipped; native controller execution and game input were not run locally.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Maintainer edits are off on this branch, so we couldn't push the update here. We carried it as #14457 with your commit and credit, merged with current main, and we'll close this one once that merges. Thank you!

@oneandonlydean oneandonlydean closed this pull request by merging all changes into boykopovar:main in 31887b4 Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs rebase Conflicts with main, rebase needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants