[REGEDIT] Fix crashes in regedit-find affecting CORE-15986 and CORE-18230

[Doug-Lyons]

Purpose

Fix crashes in regedit-find.

JIRA issue: CORE-15986
JIRA issue: CORE-18230

Proposed changes

After possible RegQueryValueExW append 3 zero bytes to guarantee that we will end with a UNICODE NULL.

[tkreuzer]

This is not how to use SEH. If there is a crash without SEH, then there is a bug, like a buffer overflow. Please remove SEH and fix that bug instead.

[Doug-Lyons]

Closed in favor of #5150.

[Doug-Lyons]

Reopening to only fix read beyond end of buffer crashes.

[ThFabba]

I wouldn't put this much work into it TBH. I'd say just write a single UNICODE_NULL -- if the string is an odd length, overwrite the last byte. I.e.

PWCHAR ValueBuffer = (PWCHAR)pb;
ValueBuffer[cb / sizeof(WCHAR)] = UNICODE_NULL;

Weird data in the registry just happens. To me, it doesn't seem worth even DPRINTing for. Except maybe from a dedicated tool that helps us find bugs by pointing out weird registry data so we can locate and fix the code that wrote it.

[Doug-Lyons]

I tested your suggestion by going to 'master' and adding your two lines at line number 234. This was right after the RegQueryValueEx and its checking for returned value error. This produced a very quick crash with the log as follows:

Unhandled exception
ExceptionCode:    c0000005
Faulting Address: 20230A38
CS:EIP 1b:7c93f73a
DS 23 ES 23 FS 3b GS 0
EAX: 00230a3c   EBX: 0024e218   ECX: ffffffff
EDX: 07ffffff   EBP: 0022dc38   ESI: 00230000   ESP: 0022dc38
EDI: ffffffff   EFLAGS: 00010a07

*** Assertion failed: FreeEntry->PreviousSize == CurrentEntry->Size
***   Source File: ../sdk/lib/rtl/heap.c, line 1166

Break repeatedly, break Once, Ignore, terminate Process or terminate Thread (boipt)? 
kdb:> o
Execute '.cxr 0022CD60' to dump context
traywnd.cpp:3259: Unexpected failure (m_TrayBandSite->ProcessMessage(m_hWnd, uMsg, wParam, lParam, &Ret))=80004005.
fixme:(dll/win32/imm32/ctf.c:77) (04090409)
aclmulti.cpp:62: Unexpected failure (punk->QueryInterface(IID_PPV_ARG(IACList, &fObjects[fObjectCount].pACL)))=80004002.
err:(dll/win32/shlwapi/autocomp.cpp:51) punk->QueryInterface failed: 0x80004002
fixme:(dll/win32/shlwapi/url.c:1977) (L"regedit" 4): stub
fixme:(dll/win32/shell32/shlexec.cpp:1926) flags ignored: 0x00000100
fixme:(dll/win32/imm32/ctf.c:77) (04090409)
Unhandled exception
ExceptionCode:    c0000005
Faulting Address: 20230A38
CS:EIP 1b:7c93f73a
DS 23 ES 23 FS 3b GS 0
EAX: 00230a3c   EBX: 0024e218   ECX: ffffffff
EDX: 07ffffff   EBP: 0022dc38   ESI: 00230000   ESP: 0022dc38
EDI: ffffffff   EFLAGS: 00010a07

*** Assertion failed: FreeEntry->PreviousSize == CurrentEntry->Size
***   Source File: ../sdk/lib/rtl/heap.c, line 1166

Break repeatedly, break Once, Ignore, terminate Process or terminate Thread (boipt)? 
kdb:> O
Execute '.cxr 0022CD60' to dump context

Suggestions? Thanks for your help on this.
Later: Maybe I should have done this only after checking for an odd number of bytes returned?
But this does not handle the case where we are missing a UNICODE_NULL at the end of the string?
Reason for editing: Jira wanted to turn my pound sign with number following into a link. :(

[katahiromz]

PWCHAR ValueBuffer = (PWCHAR)pb;
ValueBuffer[cb / sizeof(WCHAR)] = UNICODE_NULL;

This broke the original data. I would reject it.

[tkreuzer]

Jesus. When a unicode string has an odd size in bytes, there is a major problem. This doesn't just happen. It means there is a bug somewhere. This whole "let's ignore the bug and just pretend it's all good" thing is not the way to go. Can we please stop adding hacks and fix the shit where it is broken? "X crashes and Y does not, so Y must be the correct code" is complete BS. If there is a crash, then there is a bug somewhere. Find it! Stop being lazy asses and commit hacks to work around existing bugs that you are too lazy to find.

[JoachimHenze]

I unresolved this conversation with the aim to make @tkreuzer comment more visible in the future.
Please leave it in unresolved state.

[HBelusca]

In any case, a registry viewer/editor should not crash when it encounters malformed data.

[tkreuzer]

The data comes from the registry. If it is malformed, there is a either a bug in the kernel or - and that is more likely - a bug in some other regedit code. Either way, someone needs to investigate it instead of blindly assuming that random crashes on invalid data are simply something "normal".

[katahiromz]

https://github.com/reactos/reactos/pull/5150/files#diff-e6020d9b127f328f907c818bb9c674c07e03b834649b131637b4c9feb607bc46R103

DebugBreak(); will be added for odd size.

[ThFabba]

Please don't add extra code for this. Just handle it safely without crashing. If we want to find bugs in places that write to the registry, let's have a separate scanner tool.
My suggestion from the top of this thread still stands. The only reason it crashed is because the malloc call needs space for the extra null terminator, just like it does with the weird 3-byte solution.

[Doug-Lyons]

Thanks for everyone's help with reviews on this and thanks to @katahiromz for committing it.