Skip to content

fix: bind string.pack only when it speaks the 5.3 dialect - #23

Merged
derek-miller merged 2 commits into
mainfrom
fix/probe-string-pack-dialect
Sep 11, 2026
Merged

derek-miller merged 2 commits into
mainfrom
fix/probe-string-pack-dialect

Conversation

@derek-miller

Copy link
Copy Markdown
Contributor

Fixes FL-20, found by the bot on control4-driver-template#58 and confirmed on the dev controller.

bit32/bit64's eight byte helpers took the string.pack/string.unpack fast path whenever rawget(string, "pack") was truthy, then called it with the 5.3 contract. A Control4 controller has both functions as lpack: no size suffixes, unpack(data, fmt, pos) returning the position first. string.pack("<I4", 1) raises there, so every helper raised exactly where the manual fallback would have been right. No stock interpreter shows it: LuaJIT and 5.1 have no string.pack, 5.3+ has the real one.

  • _compat.lua probes once at load: pack("<I4", 0x04030201) must give 01 02 03 04 and unpack("<I4", "\0\1\2\3\4", 2) must give (0x04030201, 6), each under pcall; otherwise string_pack/string_unpack stay nil and the byte math runs. bit32 and bit64 bind from _compat instead of rawget.
  • test/lpack_stub.lua installs the lpack shape (the one c4_shim documents as verified on a controller), and run_tests.sh gains an lpack module that loads it first, asserts the probe declined it, and runs all three selftests. It is in the default set, so make test and every CI matrix leg run it.

Verified: make check; run_tests.sh all on 5.1, 5.4 and LuaJIT 2.1 (the probe binds only on 5.4, where it is the real 5.3 dialect, so that leg still exercises the fast path); the new lpack module against main's guard fails with unsupported code '4' on 5.1 and LuaJIT, so it sees the defect. With this build first on the path, the rendered crypto v0.2.3, bthome v0.1.6, protobuf v0.6.8 and noiseprotocol v0.6.5 amalgams that raised under the template's c4_shim all pass their selftests there.

Tagging v0.6.4 on merge, then re-vendoring bottom-up (crypto, bthome, protobuf, noiseprotocol, template). The hardware end-to-end goes on the template PR once the new amalgams exist.

The byte helpers took their string.pack/unpack fast path whenever the functions existed. Control4's LuaJIT has both as lpack, which has no size suffixes and returns the position first from unpack, so every helper raised on a controller where the manual path would have been right. _compat now probes both functions once at load and binds them only if the answers match; the runner gains an lpack pass that installs that shape before every selftest. Refs FL-20.
@derek-miller

Copy link
Copy Markdown
Contributor Author

@svc-finitelabs review requested: FL-20 fix, 8/8 green. This is the head of the re-release chain that unblocks control4-driver-template#58, so it is the one to look at first.

svc-finitelabs[bot]
svc-finitelabs Bot previously approved these changes Sep 11, 2026

@svc-finitelabs svc-finitelabs Bot 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.

Approving. The diagnosis is right, the probe fails closed for the right reason, and I confirmed both the defect and the fix on the dev controller rather than through a double. Findings below are non-blocking, but item 1 is cheap and I would take it before the tag.

Verified

On hardware (dev controller 192.168.2.92, director 4.2.1.757028-res, LuaJIT 2.1.1700206165, device 587, today):

  • rawget(string, "pack") is truthy there, so main genuinely takes the fast path and breaks. The bug is real, not inferred.
  • This PR's probe, run verbatim: pack("<I4", 0x04030201) raises, so pcall catches and _compat.string_pack stays nil. That is the operative decline and it happens at the digit 4, before the unpack half is ever reached.
  • The unpack half also fails closed independently, for anyone wondering whether the ordering matters: under lpack's (data, fmt, pos) signature the call returns pos=2, val=nil, so the value ~= 0x04030201 check rejects it too.

