Repository navigation
fix: size the SIOCGIFMTU buffer for a whole struct ifreq - #64
Conversation
|
One note on the 3.14 leg, recorded because it is new coverage rather than new code: the first run of this PR failed on The failure was the first assertion, "the follower announced ready", with |
Python 3.14 put a guard byte after the argument fcntl.ioctl() is handed and raises SystemError when the kernel writes past it. SIOCGIFMTU writes back a whole struct ifreq — 40 bytes on 64-bit Linux — so the 18-byte buffer _iface_mtu() passed tripped the guard: connect() died with "buffer overflow" on every real gripper, while the simulated paths never reach _iface_mtu() and stayed green. Pad the buffer to sizeof(struct ifreq), and slice the mtu back out of the front of the reply — the reply is as long as the argument on every version. The suite could not have caught this, because 3.10 and 3.12 accept the short buffer silently, so the unit tests now run on 3.14 as well. Closes #63
442c712 to
ad605f6
Compare
|
🎉 This PR is included in version 1.0.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
connect()failed on Python 3.14 against every real gripper._iface_mtu()handedfcntl.ioctl()an 18-byte buffer forSIOCGIFMTU, while the kernel writes back awhole
struct ifreq(40 bytes on 64-bit Linux). Since cpythonc2eaeee3theioctl()wrapper puts a guard byte after the argument and raisesSystemError: buffer overflowwhen that write lands past it — aSystemError, not anOSError,so it escaped
_iface_mtu()'sexcept OSErrorand killed the connect. Thesimulated backends never call
_iface_mtu(), and 3.13 and earlier accepted theshort buffer silently, which is why a self-check passed while real hardware could
not be reached.
No breaking change: a private helper, same signature, same return, and a missing MTU
already meant "fall back to the requested mode". Upstream's callers see nothing but
a connect that works again on 3.14.
Closes #63.
Changes
src/litegrip/can/transport.py— pad theifreqto_IFREQ_SIZE(40 bytes,sizeof(struct ifreq)on 64-bit Linux, 32 on 32-bit) before the ioctl, and readthe mtu out of the first 18 bytes of the reply. The reply is exactly as long as the
argument on every version, so it has to be sliced back down.
tests/test_iface_mtu.py— new. Pins the buffer length with a stubbed ioctl(fails on any interpreter without the fix), pins
Nonewhen the ioctl fails, andreads every interface's MTU back through the real ioctl, comparing against
/sys/class/net/<iface>/mtu..github/workflows/ci.yml— the suite now runs twice in the same job: on therunner's own
python3(3.10) and on 3.14. A matrix is the obvious way to spelltwo interpreters, and it is wrong here:
mainrequires a status check namedexactly
test, while a matrix names its checkstest (3.10)/test (3.14). Thefirst push of this PR did use a matrix, and
gh pr mergeansweredthe base branch policy prohibits the merge— the required check would never report again, for thisPR or any later one. The second interpreter is a step instead, and it prints its
version before running so the log says which interpreter the results are from.
Testing
On this machine, 3.10.12 (there is no 3.14 here; the second run is what CI is for),
from the repository root:
The 12 skips are
test_zenoh_link.py, which needseclipse-zenohinstalled.The new test against the unfixed helper — what the suite says without this PR:
On CI's 3.14.8 run, all three
test_iface_mtucases pass with this change. Thereal-ioctl test passes on 3.10 either way — the regression is invisible there, which
is exactly what the 3.14 run covers; on 3.14 that path raises the
SystemErrorfrom#63 instead of returning a value.
Also re-ran the reporter's standalone repro on 3.10.12: the 18-byte argument returns
18 bytes with the mtu in place, the 40-byte one returns 40 bytes with the same mtu —
so padding the call changes nothing about what the transport reads on the versions it
already worked on.
Issues
Closes #63