Minor fixes and refactoring - #2979
Merged
Merged
Conversation
The x87 path of Normalize() ended with 'finit', which resets the entire FPU: the control word (precision and rounding mode), the exception masks, the tag word and every register. That is far more than this function is entitled to touch - any caller with values live on the x87 stack lost them silently, and the MSVC runtime's 53-bit precision control was replaced by the 64-bit default. It was also unnecessary on the normal path, where the three fstp stores already pop everything the function pushed. Only the zero-length branch jumped to the epilogue with five values still on the stack. Balance the two paths explicitly instead: the normal path jumps over the cleanup, and the zero-length path pops its five entries with fstp ST(0). Both now leave the stack at depth 0 and the control word untouched.
_findBuffer walked m_writeBuffers/m_readBuffers with 'while (_buffers[i] != nullptr)' and no upper bound. std::array::operator[] does not range check, so once all 6 slots were occupied the loop read _buffers[6] - out of bounds - and that stray value decided whether it kept walking. The assert() fired only after the bad read had already happened, and only in debug builds; in release the function returned index 6 and both callers used it unconditionally to write, corrupting whatever follows the array. Rewrite as a bounded loop with an explicit contract: found -> (true, index of the entry) not found, space available -> (false, index of the first free slot) not found, array full -> (false, _buffers.size()) Both writing call sites now check the index against size() before indexing. The third caller only inspects .first and is unaffected. This also fixes the off-by-one on a hit: '_buffers[i++] == _buf' returned k+1 rather than k. That had to go with it - the point of the change is to make the returned index safe to use, and an index that is wrong on a hit is not. The full-array case is a legitimate runtime condition (a game touching more than 6 distinct framebuffers between GetInfo calls), not a programming error, so it is now handled rather than asserted on. That left <assert.h> unused and it is removed.
… bool RSP_LoadMatrix built a _N64Matrix* by casting &RDRAM[address]. That violates strict aliasing and assumes an alignment an arbitrary display list address does not provide. It also read 64 bytes with no check that they lie inside RDRAM. Only the three gSP.cpp callers guarded the address beforehand; the seven uCode call sites did not, so a malformed display list could read past the end of RDRAM. The generic implementation now reads each element with memcpy from computed byte offsets, and the NEON one memcpy's the 64 bytes into a naturally aligned local before running the intrinsics over it. Neither overlays a struct on RDRAM any more. The X86 asm reads through esi with a memory clobber and needed no aliasing change. All three implementations validate the address and return false when it does not hold a whole matrix, leaving mtx untouched. The check is written as 'address > RDRAMSize || RDRAMSize - address < 64' rather than 'address + 64 > RDRAMSize' because address is untrusted and the addition wraps: 0xFFFFFFC0 + 64 is 0, which passes the naive form. Callers now check the result: - gSP.cpp: the pre-checks are redundant once the callee validates, so they are replaced by the return value test. Their DebugMsg output is preserved in the failure branch. - ZSort.cpp, ZSortBOSS.cpp: the gSP.changed update is skipped on failure, so a stale matrix is no longer marked as freshly loaded, and the bad address is logged. - F5Indi_Naboo.cpp: logs on failure but still calls F5INDI_LoadSTMatrix, which reads DMEM and does not depend on mtx_vtx_gen.
Both microcodes walked RDRAM with no bounds checking at all. A malformed display list could read arbitrary host memory past the end of the buffer. The worst case was the outer loop of RunTurbo3D and RunT3DUX. Both read their command words from RSP.PC[RSP.PCi] and then advance the PC by 16 resp. 24 indefinitely, terminating only when they happen to find a zero pstate. RSP.PC[0] itself comes unvalidated from DMEM[0x0FF0], so neither the starting point nor the walk was checked. _ProcessDList guards exactly this case for the normal microcodes; these two loops were the only display list loops without such a guard. They now check before every command and halt with a log if the PC leaves RDRAM. Turbo3D validates the 16 bytes it reads and T3DUX the 20 it reads, rather than the 24 byte stride, because the sixth word is commented out. Turbo3D_ProcessRDP and T3DUX_ProcessRDP had the same problem one level down: an unterminated command list walked off the end two words at a time. Every read there is now checked, including the extra pair consumed by G_TEXRECT and G_TEXRECTFLIP. Note that in T3DUX that read sits inside a switch, so a plain break would only leave the switch and the guard would have had no effect; it needs the explicit test after the switch. The remaining accesses overlaid structs directly on RDRAM at unvalidated addresses: T3DGlobState, T3DState, T3DUXGlobState, T3DUXState, the T3DTriN and T3DUXTriN walks, the VtxOut array, and the T3DUX colour and texture coordinate lookups. Casting u8* to a struct pointer also violates strict aliasing and assumes an alignment a display list address does not provide, so these now memcpy into a local after a range check. Triangle and vertex counts are u8, so a list is at most 255 elements and the size computations cannot overflow. The range checks share a new isRDRAMRangeValid() in N64.h, next to RDRAM and RDRAMSize. RDRAMSize is the index of the last valid byte, not the size, so the helper compares against RDRAMSize + 1 and is written so that neither the addition nor the subtraction can wrap. It is exact: it accepts a range ending on the last valid byte, where the older 'address + n > RDRAMSize' idiom used elsewhere is one byte conservative.
Division by zero in F3DSWRS_TexrectGen -------------------------------------- offset_x_i and offset_y_i are the result of an integer division and can round down to zero, after which param4X / offset_x_i divides by zero. This is less obvious than it looks, because the existing tests do cover the ordinary case: lrx - ulx is exactly 2 * offset_x_f, so a zero offset makes the difference zero and the function returns. The hole is that screenX is v.x / v.w * vscale + vtrans. When v.w is zero screenX is infinite, lrx - ulx is inf - inf, that is NaN, and NaN <= 0.0f is false, so both tests fall through to the divisions. Guard the divisors directly. Unvalidated colour data reads ----------------------------- TriGen0000_PrepareColorData copied 25 words from an address taken straight out of the display list, with no check that they lie inside RDRAM. TriGen0001_PrepareColorData, the adjacent function, reads a variable number of words from an equally unvalidated address in a loop. Both now range check with isRDRAMRangeValid first. Both also built a u32* by casting &RDRAM[address], which violates strict aliasing and assumes an alignment the address does not provide. In TriGen0001 the address is deliberately offset by up to 7 bytes, so the loads really are misaligned; UBSAN reports them on the old code. Both now read through memcpy. Unsigned underflow in the TriGen0001 data size ---------------------------------------------- dataSize was ((x & 0xFF0) - (_params[3] & 0x07)) / 4. The two terms are independent, so the subtraction can wrap: with the masked end at 0 and the offset at 4 it yields 0x3FFFFFFF words. That runs the loop off the end of RDRAM, and it also defeats the bounds check meant to prevent it, because (0x3FFFFFFF + 5) * sizeof(u32) overflows u32 down to 0x10. Reject the underflow before dataSize is formed, which restores the cap the rest of the reasoning depends on. Verified with ASAN and UBSAN. TriGen0000 matches the previous code over 4000 addresses including unaligned ones, and its bound accepts a read ending on the last valid byte while rejecting one past it. TriGen0001 matches over 3986 well formed cases, and the smallest slack between the dataSize + 5 bound and the highest word the loop actually reads is exactly zero, so the bound is tight. The underflow case is rejected. For the texrect guard, v.w of 1 and 1e-30 are caught by the pre-existing test as described, while v.w of 0 and -0 are not and reach the division.
…g on Linux 64-bit
readPNG allocated with malloc(row_bytes * o_height), both ints. row_bytes comes from png_get_rowbytes and o_height from the IHDR chunk, so the product is attacker controlled. Overflowing a signed int is undefined behaviour, and in practice it wraps to a small value: a 32768 x 32768 RGBA image needs 4 GiB and the multiplication yields exactly 0. The png_read_rows loop below then writes o_height rows of row_bytes each into that buffer, overflowing the heap. Compute the size in png_size_t and validate before allocating. The guard rejects a non-positive height, which also covers a height above INT_MAX, since o_height is an int filled through a (png_uint_32*) cast; a zero row size; a row size that does not fit the int row_bytes used further down; and any product that would overflow size_t. Failure takes the same png_destroy_read_struct and return nullptr path as the format check above. Note that a large image is not itself the problem, under allocating for one is. On 64-bit the corrected size is simply passed to malloc, which fails for the absurd cases, and the existing 'if (image)' already handles that. On a 32-bit size_t the guard rejects them outright. Checked against libpng's default user limits of 1000000 x 1000000: that image needs 3.6 TiB and the old code asked for 1.29 GiB. 100000 x 100000 needs 37 GiB and asked for 1.25 GiB. Every size the new guard accepts allocates exactly the true product, verified for both a 64-bit and a simulated 32-bit size_t, along with six boundary cases around INT_MAX and zero.
init() allocated the staging buffer with malloc and never checked the result. Every use of m_pbuf in _copyFromRDRAM writes through it, as the destination of _copyBufferFromRdram and _copyPixelsFromRdram, as the target of the float conversion, and as updateParams.data, so a failed allocation is an immediate null dereference. init() returns void and cannot report the failure, so log there and guard at the only consumer. The guard sits after the Cleaner is constructed, like the existing 'if (!bCopy) return;' below it, so the RAII reset still runs. addAddress computed pixelSize as (1 << m_size) >> 1, that is bytes per pixel, which is zero for G_IM_SIZ_4b because a 4bpp pixel is half a byte. _address % pixelSize then divides by zero. Guard only the modulo rather than returning early on pixelSize == 0, because the two are not equivalent: when _size is also zero the size comparison short circuits and the address is still pushed, and that path works today. Verified exhaustively over all four m_size values, _size 0 to 8 and 4096 addresses: of 147456 combinations 114688 decide identically, 32768 are exactly the cases where the old code evaluated _address % 0, and none differ outside that undefined region. The undefined set is precisely m_size == 0 with _size != 0. Skipping those addresses rather than pushing them is correct, since copyFromRDRAM rejects any buffer below G_IM_SIZ_16b and a 4bpp buffer's addresses could never be consumed.
Both mapping sites used the returned pointer without checking it. If the GPU map fails, under memory pressure or a driver error, the memcpy that follows dereferences null. Checking for null is not enough on its own: the caller has to skip the draw as well, otherwise it renders from stale buffer contents at an offset that was never written. _updateBuffer therefore returns bool now, and that propagates through _updateRectBuffer and _updateTrianglesBuffers to drawRects, drawTriangles and drawLine, each of which returns before its glDraw call. Three details this depends on: - offset and pos are not advanced when the write fails. The early return happens before that bookkeeping. Advancing them would leave the buffer position out of step with its contents and corrupt every later draw, not just the failed one. - glUnmapBuffer is not called after a failed map, since nothing is mapped. - The persistent path fails at init, not at use. _initBuffer maps once in the constructor, and if that returns null then _buffer.data stays null and every later memcpy(&_buffer.data[offset], ...) dereferences it. That is now caught by a null check in _updateBuffer, with the message logged once at init rather than once per draw call. Not compile tested: the build environment for this change had no GL headers available. Verified instead by checking that all call sites of the four changed methods are confined to this class and that the three overridden signatures are unchanged, and by running a mock of the control flow under ASAN and UBSAN over all four combinations of bufferStorage and map success. On failure no draw is issued, offset and pos are unchanged and glUnmapBuffer is never called; on success all draws are issued, offsets advance and each map is matched by one unmap.
_convertFloatTextureBuffer computed bytesToCopy from the caller supplied _height, but m_tempPixelData is sized for the texture. One row over the texture height and the copy_n runs past both ends: it reads beyond the GPU buffer and writes beyond the staging buffer. Clamp the copy to the staging buffer, and compute the size in size_t, since the old int could overflow before anyone compared it against anything. Clamping the copy alone is not enough. The existing _height clamp bounds the writes into m_pixelData, but nothing bounded the reads out of m_tempPixelData, and the loop indexes at (heightIndex + _heightOffset) * stridePixels + widthIndex. For the two PBO readers _heightOffset is 0 and _stride equals the texture width, so that happens to coincide with what was copied. ColorBufferReaderWithEGLImage sets _heightOffset to _params.y0 and _stride to the hardware buffer stride, both non-zero, so the reads can leave the copied region even after the first clamp. _height is now also clamped to the rows actually present in the staging buffer. Truncating rather than reading past the end matches what the existing destination clamp already does.
Follow up to 6d0ed40. Making m_swapBuffersQueued atomic removed the data race between the main and GL threads, but not a lost wakeup left behind by it. ReduceSwapBuffersQueued runs on the GL thread and both decrements the counter and calls notify_all without ever acquiring m_condvarMutex. Both operations can therefore land in the gap between WaitForSwapBuffersQueued testing its predicate and actually blocking. The predicate then reads true, the notification has already been delivered to nobody, and the main thread waits for a wakeup that never arrives, because the GL thread has nothing left to decrement. That is a hang, not a glitch. Acquiring the mutex after the state change and before the notify closes the window: the notifier either blocks until the waiter has released the mutex and enqueued itself, or arrives before the waiter took it at all. The two shutdown paths, CoreVideo_Quit and windowsStop, notify the same condition variable the same way and get the same guard. Guarding the shutdown notifications alone would have achieved nothing, though. The wait predicate tested only m_swapBuffersQueued <= MAX_SWAP, so a shutdown notify_all woke the waiter, the predicate was still false and it went straight back to sleep; shutting down could never release a blocked waiter. m_shutdown is now part of the predicate as well as the guard in front of it, and it becomes std::atomic<bool> because the predicate may be evaluated on a different thread from the one that calls CoreVideo_Quit or windowsStop.
Four unchecked dereferences, two of which the review did not list. glGetString returns null if the context is invalid or the enum is not accepted. On the save path both results went straight into strlen. The check is hoisted above the ofstream, which is opened with trunc, so bailing out later would leave a truncated cache file behind. The load path has the same null, reaching strncmp instead of strlen. It is now treated as a cache mismatch and falls through to _loadFromCombinerKeys, which is what every other validation failure in that function does. Combiner_Compile was dereferenced without a check in _loadFromCombinerKeys and again in loadShadersStorage, in the 'shader is not a valid binary, compiling from key instead' fallback. Both now return false, which is the right signal: CombinerInfo responds by deleting every combiner loaded so far, clearing the map and building on demand, so bailing mid loop leaks nothing. Both new failure paths also clear the load progress display before returning, otherwise the LOAD COMBINER SHADERS overlay stays on screen.
In the GT_FLAG_NO_XFM branch only vertices [0, vtxCount) are written by the loop above, but the triangle indices were not checked against that.
_convertIntegerTextureBuffer has no staging copy: its memcpy reads
straight out of the buffer returned by _readPixels, and the only clamp
was against the destination. The last byte it touches is
(_height - 1 + _heightOffset) * strideBytes + widthBytes - 1, which the
destination clamp does not bound.
The trigger is not an oversized _height on its own. When _width equals
the texture width the destination clamp incidentally caps _height at the
texture height. The overread needs either a non-zero _heightOffset, which
the EGL async path sets to _params.y0 so the rows run from y0 to
y0 + height - 1, or a narrow _width, which shrinks widthBytes so the
destination clamp permits a much larger _height while strideBytes stays
at the full texture width. A 64 pixel wide read at four times the height
indexes about 653 KB into a 160 KB buffer.
Only the derived reader knows how large its buffer is, so _readPixels
gains a size_t& _gpuDataSize out parameter. The five paths report:
PixelBuffer the glMapBufferRange length, since only the mapped
range is readable and an oversized request makes the
map fail, which readPixels already handles
BufferStorage textureBytes, matching glBufferStorage and the
persistent mapping
ReadPixels m_pixelData.size()
EGLImage async stride * texture height * 4, from the
AHardwareBuffer_Desc used in _initBuffers
EGLImage sync m_pixelData.size()
Both conversion helpers clamp against it. The float path was already
bounded by the staging buffer, but bounding it by the source as well is
correct and costs nothing.
The renderer and GL version strings are stored without a terminator, and strncmp stops after len characters, so the check only ever verified that the cached string is a prefix of the current one. A cache written on a GPU reporting 'NVIDIA' was accepted on one reporting 'NVIDIA GeForce RTX 4090', and a zero length entry matched every renderer. Only the shortening direction was broken: a cached string longer than the current one runs into the current one's terminator and mismatches correctly, which is why this went unnoticed. Compare the lengths first, then memcmp exactly those bytes. The length check deliberately precedes the std::vector<char> strBuf(len) allocation. len is read straight off disk, so a corrupt file could previously request an arbitrary allocation before anything validated it. A garbage length now cannot match the real strlen and is rejected without allocating. Returning before consuming the string bytes is fine: _loadFromCombinerKeys opens the keys file, so the shaders stream is abandoned either way.
… add nullptr check.
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
1 participant
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.
Most of fixes address potential issues, such as out-of-bounds reads/writes or data races.
No changes in emulation were implied.