Reject malformed hexadecimal in machine:readmem, machine:writemem and machine:debugreg

[JC-000]

What's wrong

/v1/machine:writemem and /v1/machine:readmem parse the address query parameter with

int address = strtol(args["address"], NULL, 16);

The end pointer is passed as NULL and never checked, so strtol() silently yields 0 for input
that is not a number at all. 0 then passes the (address < 0) || (address > 65535) range check,
and the request proceeds against $0000.

For writemem that means a destructive write to zero page, answered with HTTP 200. Measured on
an Ultimate 64 Elite (fw 3.15):

readmem  GET   address=ZZZZ           -> HTTP 200 b'\xa1'                     (the byte at $0000)
writemem PUT   address=ZZZZ  data=00  -> HTTP 200 { "address" : "0000-0..." }
writemem POST  address=ZZZZ  +body    -> HTTP 200 { "address" : "0000-0..." }

The firmware names $0000 in its own success payload — it reports success for a write the caller
never asked for.

Also #885

PUT /v1/machine:debugreg had the same defect in its value parameter, found
while reviewing this PR and filed as #885. It is fixed here because it is the
same parser: ZZ, 0xZZ and -0 wrote 00, 1G wrote 01, and 1FF was
truncated to FF, each answered HTTP 200 with the register changed.

Scope

Reaching this requires sending an address that is not hexadecimal, which a careful client will not do
deliberately — but a typo, an unvalidated string, or a programmatically built query does it easily, and
the failure is silent: the caller is told 200 while zero page is overwritten. The argument for the
change is the API contract rather than a report of damage in the field: an unparseable address should
be refused, not resolved to $0000 and acted on.

The change

One parser, used by the three handlers that take an address — PUT writemem,
POST writemem, GET readmem. It implements the grammar the API documents,
"Start address in hexadecimal, 0000 to FFFF", and nothing else, built on the
chartohex helper already at the top of the file:

static bool parse_address(const char *text, int &address)
{
    if ((text == NULL) || (*text == '\0')) {
        return false;
    }
    int value = 0;
    for (const char *p = text; *p; p++) {
        uint8_t digit = chartohex(*p);
        if ((digit == 0xff) || (value > 0x0FFF)) {
            return false;
        }
        value = (value << 4) | digit;
    }
    address = value;
    return true;
}

machine:debugreg uses the same parser with a limit of 0xFF, so an
out-of-range byte is refused rather than truncated. Its API doc gains the
400 Invalid value response it now returns, and doc/api/rest_api_openapi_u64.yaml
is regenerated to match — make openapi_check is a build gate, so the
committed document cannot drift from the source.

Each handler is then:

int address;
if (!parse_address(args["address"], address)) {
    resp->error("Invalid address");
    resp->json_response(HTTP_BAD_REQUEST);
    return;
}

Notes on the shape:

• 400 Invalid address is already the documented response at all three
  handlers, so the API docs and the generated OpenAPI are unchanged.
• The range check is inside the loop, so an over-long address is refused as it
  is read and no errno or overflow reasoning is needed.
• This is stricter than strtol in four ways, all of them the documented
  grammar: leading whitespace, a leading sign, a 0x prefix and trailing
  characters are all refused. " D020", "+D020", "-0", "0xD020" and
  "D020 " are 400s.
• The 0x prefix is the one form that used to be accepted and now is not. No
  client in this repository sends it: the harness formats %04X, and the
  firmware's own web UI uses toString(16).padStart(4, "0") and literal
  four-digit strings, with its typed-address path validating as a 16-bit hex
  number first.

Test

New bad-address stage in tests/e2e/api/readmem_writemem_test.py, gated through the existing
FIXES table (memory-api-rejects-invalid-address, lacking=(C64U, U2)) so machines whose
released firmware predates the fix skip with a named reason instead of going red.

BAD_ADDRESSES covers ZZZZ, 0xZZZZ and gggg (nothing parses), -0 and
+1 (a sign, which strtol takes), and 12GG (a valid prefix with trailing
garbage).

A second stage, bad-debugreg, covers #885 with ZZ, 0xZZ, -0, 1G and
1FF. machine:debugreg has a GET as well as a PUT, so "the refused request
wrote nothing" is asserted directly: the register is read before and after each
rejected write and has to be unchanged. It is gated on its own fix entry,
debugreg-rejects-invalid-value, and skips where the route does not exist,
since both debugreg routes sit inside #if U64.

The stage sends a valid data= on the PUT case and a real body on the POST case deliberately:
without them the request is refused earlier — 400 for a missing required parameter, 412 for a
missing body — and the check would pass for a reason unrelated to the address. The HTTP status is
the assertion; $0000 is the 6510 processor port rather than plain RAM, so reading it back is not a
reliable witness.

Rig proof

Both halves on the same Ultimate 64 Elite, by JTAG-deploying a build of the
parent commit and then a build of this branch.

Before, every malformed address is accepted on both endpoints:

'ZZZZ'    readmem HTTP 200  writemem HTTP 200
'0xZZZZ'  readmem HTTP 200  writemem HTTP 200
'gggg'    readmem HTTP 200  writemem HTTP 200
'-0'      readmem HTTP 200  writemem HTTP 200
'+1'      readmem HTTP 200  writemem HTTP 200
'12GG'    readmem HTTP 200  writemem HTTP 200

