[PATCH 0/3] MR758: d3d10: Test D3D10CreateEffectFromMemory.

[Ziqing Hui (＠zhui)]

The test results show that data passed to D3D10CreateEffectFromMemory won't be modified.

[PATCH 1/3] d3d10/tests: Test data passed to D3D10CreateEffectFromMemory.

[Ziqing Hui]

From: Ziqing Hui

---
 dlls/d3d10/tests/effect.c | 58 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/dlls/d3d10/tests/effect.c b/dlls/d3d10/tests/effect.c
index 3bd6d6d0ecc..6cbe57ac62a 100644
--- a/dlls/d3d10/tests/effect.c
+++ b/dlls/d3d10/tests/effect.c
@@ -8244,6 +8244,63 @@ static void test_effect_fx_4_1(void)
     ok(!refcount, "Device has %lu references left.\n", refcount);
 }

+static void test_effect_data(void)
+{
+    ID3D10EffectPool *pool;
+    ID3D10Effect *effect;
+    ID3D10Device *device;
+    DWORD buffer[1024];
+    ULONG refcount;
+    HRESULT hr;
+
+    if (!(device = create_device()))
+    {
+        skip("Failed to create device, skipping tests.\n");
+        return;
+    }
+
+    memcpy(buffer, fx_test_ecbt, sizeof(fx_test_ecbt));
+
+    if (strcmp(winetest_platform, "wine")) /* Crash on wine. */
+    {
+    hr = create_effect(buffer, 0, NULL, NULL, &effect);
+    ok(hr == D3DERR_INVALIDCALL, "Got unexpected hr %#lx.\n", hr);
+    ok(!memcmp(buffer, fx_test_ecbt, sizeof(fx_test_ecbt)), "Data is not modified.\n");
+    }
+
+    hr = create_effect(buffer, 0, device, NULL, &effect);
+    ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
+    ok(!memcmp(buffer, fx_test_ecbt, sizeof(fx_test_ecbt)), "Data is not modified.\n");
+    effect->lpVtbl->Release(effect);
+
+    hr = create_effect(buffer, D3D10_EFFECT_COMPILE_ALLOW_SLOW_OPS, device, NULL, &effect);
+    ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
+    ok(!memcmp(buffer, fx_test_ecbt, sizeof(fx_test_ecbt)), "Data is not modified.\n");
+    effect->lpVtbl->Release(effect);
+
+    hr = create_effect(buffer, D3D10_EFFECT_SINGLE_THREADED, device, NULL, &effect);
+    ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
+    ok(!memcmp(buffer, fx_test_ecbt, sizeof(fx_test_ecbt)), "Data is not modified.\n");
+    effect->lpVtbl->Release(effect);
+
+    memcpy(buffer, fx_test_pool_child, sizeof(fx_test_pool_child));
+
+    hr = create_effect(buffer, D3D10_EFFECT_COMPILE_CHILD_EFFECT, device, NULL, &effect);
+    ok(hr == E_INVALIDARG, "Got unexpected hr %#lx.\n", hr);
+    ok(!memcmp(buffer, fx_test_pool_child, sizeof(fx_test_pool_child)), "Data is not modified.\n");
+
+    hr = create_effect_pool(fx_test_pool, device, &pool);
+    ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
+    hr = create_effect(buffer, D3D10_EFFECT_COMPILE_CHILD_EFFECT, device, pool, &effect);
+    ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
+    ok(!memcmp(buffer, fx_test_pool_child, sizeof(fx_test_pool_child)), "Data is not modified.\n");
+    effect->lpVtbl->Release(effect);
+    pool->lpVtbl->Release(pool);
+
+    refcount = ID3D10Device_Release(device);
+    ok(!refcount, "Device has %lu references left.\n", refcount);
+}
+
 START_TEST(effect)
 {
     test_effect_constant_buffer_type();
@@ -8271,4 +8328,5 @@ START_TEST(effect)
     test_effect_index_expression();
     test_effect_value_expression();
     test_effect_fx_4_1();
+    test_effect_data();
 }

[PATCH 2/3] d3d10/tests: Test NULL device for D3D10CreateEffectPoolFromMemory.

[Ziqing Hui]

From: Ziqing Hui

---
 dlls/d3d10/tests/effect.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/dlls/d3d10/tests/effect.c b/dlls/d3d10/tests/effect.c
index 6cbe57ac62a..c25d3288b52 100644
--- a/dlls/d3d10/tests/effect.c
+++ b/dlls/d3d10/tests/effect.c
@@ -7117,6 +7117,12 @@ static void test_effect_pool(void)
     todo_wine
     ok(hr == E_INVALIDARG, "Got unexpected hr %#lx.\n", hr);

+    if (strcmp(winetest_platform, "wine")) /* Crash on wine. */
+    {
+    hr = create_effect_pool(fx_test_pool, NULL, &pool);
+    ok(hr == D3DERR_INVALIDCALL, "Got unexpected hr %#lx.\n", hr);
+    }
+
     hr = create_effect_pool(fx_test_pool, device, &pool);
     ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);

[PATCH 3/3] d3d10: Handle invalid arguments for effect creation.

[Ziqing Hui]

From: Ziqing Hui

Passing NULL data to D3D10CreateEffectFromMemory crashes.
Passing NULL data to D3D10CreateEffectPoolFromMemory returns E_INVALIDARG.
---
 dlls/d3d10/effect.c       | 6 ++++++
 dlls/d3d10/tests/effect.c | 7 -------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
