Fix memory overwriting bug by BabyGrid

[xomx]

Fix #14855 (comment) , #14871 (comment)

BabyGrid code was overwriting foreign memory on its initialization and deinitialization. At that time (WM_NCCREATE, WM_NCCALCSIZE, WM_CREATE and WM_NCDESTROY) the relevant FindGrid func returns -1, which was used as an index pointing to a memory area before the whole BGHS object (BGHS[-1]...)!

This was a long-standing hidden bug that only started to manifest itself probably when the app memory layout shifted somehow and important objects/data started to be overwritten, resulting in the visible app crashes.

[ozone10]

@xomx
cool. Could you tell me how did you find out the issue, for future reference, thx.

[rdipardo]

Could you tell me how did you find out the issue, for future reference, thx.

You would need a disassembler to really find out. What @xomx means by "app memory layout" is the stack alignment of the function parameters and local variables. When you add a parameter or declare a new variable, more data gets pushed to the stack, and the existing data has to be aligned differently.

In a 32-bit process, each data item is aligned on a boundary of 4 bytes. If the data's byte count in not a perfect multiple of 4, padding is used to push it into the next boundary. A 32-bit pointer is also 4 bytes. Before the hDPI stuff, the pointer to the Baby Grid probably had padding around it, so a negative index would land in the padded zone where no real data was stored, without any fatal consequence. Data in a 64-bit process is 8-byte aligned, which probably results in padding, before and after the hDPI stuff, so the crash was always avoided.

[xomx]

What @xomx means by "app memory layout" is the stack alignment of the function parameters and local variables

the Baby Grid probably had padding around it, so a negative index would land in the padded zone where no real data was stored, without any fatal consequence.

Not this time, the sizeof(BGHS) > 60kB for sure!
So the BGHS[-1] can influence that much of the app memory...
IIRC from my night-debugging - the @ozone10 hidpi commit needs some extra static-memory and the CRT stuff uses the same mem-segment, so that commit revealed the bug by shifting that mem-layout and caused overwriting of the already preinitialized static CRT internals like the locale-stuff...

Could you tell me how did you find out the issue

Ok, I will describe what I did to find that later (in short - I used HW memory breakpoints to watch the relevant NULLed CRT mem-address).

[ozone10]

@xomx
what about

notepad-plus-plus/PowerEditor/src/WinControls/Grid/BabyGrid.cpp

Line 150 in 33ac6e7

void RefreshGrid(HWND hWnd)

it also uses FindGrid

[donho]

RefreshGrid is called usually after initialization, but since FindGrid could return -1, it might be worth to do the following:

void RefreshGrid(HWND hWnd)
{
	 RECT rect;
     int SI;
	 GetClientRect(hWnd,&rect);
	 InvalidateRect(hWnd,&rect,FALSE);
	 SI=FindGrid(GetMenu(hWnd));
     if(SI != -1 && BGHS[SI].EDITING)
         {
          DisplayEditString(hWnd, SI, TEXT(""));
         }

}

[xomx]

@ozone10

what about

notepad-plus-plus/PowerEditor/src/WinControls/Grid/BabyGrid.cpp

Line 150 in 33ac6e7

void RefreshGrid(HWND hWnd)

it also uses FindGrid

It is covered by my patch too.

When I saw that horrible BabyGrid code referencing that FindGrid or directly the BGHS[SelfIndex]... at zillion places, I 1st thought about a shim-fix, like the MS compatibility team does for any nasty-behaving-but-well-known-widespread-app (I wanted to change the declaration from BGHS[MAX_GRIDS] to BGHS[MAX_GRIDS + 1] and then replace the starting object ptr BGHS[0] with BGHS[1], leaving the BGHS[-1] to BGHS[0] memory area safe for those bad-writes...).

But nowadays I am really short of time so I just tried my luck - I added this to the FindGrid func before its return returnvalue;

     if (returnvalue == -1)
     {
         ::MessageBeep(MB_OK); // can put a BP here as this cannot be optimized out by the Release build compiler opts
     }

Then I just debugged and watched if the FindGrid returning -1 has to be patched at some other places than in my current PR fix. Luckily after the WM_CREATE the FindGrid never returns the -1 (until the ending WM_NCDESTROY, but this is ok too with my simple fix).

That RefreshGrid is 1st called at the WM_CREATE stage but fortunately after the point, which ensures that the FindGrid is already returning correct numbers and not the -1...

[xomx]

sorry for the delay, here is the final proof:

[donho]

73bc79e