and the stage fails on the first check:

[03] readmem rejects address='ZZZZ' ... FAIL (returned HTTP 200, expected 400: b'\x00')

machine:debugreg before, with the register set to 1F first:

'ZZ'     HTTP 200  wrote -> register now '00'
'0xZZ'   HTTP 200  wrote -> register now '00'
'-0'     HTTP 200  wrote -> register now '00'
'1G'     HTTP 200  wrote -> register now '01'
'1FF'    HTTP 200  wrote -> register now 'FF'

After, the stages are green and so is the rest of the suite:

readmem_writemem_test: OK (20 checks, 6.7s)     --test bad-address, 18/18
readmem_writemem_test: OK (8 checks, 6.4s)      --test bad-debugreg, 6/6
readmem_writemem_test: OK (78 checks, 23.3s)    --test all, all eight stages

bounds still passes, so the numeric out-of-range cases (10000, -1) are
unaffected: both were 400 before and are 400 now, refused at the parse rather
than by the range check.

Wider run, ./run-tests --profile quick u64: 25 of 26 suite runs pass. The one
failure is rest-api-coverage check 10, git_commit_hash is not a commit hash: '', with 78 of its 79 checks passing. That is an artefact of building in a git
worktree — target/common/rules.mk runs git rev-parse --short HEAD inside the
build container, where a worktree's .git pointer file resolves to a path that
is not mounted, so APP_VERSION_HASH comes out empty. It is independent of this
change and does not occur for a CI build.

On u2@c64u, with the Ultimate II+L flashed from this branch's CI artifact
(git_commit_hash 38033b87) and the fix table assumption lifted so the stage
runs rather than skipping:

readmem_writemem_test: OK (20 checks, 8.1s)     --test bad-address, 18/18
readmem_writemem_test: OK (38 checks, 17.0s)    --test all

The RISC-V build rejects the same six addresses on all three endpoints, so the
parser behaves identically on both CPUs. bad-debugreg skips there, because
both debugreg routes are inside #if U64 and a cartridge does not serve them.

tests/lib/openapi_contract_test.py and tests/e2e/api/openapi_contract_test.py
both pass against the rebuilt document and the running device.

Compiles for u64 and u64ii; route_machine.o also builds and links for u2pl's
RISC-V target.

Not covered

The same unchecked strtol(..., NULL, ...) pattern exists elsewhere — route_machine.cc's debugreg
value, routes.h's get_int (benign for length, whose range check catches 0), and
route_configs.cc / json.cc. Left alone here to keep this change to the address parse; happy to
follow up separately if wanted.

---

🤖 Generated with Claude Code

[chrisgleissner]

Thank you for putting this together. The focused regression coverage and the care around reaching each handler’s address parse are especially useful.

While auditing related handlers I found the same unchecked hexadecimal conversion in U64-only PUT /v1/machine:debugreg, tracked in #885. It can write 00 for value=ZZZZ and truncates oversized values. If you would be keen to include that here too, we’d appreciate it.

It may also be worth sharing the parsing/validation logic rather than adding another copy.

[chrisgleissner]

Pushed a commit addressing both review comments. Revert it if you disagree with
any of it.

Sign and whitespace. strtol skips leading whitespace and takes an optional
sign, so -0, +1 and 1234 all parsed and reached an address. The parse is
now parse_address, which accepts hex digits and nothing else, built on the
chartohex helper already at the top of the file. The range check moved into
the loop (value > 0x0FFF before each shift), so an over-long address is
refused as it is read.

De-duplication. One helper, and the three endpoints are five lines each.

BAD_ADDRESSES gains -0, +1 and 12GG.

Verified on an Ultimate 64 Elite by JTAG-deploying both states:

• Parent commit: all six addresses answer HTTP 200 on readmem and on
  writemem, and the stage fails on the first.
• With this commit: 18/18 on --test bad-address, and 72/72 on --test all.

Compiles for u64 and u64ii; route_machine.o builds and links for u2pl's
RISC-V target too (that build only stops later, at the updater packaging step,
on a bitstream this checkout does not have).

[chrisgleissner]

Also folded in #885, since it is the same parser. parse_hex now takes a limit,
so machine:debugreg uses it with 0xFF and refuses an out-of-range byte
rather than truncating it. Its API doc gains the 400 Invalid value it now
returns, and doc/api/rest_api_openapi_u64.yaml is regenerated — openapi_check
is a build gate, so the committed document cannot drift.

Measured on an Ultimate 64 Elite with the register set to 1F first, before the
fix:

'ZZ'   HTTP 200 -> register '00'     '1G'   HTTP 200 -> register '01'
'0xZZ' HTTP 200 -> register '00'     '1FF'  HTTP 200 -> register 'FF'
'-0'   HTTP 200 -> register '00'

After, all five are 400 and the register is unchanged. machine:debugreg has a
GET as well as a PUT, so the new bad-debugreg stage asserts that directly
rather than inferring it from the status code.

Rerun on both machines: u64 78/78 across all eight stages, u2@c64u 38/38 (the
debugreg stage skips there, both routes being inside #if U64). Both OpenAPI
suites pass.

I also retitled the PR, since it now covers three endpoints rather than two.
Revert any of this if you would rather keep #885 separate.