Repository navigation
fix: probe the string.pack dialect before binding the byte helper fast path - #22
Closed
svc-finitelabs[bot] wants to merge 3 commits into
Closed
svc-finitelabs[bot] wants to merge 3 commits into
svc-finitelabs[bot] wants to merge 3 commits into
Conversation
…t path The eight byte helpers took their fast path whenever rawget(string, "pack") and rawget(string, "unpack") were truthy, and then called them with the Lua 5.3 contract. Control4's DriverWorks supplies both names from lpack instead: pack(fmt, ...) has no size-suffixed codes, and unpack(s, fmt, pos) is argument-swapped and returns position before value. Every helper therefore failed on a controller, in code paths where the manual fallback directly below would have been correct. It did not fail loudly everywhere. lpack reads a 5.3-shaped call's payload as its format string, and that parser stops at a NUL byte or when the data runs out rather than raising, so on a controller be_bytes_to_u32 returned 1 for 0, 42, 1000 and 65535 alike, silently. Sweeping the leading payload byte over 0-255 there, 22 values decode to a position instead of raising. _compat now probes the contract by value across every format string the library uses, plus a non-1 offset, and exports string_pack/string_unpack as nil unless all of them answer exactly. bit32 and bit64 bind from those rather than from string directly. run_tests.sh runs every module a second time with test/lpack_shim.lua installed, an lpack-shaped pair modelled on measurements from a controller. That pass asserts the probe rejected it before running the selftest, so a green result cannot come from an interpreter that never had string.pack at all, and on 5.3+ it overwrites a genuine string.pack so the fallback is exercised on hosts that would otherwise always take the fast path. Refs FL-20
The header claimed the shim/controller divergence in accepted format codes was unreachable "because this library emits I4 alone". That reasons about the format bitn passes, but the swapped lpack signature is precisely what puts the payload in the format slot, so the payload bytes are format codes and the divergence is reachable. A little-endian u32 whose low byte is one of A a c d f n p z is quiet on a controller and raises in the shim, about 3% of values; B inverts. Measured by sweeping the first payload byte over 0-255 through the shim under lua5.1 and luajit: 14 of 256 accepted against the controller's 22. The conclusion holds for a different reason, now stated: packs_like_lua53() compares returned values across every format the library emits including the offset case, so it fails closed whether the dialect raises or answers quietly, and run_tests.sh asserts _compat.string_pack came out nil, which neither behaviour can perturb. Comment only, no behaviour change. Suite stays 6/6.
b3df66b replaced a wrong reachability reason with another wrong one. It said the shim/controller divergence in accepted format codes is harmless because packs_like_lua53() compares returned values on every format including the offset case, so it fails closed either way. True in the abstract, but the probe never gets there. Counting calls at the shim boundary while bitn._compat loads, under lua5.1 and luajit, gives pack 1 and unpack 0. The probe declines at its first pack(">I4", ...) on the 4 and returns, so the unpack parser the divergence lives in is never entered. The short circuit is what holds, not the value comparisons, and only counting at the boundary could tell the two apart: both mechanisms predict a green suite. Also records what would make the divergence live, since that is the part a future editor needs: widening this shim's pack to accept size suffixes puts the probe into the unpack parser. Comment only, no behaviour change. Suite stays 6/6.
Contributor
|
Superseded by #23, which carries the same probe plus the lpack test pass and is approved; closing so nothing re-vendors from this branch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
bit32andbit64's eight byte helpers took a fast path wheneverrawget(string, "pack")/rawget(string, "unpack")were truthy, then called themwith the Lua 5.3 contract. Control4's DriverWorks supplies both names from lpack,
a different API:
pack(fmt, ...)has no size-suffixed codes, andunpack(s, fmt, pos)is argument-swapped and returns position before value.Measured on the dev controller (LuaJIT 2.1,
_VERSIONLua 5.1, both functionspresent):
string.pack(">I4", n)string.unpack(">I4", "\1\2\3\4", 1)string.unpack(">I4", <BE bytes of 0>, 1)1string.unpack(">I4", <BE bytes of 42>, 1)1string.unpack(">I4", <BE bytes of 65535>, 1)1The decoders do not reliably raise. lpack reads the payload of a 5.3-shaped call
as its format string, and that parser stops at a NUL byte or when the data runs out
instead of erroring. Sweeping the leading payload byte over 0-255 on the controller,
22 values return a position rather than raising, and
0x00is one of them. Since(string_unpack(...))keeps only the first return value, and lpack returns positionfirst, the helper hands back a small integer. Every big-endian
u32below 2^24decodes to
1.Encoders are uniformly loud; only the decode side is silent.
Fix
_compatprobes the contract rather than presence, by value, across all four formatstrings this library uses plus a non-
1offset, and exports_compat.string_pack/_compat.string_unpackas nil unless every check answersexactly.
bit32/bit64bind from those instead of fromstringdirectly, so afailed probe falls through to the manual byte math that was always sitting below it.
This follows the backend selection already in
_compat: probe behaviour, not version.Test mode
run_tests.shruns every module a second time withtest/lpack_shim.luainstalledbefore the require, an lpack-shaped pair modelled on the controller measurements
above. The pass asserts
_compat.string_packcame out nil before running theselftest, so a green result cannot come from an interpreter that never had
string.pack. On 5.3+ the shim overwrites a genuinestring.pack, so the fallbackgets exercised on hosts that would otherwise always take the fast path.
Needs no CI change: the matrix already runs
make test-allon 5.1, 5.2, 5.3, 5.4,LuaJIT 2.0 and 2.1, so the new mode runs on all six.
Verification
false.The fallback then round-trips 0, 1, 42, 1000, 65535, 16909060, 1789071852 and
4294967295 with 0 mismatches, and
be_bytes_to_u32of 0 / 42 / 65535 returns0 / 42 / 65535 instead of 1 / 1 / 1.
src/at330c855: bit32 and bit64 fail on both LuaJIT and 5.5, bit16 stays green (it hasno byte helpers). So the test fails for the reason it claims.
_compat.string_packis non-nil on stock Lua 5.5and nil on LuaJIT 2.1, Lua 5.1, and on all three under the shim. The fast path is
still taken where it is genuinely available.
case this library can reach: pack raises, the
0x01payload raises, and 0 / 42 /1000 / 65535 all return 1. It accepts 14 of 256 leading bytes against the
controller's 22; the sets differ both ways on codes the library never emits, which
is documented in the file.
bit backends (native operators, bit library, pure Lua).
amalghas no Lua 5.5 rock somake buildcould not run here.A hand-built
package.preloadbundle of the same shape loads and passes on allthree interpreters, with the fast path bound on 5.5 and declined elsewhere. The CI
build job runs the real thing on 5.4.
make format-checkandmake typecheckclean (luals 3.19.0, the pinned version).make lintwill not run on this host: luacheck 1.2.0 does not load under Lua 5.5,and it fails identically at
330c855, so it is the box and not this change.Not in this PR
The re-vendor chain.
v0.6.3is vendored in lua-crypto, lua-bthome-ble, lua-protobuf,lua-noiseprotocol and control4-driver-template, and the affected helpers are reached
by the released
lua-crypto v0.2.3,lua-bthome-ble v0.1.6andlua-protobuf v0.6.8.Tagging
v0.6.4and re-vendoring bottom-up is a separate call and is yours to make.Refs FL-20