Repository navigation
fix: bind string.pack only when it speaks the 5.3 dialect - #23
Conversation
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.
|
@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. |
There was a problem hiding this comment.
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, somaingenuinely takes the fast path and breaks. The bug is real, not inferred.- This PR's probe, run verbatim:
pack("<I4", 0x04030201)raises, sopcallcatches and_compat.string_packstaysnil. That is the operative decline and it happens at the digit4, 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 returnspos=2, val=nil, so thevalue ~= 0x04030201check 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 secondInever runs. No raise.unpack("\1\2", "II")returnspos=1, val=nilwhen 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.
|
Took 1, 2 and the nit in 3; 4 is done (#22 closed, branch deleted).
|
There was a problem hiding this comment.
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-checkclean,make typecheckreports no problems.make lintdies inside luacheck 1.2.0's ownrequireunder 5.5. Checked out330c855and it fails identically there, so that is my box and not this branch. CI'sCheckis green.make buildwantsamalg, which I do not have locally.Build Combined Moduleis green in CI.- All eight checks
successat2cc62ec, re-resolved frompulls/23at review time, not taken from the webhook. - Delta since the head I last reviewed (
d126390) is exactlyrun_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.
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 thestring.pack/string.unpackfast path wheneverrawget(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 nostring.pack, 5.3+ has the real one._compat.luaprobes once at load:pack("<I4", 0x04030201)must give01 02 03 04andunpack("<I4", "\0\1\2\3\4", 2)must give(0x04030201, 6), each underpcall; otherwisestring_pack/string_unpackstayniland the byte math runs.bit32andbit64bind from_compatinstead ofrawget.test/lpack_stub.luainstalls the lpack shape (the onec4_shimdocuments as verified on a controller), andrun_tests.shgains anlpackmodule that loads it first, asserts the probe declined it, and runs all three selftests. It is in the default set, somake testand every CI matrix leg run it.Verified:
make check;run_tests.sh allon 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 newlpackmodule againstmain's guard fails withunsupported 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'sc4_shimall 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.