Locally: run_tests.sh all is 4/4 on 5.1, LuaJIT 2.1 and Lua 5.5 (sampled a second number model deliberately, since this repo's 64-bit paths have bitten on exactly that axis before). format-check and typecheck are clean. lint dies inside luacheck 1.2.0's own require under 5.5 identically at the base commit, so that is my box, and CI's Check is green. 8/8 green at d126390.

Discrimination, re-run rather than taken from the body: grafted this PR's run_tests.sh and test/lpack_stub.lua onto the unfixed 330c855 source. The lpack module fails on 5.1, LuaJIT and 5.5. The new test does see the defect.

1. The suite cannot tell a working probe from a dead one

Mutation: replace probe_string_pack()'s result with nil, nil so the fast path never binds. All four modules still pass on 5.1, LuaJIT and 5.5, where the probe otherwise binds for real. Nothing goes red.

The new lpack module asserts the probe declined. Nothing anywhere asserts it binds on a genuine 5.3 interpreter. So if a later change to the probe fails closed by accident on real 5.3, the byte helpers silently fall back to the manual path, #21's speedup quietly reverts, and CI stays green through all eight legs. Negative assertion, no positive counterpart.

One line closes it: in the non-lpack legs, when rawget(string, "pack") exists, require _compat.string_pack ~= nil. That makes the 5.3 and 5.4 legs prove the thing they are the only legs able to prove.

2. The stub is wrong about the hardware it claims to model

Both axes measured on the controller above. Neither weakens this fix, because the probe declines at the raise on 4 before either path is reachable. I am raising it because test/lpack_stub.lua becomes the repo's standing answer to "what does lpack actually do", and it is currently wrong twice.

Endianness. The header says "'<' accepted and ignored" and the code ignores <, > and =. The controller honours all three, on both sides, at every width:

pack <I -> 04 03 02 01     pack >I -> 01 02 03 04     pack =I -> 04 03 02 01
pack <H -> 04 03           pack >H -> 03 04
unpack <I -> val=67305985  unpack >I -> val=16909060

Default is little-endian, = is little, > flips. The measurement is self-validating in the measured quantity: <I and >I return byte-reversed answers on the same input, so the instrument is not reading a constant.

This matters past the test file. It is what decides whether be_bytes_to_u32 and le_bytes_to_u32 are even distinguishable on a controller. They are, and the stub says they are not.

Loud where the hardware is quiet. The stub error()s on every unknown code. The controller does raise on the digit, which is the case this PR depends on, but it is silent in two places the stub is not:

  • pack("I\0I", 1, 2) returns length 4, 01 00 00 00. The NUL terminates the format and the second I never runs. No raise.
  • unpack("\1\2", "II") returns pos=1, val=nil when the data runs out. No raise.

The stub raises on the first and throws arithmetic-on-nil at line 22 on the second. That inverts the observable for anything downstream that ever probes those two behaviours, which is the failure mode where a double is worse than no double.

3. The probe validates one of the four formats the helpers emit (nit)

The helpers emit >I4, <I4, >I4I4 and <I4I4 (bit32.lua:236,252,276,295, bit64.lua:278,298,325,346). The probe checks <I4. Real lpack raises on the digit in all four, so this is not a live gap, only thinner defence against a dialect nobody has measured yet.

4. #22 is still open with the same fix

agent/FL-20-pack-dialect-probe, head e6bee93, zero comments, green. Two PRs racing for the same v0.6.4 tag. Worth closing as superseded when this lands, so the branch does not get re-vendored from by mistake.

Merge and tag

Not merging or tagging. Checked rather than assumed: release.yml here is tag-gated (on: push: tags: v*) and build.yml does no tagging, so merging this does not cut v0.6.4 by itself. The tag and the bottom-up re-vendor order stay yours.

A probe module in the runner fails when string.pack is present but unbound, so a probe that fails closed on a real 5.3 interpreter cannot silently revert the fast path. The stub now honours '<', '=' and '>', stops at a NUL in the format, and returns nil for a value the data cannot supply, all as measured on a controller. The probe also checks the big-endian pack.
@derek-miller

Copy link
Copy Markdown
Contributor Author

Took 1, 2 and the nit in 3; 4 is done (#22 closed, branch deleted).

  • 1: new probe module in run_tests.sh, in the default set: when rawget(string, "pack") exists, _compat.string_pack must be bound. Prints string.pack bound on 5.4 and no string.pack on this interpreter on 5.1/LuaJIT. Mutation-checked: with the probe forced to nil, nil, the 5.4 leg fails with present but the probe declined it.
  • 2: test/lpack_stub.lua now honours </=/> on both sides, stops the format at a NUL, and returns nil for a value the data cannot supply; checked against every row in your table (<I/>I/=I/<H/>H pack, <I/>I unpack, I\0I length 4, short-data 1, nil), and <I4 still raises so the probe still declines.
  • 3: the probe also checks pack(">I4", 0x04030201) == "\4\3\2\1".

make check clean; run_tests.sh all 5/5 on 5.1, 5.4 and LuaJIT. The push dismissed the approval; please re-approve and I will merge and tag v0.6.4.

@svc-finitelabs svc-finitelabs Bot 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.

Re-approving. All four items are addressed, and I re-derived each one against 2cc62ec rather than taking the summary.

The positive assertion actually discriminates (1)

Ran the mutation myself rather than trusting the report. Forced _compat.string_pack, _compat.string_unpack = nil, nil, then run_tests.sh all:

interpreter probe leg says result
Lua 5.5 string.pack is present but the probe declined it 4/5, red
Lua 5.1 no string.pack on this interpreter 5/5, green
LuaJIT 2.1 no string.pack on this interpreter 5/5, green

That is the right shape: load-bearing on exactly the legs able to carry it, vacuous by construction elsewhere, and the comment above it says so out loud. Restored the file and confirmed a clean tree before continuing.

The stub matches every row I measured (2)

Asserted the fifteen observations from the controller session against test/lpack_stub.lua in a script, rather than re-reading the diff and agreeing with myself:

pack <I -> 04 03 02 01    pack >I -> 01 02 03 04    pack =I -> 04 03 02 01
pack  I -> 04 03 02 01    pack <H -> 04 03          pack >H -> 03 04
unpack <I -> pos=5 val=67305985   unpack >I -> pos=5 val=16909060
pack("I\0I",1,2) -> len 4, 01 00 00 00
unpack("\1\2","II") -> pos=1, val=nil
pack("<I4",...) raises

All fifteen match, on 5.1, LuaJIT 2.1 and 5.5. Endianness is honoured on both sides at both widths, the NUL ends the format quietly, and short data yields nil instead of an arithmetic error. The stub is now a correct standing answer to "what does lpack do", which was the part that mattered past this fix.

Nit 3 and item 4

>I4 is in the probe. Recording that nothing in the matrix exercises it: real 5.3 answers correctly, and real lpack raises at the digit in <I4 before reaching it. It is defence against an unmeasured dialect that accepts I4 yet ignores endian markers, which is what I asked for.

#22 is closed, merged=false, and agent/FL-20-pack-dialect-probe is gone from the remote. Confirmed both.

Suite, checks, scope

  • run_tests.sh all: 5/5 on Lua 5.1, LuaJIT 2.1 and Lua 5.5. Second number model sampled deliberately, since this repo's 64-bit paths have bitten on that axis before.
  • make format-check clean, make typecheck reports no problems.
  • make lint dies inside luacheck 1.2.0's own require under 5.5. Checked out 330c855 and it fails identically there, so that is my box and not this branch. CI's Check is green.
  • make build wants amalg, which I do not have locally. Build Combined Module is green in CI.
  • All eight checks success at 2cc62ec, re-resolved from pulls/23 at review time, not taken from the webhook.
  • Delta since the head I last reviewed (d126390) is exactly run_tests.sh, src/bitn/_compat.lua, test/lpack_stub.lua. No drive-by changes rode along.

Two notes, neither a request

On 5.3+ the stub returns floats from unpack, because it builds values with 2^(8*k). That diverges from real 5.3 string.unpack, which returns integers, but it is modelling lpack on a controller where every number is a double, so the float is the more faithful answer. Leaving it; noting it so nobody "corrects" it later.

The usage header in run_tests.sh still reads Available modules: bit16, bit32, bit64, now two behind the actual list. Stale before this PR, cheap to fix whenever.

Merging and tagging stay yours.

@derek-miller
derek-miller merged commit 85f6799 into main Sep 11, 2026
8 checks passed
@derek-miller
derek-miller deleted the fix/probe-string-pack-dialect branch September 11, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant