binlua: Expose PCSX.Misc.uclUnpack for NRV2E decompression.#2025
binlua: Expose PCSX.Misc.uclUnpack for NRV2E decompression.#2025nicolasnoble wants to merge 2 commits into
Conversation
Mirror of PCSX.Misc.uclPack on the read side. Calls ucl_nrv2e_decompress_safe_8 so corrupt or truncated input errors out cleanly instead of overrunning the output buffer. Decompressed size is a required argument since callers (SLZ-family chunk readers, archive containers) always have it from a header. Same input/output shape conventions as uclPack: File / string / LuaBuffer / Slice in, File / LuaBuffer / Slice out (or nil for a fresh Slice return). Adds third_party/ucl/src/n2e_ds.c to the build for the safe-variant entry points. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds NRV2E decompression: builds the new UCL source, implements a bounds-checked native decompressor wrapper, and exposes it to Lua via an FFI-compatible API with flexible input/output handling and size validation. ChangesUCL decompression support
Sequence DiagramsequenceDiagram
participant Lua as Lua (PCSX.Misc.uclUnpack)
participant Wrapper as uclUnpackWrapper
participant UCL as n2e_ds.c (UCL library)
Lua->>Wrapper: call(src_ptr, src_len, dest_ptr, expectedOutSize)
Wrapper->>UCL: ucl_nrv2e_decompress_safe_8(src_ptr, src_len, dest_ptr, expectedOutSize)
UCL-->>Wrapper: decompressed_bytes / error
Wrapper-->>Lua: decompressed_bytes or 0 on error
🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/supportpsx/binffi.lua`:
- Around line 220-222: Validate that decompressedSize is an integer in the
0..0xffffffff range before calling dest:resize(decompressedSize) and before
passing it as expectedOutSize to C.uclUnpackWrapper: after the existing type
check add a guard that checks math.floor(decompressedSize) == decompressedSize
(or equivalent) and decompressedSize >= 0 and decompressedSize <= 0xffffffff,
otherwise error; use the validated value (no implicit wrapping/coercion) when
calling dest:resize(...) and C.uclUnpackWrapper(..., expectedOutSize).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf2ede71-bf77-4900-9f68-f721139c68a0
📒 Files selected for processing (3)
Makefilesrc/supportpsx/binffi.luasrc/supportpsx/binlua.cc
| if type(decompressedSize) ~= 'number' then | ||
| error('Expected the decompressed size as the second argument') | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In LuaJIT FFI, how are negative or fractional Lua numbers converted when passed to a uint32_t C argument?
💡 Result:
In LuaJIT FFI, when you pass a Lua number to an argument of type uint32_t, the conversion path is “double/float → trunc to an integer → narrow to 32 bits”. Specifically, FFI converts a double/float by truncating toward zero to an (u)int64_t (for uint32_t it goes through int64_t) and then narrows to the target (unsigned) width. This effectively means fractional inputs drop their fractional part before the modulo-to-32-bit narrowing happens. [1][2] For negative values converted to uint32_t: the result is the target-width two’s-complement bit pattern (i.e., effectively modulo 2^32 after truncation). LuaJIT documents that double/float → (u)int32_t is truncation, and unsigned integers are represented as an Integer after narrowing/zero-extension rules; in practice, this yields the wraparound bit pattern consistent with two’s complement narrowing. [1][2] For values outside the defined range for the intermediate step, the behavior is undefined; for example, conversion from Lua numeric literals to uint64_t with -1 is described as outside the defined range for unsigned conversion and may vary. (For uint32_t specifically, the conversion goes via a 32-bit narrowing after the truncation-to-integer step, so keep values within the relevant representable range to avoid undefined behavior.) [3] Practical takeaway: - If x is fractional (e.g., -1.7, 3.9): truncation happens first (toward zero), then the truncated integer is narrowed to 32 bits and treated as unsigned. [1][2] - If x is negative but within the conversion’s defined numeric range: you’ll get the corresponding unsigned 32-bit wraparound bit pattern (two’s complement / modulo 2^32). [1][2] - Avoid relying on conversions where the intermediate conversion is outside the defined range (LuaJIT notes undefined behavior for out-of-range unsigned conversions in related cases). [3] Cited sources: [1] LuaJIT FFI semantics conversion rules; [2] same doc in LuaJIT repo (v2.1); [3] LuaJIT issue discussing undefined behavior when converting out-of-range values like -1 to unsigned 64-bit.
Citations:
- 1: https://luajit.org/ext_ffi_semantics.html
- 2: https://github.com/LuaJIT/LuaJIT/blob/v2.1/doc/ext_ffi_semantics.html
- 3: LuaJIT 2.1 ffi.cast("uint64_t", -1) returns wrong value LuaJIT/LuaJIT#459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant region around the referenced lines
sed -n '180,260p' src/supportpsx/binffi.lua | nl -ba | sed -n '1,120p'
# Locate all uses of decompressedSize within the file
rg -n "decompressedSize" src/supportpsx/binffi.lua
# Look for FFI calls / uint32_t / resize usage around the function
rg -n "resize\\(|uint32_t|ffi\\.cast|ffi\\.new\\(|PCSX\\.Misc\\.uclUnpack" src/supportpsx/binffi.luaRepository: grumpycoders/pcsx-redux
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/supportpsx/binffi.lua"
# Print referenced region with line numbers (without nl)
awk 'NR>=180 && NR<=260 { printf "%d: %s\n", NR, $0 }' "$file"
# Show how decompressedSize is used
rg -n "decompressedSize" "$file"
# Find likely FFI / resize usage in the nearby code
rg -n "resize\\(|uint32_t|ffi\\.cast|ffi\\.new\\(|ffi\\.C|memcpy|cast\\(" "$file"Repository: grumpycoders/pcsx-redux
Length of output: 5406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/supportpsx/binffi.lua"
# Show FFI declarations including uclUnpackWrapper signature
awk 'NR>=1 && NR<=110 { printf "%d: %s\n", NR, $0 }' "$file"
# Show uclUnpack logic around the decompressedSize call/comparison
awk 'NR>=250 && NR<=310 { printf "%d: %s\n", NR, $0 }' "$file"Repository: grumpycoders/pcsx-redux
Length of output: 6646
Validate decompressedSize as a non-negative 32-bit integer before resize() / uclUnpackWrapper.
decompressedSize is only type-checked, but it’s used to size destination buffers (dest:resize(decompressedSize)) and passed to C.uclUnpackWrapper(..., expectedOutSize) where expectedOutSize is uint32_t. With LuaJIT FFI, Lua numbers are truncated toward zero and then narrowed to 32 bits—so fractional values lose their fraction and negative values wrap modulo 2^32. Enforce 0 <= decompressedSize <= 0xffffffff and integer-ness.
💡 Suggested fix
PCSX.Misc.uclUnpack = function(src, decompressedSize, dest)
if type(decompressedSize) ~= 'number' then
error('Expected the decompressed size as the second argument')
end
+ if decompressedSize < 0
+ or decompressedSize > 0xffffffff
+ or decompressedSize ~= math.floor(decompressedSize) then
+ error('Expected the decompressed size as a non-negative 32-bit integer')
+ end
+ decompressedSize = math.floor(decompressedSize)
local srcPtr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if type(decompressedSize) ~= 'number' then | |
| error('Expected the decompressed size as the second argument') | |
| end | |
| if type(decompressedSize) ~= 'number' then | |
| error('Expected the decompressed size as the second argument') | |
| end | |
| if decompressedSize < 0 | |
| or decompressedSize > 0xffffffff | |
| or decompressedSize ~= math.floor(decompressedSize) then | |
| error('Expected the decompressed size as a non-negative 32-bit integer') | |
| end | |
| decompressedSize = math.floor(decompressedSize) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/supportpsx/binffi.lua` around lines 220 - 222, Validate that
decompressedSize is an integer in the 0..0xffffffff range before calling
dest:resize(decompressedSize) and before passing it as expectedOutSize to
C.uclUnpackWrapper: after the existing type check add a guard that checks
math.floor(decompressedSize) == decompressedSize (or equivalent) and
decompressedSize >= 0 and decompressedSize <= 0xffffffff, otherwise error; use
the validated value (no implicit wrapping/coercion) when calling
dest:resize(...) and C.uclUnpackWrapper(..., expectedOutSize).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2025 +/- ##
==========================================
+ Coverage 11.42% 12.33% +0.90%
==========================================
Files 529 532 +3
Lines 148489 149650 +1161
==========================================
+ Hits 16961 18454 +1493
+ Misses 131528 131196 -332 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Mirror of PCSX.Misc.uclPack on the read side. Calls ucl_nrv2e_decompress_safe_8 so corrupt or truncated input errors out cleanly instead of overrunning the output buffer. Decompressed size is a required argument since callers (SLZ-family chunk readers, archive containers) always have it from a header. Same input/output shape conventions as uclPack: File / string / LuaBuffer / Slice in, File / LuaBuffer / Slice out (or nil for a fresh Slice return).
Adds third_party/ucl/src/n2e_ds.c to the build for the safe-variant entry points.