Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: genlayerlabs/genvm-manager/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GenVM PR actionsTick a box to run it (the box unticks itself when handled). Actions only run while the PR has the
Commands
|
c722b34 to
bee9d94
Compare
bee9d94 to
25e15e5
Compare
25e15e5 to
41f3213
Compare
41f3213 to
4200db2
Compare
4200db2 to
32fd6de
Compare
|
Hello, I do not see much value in this check, because it is checked on the rust level. Moreover, this is a big breaking change, so I will leave it as is for now |
32fd6de to
9099ff7
Compare
What
Slot.read/Slot.writeingenlayer-py-stdpass the byte offset straight to_genlayer_wasi.storage_read/storage_write, whosePyArg_ParseTupleformat"y*Iw*"/"y*Iy*"uses theIconverter — documented as "without overflow checking". An offset of2**32 + 5reaches the executor as5,-1as0xffffffff,2**40 + 7as7. That defeats the executor'sslot_access_fitsguard (wasi/genlayer_sdk/mod.rs), which exists precisely so an out-of-slot access becomesErrno::Inval: the guest never sends the out-of-range offset, it silently reads or overwrites another location in the same slot.Offsets are plain Python ints built by pointer arithmetic (
VLA.__getitem__:self._off + 4 + idx * size;Slot.read/writeare public), so e.g.vla.set_length(2**31); vla[2**30]on a 32-byte element aliases offset(4 + 2**35) mod 2**32 == 4.Slot.indirectis already protected byoff.to_bytes(4, 'little');read/writewere not.Change (executor line v0.3,
runners/genlayer-py-std)storage/core.py: addSLOT_SIZE = 1 << 32andcheck_slot_access(off, len)mirroring the executor'sslot_access_fits(off >= 0,len >= 0,off + len <= SLOT_SIZE), called fromSlot.readandSlot.write; raisesOverflowError, consistent with whatindirectalready raises.tests/test_storage_core.py: in-range boundaries accepted;SLOT_SIZE,SLOT_SIZE - 3 + 4,2**40 + 7, negative offset and negative length rejected on both read and write (viaInmemManager, which would otherwise try to allocate the wrapped range).runners/support/versions/current.nix: refreshedgenlayer-std,py-genlayer,py-genlayer-multihashes.Executor commit: kriss39/genvm-executor@90d4ef1 on
pr/v0.3/fix/slot-access-bounds(rebased onto the current pinned7e0936a).The root cause in the C shim (
runners/cpython/modules/_genlayer_wasi/genlayer.c,"I"→ parse as object +PyLong_AsUnsignedLongLong+> UINT32_MAXcheck) is worth fixing too, but it needs a cpython runner rebuild, which I cannot produce here; the Python-side guard covers the only caller. Happy to add the C change if you prefer it in this PR and can refresh thecpythonhash.Verification
runners/genlayer-py-std:pytest tests --ignore=tests/embeddings→ 978 passed, 1 failed (test_render_rejects_malformed_image, PIL not installed in my venv — unrelated).PyArg_ParseTuple('I')masking behaviour confirmed against CPython (PyLong_AsUnsignedLongMask).ruff format/ruff checkclean on the changed files.hash-updater.pyhere (no nix; runners are not built on macOS). I reproduced the content-addressing locally (make-zip.pyunder Python 3.13 over thecleanSource-filteredsrc/+runner.json, then the wrapperDependsuids) and confirmed it reproduces the three currently committed hashes bit-for-bit before applying it to the patched tree. Please treat the#runners-allnix build as unrun on my side; if it reports a mismatch I will update from thegot:values.v0.2.x does not vendor the Python runner sources, so no
pr/v0.2/...branch.Independent of the other two PRs I opened today; whichever
current.nixchange lands second I will rebase and recompute.