Codejock Forums Homepage
Forum Home Forum Home > Codejock Products > Visual C++ MFC > Controls
  New Posts New Posts RSS Feed - [SOLVED]CXTPHexEdit crash when replacing Data buff
  FAQ FAQ  Forum Search   Events   Register Register  Login Login

[SOLVED]CXTPHexEdit crash when replacing Data buff

 Post Reply Post Reply
Author
Message
grumat View Drop Down
Newbie
Newbie


Joined: 02 September 2021
Location: Austria
Status: Offline
Points: 4
Post Options Post Options   Thanks (0) Thanks(0)   Quote grumat Quote  Post ReplyReply Direct Link To This Post 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
Back to Top
agontarenko View Drop Down
Admin Group
Admin Group


Joined: 25 March 2016
Status: Offline
Points: 294
Post Options Post Options   Thanks (0) Thanks(0)   Quote agontarenko Quote  Post ReplyReply Direct Link To This Post 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  
Back to Top
grumat View Drop Down
Newbie
Newbie


Joined: 02 September 2021
Location: Austria
Status: Offline
Points: 4
Post Options Post Options   Thanks (0) Thanks(0)   Quote grumat Quote  Post ReplyReply Direct Link To This Post 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.
Back to Top
agontarenko View Drop Down
Admin Group
Admin Group


Joined: 25 March 2016
Status: Offline
Points: 294
Post Options Post Options   Thanks (0) Thanks(0)   Quote agontarenko Quote  Post ReplyReply Direct Link To This Post 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 
Back to Top
grumat View Drop Down
Newbie
Newbie


Joined: 02 September 2021
Location: Austria
Status: Offline
Points: 4
Post Options Post Options   Thanks (0) Thanks(0)   Quote grumat Quote  Post ReplyReply Direct Link To This Post 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.
Back to Top
agontarenko View Drop Down
Admin Group
Admin Group


Joined: 25 March 2016
Status: Offline
Points: 294
Post Options Post Options   Thanks (0) Thanks(0)   Quote agontarenko Quote  Post ReplyReply Direct Link To This Post 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.
Back to Top
 Post Reply Post Reply
  Share Topic   

Forum Jump Forum Permissions View Drop Down

Forum Software by Web Wiz Forums® version 12.04
Copyright ©2001-2021 Web Wiz Ltd.

This page was generated in 0.172 seconds.