PCSX2 / pcsx2

PCSX2 - The Playstation 2 Emulator
https://pcsx2.net
GNU General Public License v3.0
11.71k stars 1.62k forks source link

gsdx memcpy self-overlap #826

Closed gregory38 closed 8 years ago

gregory38 commented 9 years ago

The issue was reported by Coverity. I'm not sure to understand fully the intent of the code. I can just replace the memcpy by a memmove but I'm not sure that we copy the expected data. Sizeof(*m_clut) is 2.

Full code:

119void GSClut::Write(const GIFRegTEX0& TEX0, const GIFRegTEXCLUT& TEXCLUT)
120{
...
128        // Mirror write to other half of buffer to simulate wrapping memory
129
130        int offset = (TEX0.CSA & (TEX0.CPSM < PSM_PSMCT16 ? 15 : 31)) * 16;
131
   1. Condition TEX0.PSM == PSM_PSMT8, taking true branch
132        if(TEX0.PSM == PSM_PSMT8 || TEX0.PSM == PSM_PSMT8H)
133        {
134                int size = TEX0.CPSM < PSM_PSMCT16 ? 512 : 256;
135
136                memcpy(m_clut + 512 + offset, m_clut + offset, sizeof(*m_clut) * min(size, 512 - offset));
   CID 146727 (#1 of 1): Overlapping buffer in memory copy (BUFFER_SIZE)2. overlapping_buffer: The source buffer this->m_clut + 512 potentially overlaps with the destination buffer this->m_clut, which results in undefined behavior for memcpy.
   Use memmove instead of memcpy.
137                memcpy(m_clut, m_clut + 512, sizeof(*m_clut) * max(0, size + offset - 512));
138        }
139        else
140        {
...
149        }
150}
gregory38 commented 9 years ago

@sudonim1 I think it is your code. If you remember anything?

gregory38 commented 9 years ago

@gabest11 any ideas?

sudonim1 commented 9 years ago

Complete false positive as far as I can tell, neither copy can copy more than 512 entries, which is what coverity is reporting as a potential issue.

gregory38 commented 9 years ago

Ok. Thanks for the checking.