index 84ecfdcbbab..a9c8eaed5e8 100644
--- a/dlls/d3d10/effect.c
+++ b/dlls/d3d10/effect.c
@@ -9537,6 +9537,9 @@ static HRESULT d3d10_create_effect(void *data, SIZE_T data_size, ID3D10Device *d
     struct d3d10_effect *object;
     HRESULT hr;

+    if (!device)
+        return D3DERR_INVALIDCALL;
+
     if (!(object = calloc(1, sizeof(*object))))
         return E_OUTOFMEMORY;

@@ -9636,6 +9639,9 @@ HRESULT WINAPI D3D10CreateEffectPoolFromMemory(void *data, SIZE_T data_size, UIN
     TRACE("data %p, data_size %Iu, fx_flags %#x, device %p, effect_pool %p.\n",
             data, data_size, fx_flags, device, effect_pool);

+    if (!data)
+        return E_INVALIDARG;
+
     if (FAILED(hr = d3d10_create_effect(data, data_size, device, NULL,
             D3D10_EFFECT_IS_POOL, &object)))
     {
diff --git a/dlls/d3d10/tests/effect.c b/dlls/d3d10/tests/effect.c
index c25d3288b52..a13ef2bfb91 100644
--- a/dlls/d3d10/tests/effect.c
+++ b/dlls/d3d10/tests/effect.c
@@ -7114,14 +7114,10 @@ static void test_effect_pool(void)
     ok(!!device2, "Failed to create d3d device.\n");

hr = D3D10CreateEffectPoolFromMemory(NULL, 0, 0, device, &pool);
-    todo_wine
     ok(hr == E_INVALIDARG, "Got unexpected hr %#lx.\n", hr);

-    if (strcmp(winetest_platform, "wine")) /* Crash on wine. */
-    {
     hr = create_effect_pool(fx_test_pool, NULL, &pool);
     ok(hr == D3DERR_INVALIDCALL, "Got unexpected hr %#lx.\n", hr);
-    }

hr = create_effect_pool(fx_test_pool, device, &pool);
     ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
@@ -8267,12 +8263,9 @@ static void test_effect_data(void)

memcpy(buffer, fx_test_ecbt, sizeof(fx_test_ecbt));

-    if (strcmp(winetest_platform, "wine")) /* Crash on wine. */
-    {
     hr = create_effect(buffer, 0, NULL, NULL, &effect);
     ok(hr == D3DERR_INVALIDCALL, "Got unexpected hr %#lx.\n", hr);
     ok(!memcmp(buffer, fx_test_ecbt, sizeof(fx_test_ecbt)), "Data is not modified.\n");
-    }

hr = create_effect(buffer, 0, device, NULL, &effect);
     ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/d3d10/tests/effect.c:
> - if (strcmp(winetest_platform, "wine")) /* Crash on wine. */
> - {
> - hr = create_effect(buffer, 0, NULL, NULL, &effect);
> - ok(hr == D3DERR_INVALIDCALL, "Got unexpected hr %#lx.\n", hr);
> - ok(!memcmp(buffer, fx_test_ecbt, sizeof(fx_test_ecbt)), "Data is not modified.\n");
> - }
> -
> - hr = create_effect(buffer, 0, device, NULL, &effect);
> - ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
> - ok(!memcmp(buffer, fx_test_ecbt, sizeof(fx_test_ecbt)), "Data is not modified.\n");
> - effect->lpVtbl->Release(effect);
> -
> - hr = create_effect(buffer, D3D10_EFFECT_COMPILE_ALLOW_SLOW_OPS, device, NULL, &effect);
> - ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
> - ok(!memcmp(buffer, fx_test_ecbt, sizeof(fx_test_ecbt)), "Data is not modified.\n");
> - effect->lpVtbl->Release(effect);

Could you explain, where does this come from? Why would you think that input blob would ever change?

[Matteo Bruni (＠Mystral)]

On Thu Sep  1 09:49:55 2022 +0000, Nikolay Sivov wrote:
> Could you explain, where does this come from? Why would you think that
> input blob would ever change?

This is my fault, see https://gitlab.winehq.org/wine/wine/-/merge_requests/697#note_7369 / https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3151.

I should have mentioned that I didn't expect a Wine test to check for that, just a manual look on the side (in fact I was picturing this as VirtualAlloc(), copy the source there, VirtualProtect(PAGE_READONLY), call D3D10CreateEffectFromMemory(), run on Windows and verify that it doesn't crash). I will probably have a go at it myself.

I don't think that at this point we have a good reason to have the tests from this patch in Wine. Sorry for the extra work that I accidentally had you do.

[Matteo Bruni (＠Mystral)]

Rest of the MR seems fine to me

[Matteo Bruni (＠Mystral)]

On Thu Sep  1 12:07:39 2022 +0000, Matteo Bruni wrote:
> This is my fault, see
> https://gitlab.winehq.org/wine/wine/-/merge_requests/697#note_7369 / https://gitlab.winehq.org/wine/wine/-/merge_requests/332#note_3151.
> I should have mentioned that I didn't expect a Wine test to check for
> that, just a manual look on the side (in fact I was picturing this as
> VirtualAlloc(), copy the source there, VirtualProtect(PAGE_READONLY),
> call D3D10CreateEffectFromMemory(), run on Windows and verify that it
> doesn't crash). I will probably have a go at it myself.
> I don't think that at this point we have a good reason to have the tests
> from this patch in Wine. Sorry for the extra work that I accidentally
> had you do.

FWIW I did the test I mentioned earlier and no surprises, everything works just fine.