[SOLVED]CXTPHexEdit crash when replacing Data buff

Message Topic Search Topic Options Post Reply Create New Topic Printable Version Translate Topic

   Hi,

I had a interesting scenario here:
An instantiated control displaying a 60K buffer.
Users scrolls up to the end of buffer and puts the cursor on the last position
User commands reload data
Crash inside CXTPHexEdit::OnDraw().

Reload Data does actually:
ctrl.SetData(NULL, 0);
Loads data
If data load OK: ctrl.SetData(newbuffer, newlength);

Problem is: The first command shrinks the buffer to 0.
The current implementation of CXTPHexEdit::SetData() does:
Replaces the buffer (m_pData) to the empty size
Calls SetSel(-1,-1)
Updates the indexes, including buffer bounds

Issue: SetSel causes repaint, and OnDraw ends up accessing invalid memory because buffer bounds refers to the old buffer size.

My current fix was to command:
    ctrl..m_nLength = 0;    // OMG! access to internal stuff!!
before the new SetData().

Surely that a definitive fix will needs more accuracy and tests, but hoping to see it on the next release.

Best regards,

M. Gruber

Author	Message Topic Search Topic Options Post Reply Create New Topic Printable Version Translate Topic
grumat Members Profile Send Private Message Find Members Posts Add to Buddy List Newbie Joined: 02 September 2021 Location: Austria Status: Offline Points: 4	Post Options Post Reply Quote grumat Report Post Thanks(0) Quote Reply Topic: [SOLVED]CXTPHexEdit crash when replacing Data buff Posted: 02 September 2021 at 4:19am
	Hi, I had a interesting scenario here: An instantiated control displaying a 60K buffer. Users scrolls up to the end of buffer and puts the cursor on the last position User commands reload data Crash inside CXTPHexEdit::OnDraw(). Reload Data does actually: ctrl.SetData(NULL, 0); Loads data If data load OK: ctrl.SetData(newbuffer, newlength); Problem is: The first command shrinks the buffer to 0. The current implementation of CXTPHexEdit::SetData() does: Replaces the buffer (m_pData) to the empty size Calls SetSel(-1,-1) Updates the indexes, including buffer bounds Issue: SetSel causes repaint, and OnDraw ends up accessing invalid memory because buffer bounds refers to the old buffer size. My current fix was to command: ctrl..m_nLength = 0; // OMG! access to internal stuff!! before the new SetData(). Surely that a definitive fix will needs more accuracy and tests, but hoping to see it on the next release. Best regards, M. Gruber

agontarenko Members Profile Send Private Message Find Members Posts Add to Buddy List Admin Group Joined: 25 March 2016 Status: Offline Points: 293	Post Options Post Reply Quote agontarenko Report Post Thanks(0) Quote Reply Posted: 03 September 2021 at 3:57am
	Hello, Unfortunately it is not clear what exactly is going wrong. Can you please provide more detailed information. If it cannot be re-produced in any of our sample applications provided I would appreciate you sending a sample application so that we could debug it. Regards, Artem Gontarenko

grumat Members Profile Send Private Message Find Members Posts Add to Buddy List Newbie Joined: 02 September 2021 Location: Austria Status: Offline Points: 4	Post Options Post Reply Quote grumat Report Post Thanks(0) Quote Reply Posted: 03 September 2021 at 4:27am
	Just see your source code: XTPHexEdit.cpp: void CXTPHexEdit::SetData(LPBYTE p, int nLength, int nMaxLength) { if (m_pData) free(m_pData); m_pData = (LPBYTE)malloc(nLength); MEMCPY_S(m_pData, p, nLength); SetSel(-1, -1); m_nMaxLength = nMaxLength < 0 ? -1 : __max(nMaxLength, nLength); m_nLength = nLength; ... See the bold lines: You free the old buffer, then allocates the new buffer with the new length. Before updating the new buffer bound (m_nLength) you issue a SetSel, which "may" cause a WM_PAINT. void CXTPHexEdit::SetSel(int nSelStart, int nSelEnd) { ... RedrawWindow(); ... } The OnDraw checks m_nLength to keep access within the buffer bounds, but since the order of the previous code has a flaw, the m_nLength is not valid for the new buffer. This is critical if the new size of the buffer decreases. The use case requires an initial "big buffer" and also that we "scroll to the end of that buffer". So when a tiny new buffer is newly applied, it causes that all indexes for the current scroll position are outside of the new buffer bound, which will cause a CPU exception in the OnDraw: void CXTPHexEdit::OnDraw(CDC* pDC) ... if (m_pData) ... if (m_bShowHex) ... if (m_nSelStart != -1 && (m_eEditMode == editHigh \|\| m_eEditMode == editLow)) ... else ... for (i = m_nTopIndex; (i < m_nLength) && (rcd.TopLeft().y < height);) ... TOHEX(m_pData[ i ], p); This is the flow up to the access exception. Although m_nLength correctly guards the array bounds for the initial state, it actually doesn't during the SetData, because it refers to the "old control state". Note: during my tests I had situations where a WM_PAINT did not happen (maybe Z-ordering), so the bug did not happen. Also CPU memory protection has a granularity with a minimum block size (4K, AFAIK), so the invalid access may not cause the exception in a 100% reproducible case. But the logic above has a clear flaw. Hope this helps.

agontarenko Members Profile Send Private Message Find Members Posts Add to Buddy List Admin Group Joined: 25 March 2016 Status: Offline Points: 293	Post Options Post Reply Quote agontarenko Report Post Thanks(0) Quote Reply Posted: 06 September 2021 at 3:35am
	Hello, I understood that you have problem. For a solution, I need to reproduce this bug. Can you change HexEdit sample that the problem is reproducible, and attach it to topic. Also, in v20.0 or 20.1 has been some changes, may be this problem already fixed. Regards, Artem Gontarenko

grumat Members Profile Send Private Message Find Members Posts Add to Buddy List Newbie Joined: 02 September 2021 Location: Austria Status: Offline Points: 4	Post Options Post Reply Quote grumat Report Post Thanks(0) Quote Reply Posted: 06 September 2021 at 3:44am
	As you don't seem to understand: I have a fix for your bug and I am sharing it to you. There are enough information for a regular experienced programmer understand and fix the issue. If you are not interested in the "continuous product improvement", then please close the issue. I won't annoy you anymore.

agontarenko Members Profile Send Private Message Find Members Posts Add to Buddy List Admin Group Joined: 25 March 2016 Status: Offline Points: 293	Post Options Post Reply Quote agontarenko Report Post Thanks(0) Quote Reply Posted: 06 September 2021 at 4:35am
	Ok, I changed our code. You right. void CXTPHexEdit::SetData(LPBYTE p, int nLength, int nMaxLength) { if (m_pData) { free(m_pData); m_nLength = 0; } Thanks.