[PATCH] ntdll/tests: Add a test to show that CommandLine buffer should be inside of the ProcessParameters block.

[Dmitry Timoshkov]

I have an application that calls GetCommandLineW() and then appends to
the returned pointer its own custom command line. Currently this leads
to the heap corruption under Wine, while under Windows this apparently
just overwrites the tail of Peb->ProcessParameters block.

Under Wine the reason of heap corruption is that dlls/kernel32/process.c,
build_command_line() replaces the Peb->ProcessParameters->CommandLine by
a heap allocated buffer which points outside of the Peb->ProcessParameters
block.

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru>
---
 dlls/ntdll/tests/env.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/dlls/ntdll/tests/env.c b/dlls/ntdll/tests/env.c
index 0c864fb2bc..d7144f6e8d 100644
--- a/dlls/ntdll/tests/env.c
+++ b/dlls/ntdll/tests/env.c
@@ -330,8 +330,18 @@ static void test_process_params(void)
     WCHAR *str;
     UINT_PTR pos;
     MEMORY_BASIC_INFORMATION info;
-    NTSTATUS status = pRtlCreateProcessParameters( ¶ms, &image, NULL, NULL, NULL, NULL,
-                                                   NULL, NULL, NULL, NULL );
+    NTSTATUS status;
+
+todo_wine
+    ok( (char *)cur_params->CommandLine.Buffer > (char *)cur_params &&
+        (char *)cur_params->CommandLine.Buffer < (char *)cur_params + cur_params->Size,
+        "params %p-%p, params->CommandLine.Buffer %p\n",
+        cur_params, (char *)cur_params + cur_params->Size, cur_params->CommandLine.Buffer);
+    size = ((char *)cur_params + cur_params->Size) - (char *)cur_params->CommandLine.Buffer;
+    ok(size > 180, "CommandLine size is too small (%lu bytes)\n", size);
+
+    status = pRtlCreateProcessParameters( ¶ms, &image, NULL, NULL, NULL, NULL,
+                                          NULL, NULL, NULL, NULL );
     ok( !status, "failed %x\n", status );
     if (VirtualQuery( params, &info, sizeof(info) ) && info.AllocationBase == params)
     {
@@ -398,6 +408,15 @@ static void test_process_params(void)
             broken( ((pos + 3) & ~3) == size ), "wrong size %lx/%lx\n", pos, size );
     }
     else ok( broken(TRUE), "environment not inside block\n" ); /* <= win2k3 */
+
+    str = get_params_string( params, ¶ms->CommandLine );
+    ok( (char *)str > (char *)params &&
+        (char *)str < (char *)params + params->Size,
+        "params %p-%p, params->CommandLine.Buffer %p\n",
+        cur_params, (char *)cur_params + params->Size, str);
+    size = ((char *)cur_params + params->Size) - (char *)str;
+    ok(size > 180, "CommandLine size is too small (%lu bytes)\n", size);
+
     pRtlDestroyProcessParameters( params );
 
     status = pRtlCreateProcessParameters( ¶ms, &image, &dummy, &dummy, &dummy, dummy_env,
-- 
2.20.1

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=46484

Your paranoid android.


=== debian9 (32 bit report) ===

ntdll:
env.c:336: Test succeeded inside todo block: params 0x220000-0x22073e, params->CommandLine.Buffer 0x220642

=== debian9 (32 bit French report) ===

ntdll:
env.c:336: Test succeeded inside todo block: params 0x220000-0x22073e, params->CommandLine.Buffer 0x220642

=== debian9 (32 bit Japanese:Japan report) ===

ntdll:
env.c:336: Test succeeded inside todo block: params 0x220000-0x22073e, params->CommandLine.Buffer 0x220642

=== debian9 (32 bit Chinese:China report) ===

ntdll:
env.c:336: Test succeeded inside todo block: params 0x220000-0x22073e, params->CommandLine.Buffer 0x220642

=== debian9 (32 bit WoW report) ===

ntdll:
env.c:336: Test succeeded inside todo block: params 0x220000-0x22073e, params->CommandLine.Buffer 0x220642

=== debian9 (64 bit WoW report) ===

ntdll:
env.c:336: Test succeeded inside todo block: params 0x120000-0x12089e, params->CommandLine.Buffer 0x1207a2

[Dmitry Timoshkov]

Hello,

I accidently noticed that this patch is marked as "Test failure", although
I never received a formal notice about the patch status change (is that a
tracker bug?) or response on wine-devel besides Marvin's automatic reply.
As usually known tesbot failures don't really mean much unless confirmed
by running on a real machine. Is it really the case? Personally I don't
see failures while running the test locally.

-- 
Dmitry.