[PATCH] d3dx9: Improve argument validation in CloneEffect()
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/d3dx9_36/effect.c | 9 +++++ dlls/d3dx9_36/tests/effect.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index f8c480303c..a98495e555 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -4367,6 +4367,15 @@ static HRESULT WINAPI ID3DXEffectImpl_CloneEffect(ID3DXEffect *iface, FIXME("(%p)->(%p, %p): stub\n", This, device, effect); + if (!effect) + return D3DERR_INVALIDCALL; + + if (This->base_effect.flags & D3DXFX_NOT_CLONEABLE) + return E_FAIL; + + if (!device) + return D3DERR_INVALIDCALL; + return E_NOTIMPL; } diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c index 1900f3eac1..28dcf47cd3 100644 --- a/dlls/d3dx9_36/tests/effect.c +++ b/dlls/d3dx9_36/tests/effect.c @@ -7294,6 +7294,91 @@ static void test_effect_null_shader(void) DestroyWindow(window); } +static void test_effect_clone(IDirect3DDevice9 *device) +{ + D3DPRESENT_PARAMETERS present_parameters; + ID3DXEffect *effect, *cloned; + IDirect3DDevice9 *device2; + IDirect3D9 *d3d; + HWND window; + HRESULT hr; + + /* D3DXFX_NOT_CLONEABLE */ + hr = D3DXCreateEffect(device, test_effect_preshader_effect_blob, sizeof(test_effect_preshader_effect_blob), + NULL, NULL, D3DXFX_NOT_CLONEABLE, NULL, &effect, NULL); + ok(hr == D3D_OK, "Got result %#x.\n", hr); + + hr = effect->lpVtbl->CloneEffect(effect, NULL, NULL); + ok(hr == D3DERR_INVALIDCALL, "Got result %#x.\n", hr); + + cloned = (void *)0xdeadbeef; + hr = effect->lpVtbl->CloneEffect(effect, NULL, &cloned); + ok(hr == E_FAIL, "Got result %#x.\n", hr); + ok(cloned == (void *)0xdeadbeef, "Unexpected effect pointer.\n"); + + hr = effect->lpVtbl->CloneEffect(effect, device, NULL); + ok(hr == D3DERR_INVALIDCALL, "Got result %#x.\n", hr); + + cloned = (void *)0xdeadbeef; + hr = effect->lpVtbl->CloneEffect(effect, device, &cloned); + ok(hr == E_FAIL, "Got result %#x.\n", hr); + ok(cloned == (void *)0xdeadbeef, "Unexpected effect pointer.\n"); + + effect->lpVtbl->Release(effect); + + hr = D3DXCreateEffect(device, test_effect_preshader_effect_blob, sizeof(test_effect_preshader_effect_blob), + NULL, NULL, 0, NULL, &effect, NULL); + ok(hr == D3D_OK, "Got result %#x.\n", hr); + + hr = effect->lpVtbl->CloneEffect(effect, NULL, NULL); + ok(hr == D3DERR_INVALIDCALL, "Got result %#x.\n", hr); + + cloned = (void *)0xdeadbeef; + hr = effect->lpVtbl->CloneEffect(effect, NULL, &cloned); + ok(hr == D3DERR_INVALIDCALL, "Got result %#x.\n", hr); + ok(cloned == (void *)0xdeadbeef, "Unexpected effect pointer.\n"); + + hr = effect->lpVtbl->CloneEffect(effect, device, NULL); + ok(hr == D3DERR_INVALIDCALL, "Got result %#x.\n", hr); + + hr = effect->lpVtbl->CloneEffect(effect, device, &cloned); +todo_wine + ok(hr == D3D_OK, "Got result %#x.\n", hr); +if (hr == D3D_OK) +{ + ok(cloned != effect, "Expected new effect instance.\n"); + cloned->lpVtbl->Release(cloned); +} + /* Try with different device. */ + hr = IDirect3DDevice9_GetDirect3D(device, &d3d); + ok(hr == D3D_OK, "Failed to get IDirect3D9 pointer.\n"); + + window = CreateWindowA("static", "d3dx9_test", WS_OVERLAPPEDWINDOW, 0, 0, + 640, 480, NULL, NULL, NULL, NULL); + ok(!!window, "Failed to create a test window.\n"); + + memset(&present_parameters, 0, sizeof(present_parameters)); + present_parameters.Windowed = TRUE; + present_parameters.SwapEffect = D3DSWAPEFFECT_DISCARD; + hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, window, + D3DCREATE_HARDWARE_VERTEXPROCESSING, &present_parameters, &device2); + ok(SUCCEEDED(hr), "Failed to create IDirect3DDevice9 object, hr %#x\n", hr); + + hr = effect->lpVtbl->CloneEffect(effect, device2, &cloned); +todo_wine + ok(hr == D3D_OK, "Got result %#x.\n", hr); +if (hr == D3D_OK) +{ + ok(cloned != effect, "Expected new effect instance.\n"); + cloned->lpVtbl->Release(cloned); +} + IDirect3DDevice9_Release(device2); + IDirect3D9_Release(d3d); + DestroyWindow(window); + + effect->lpVtbl->Release(effect); +} + START_TEST(effect) { HWND wnd; @@ -7346,6 +7431,7 @@ START_TEST(effect) test_effect_large_address_aware_flag(device); test_effect_get_pass_desc(device); test_effect_skip_constants(device); + test_effect_clone(device); refcount = IDirect3DDevice9_Release(device); ok(!refcount, "Device has %u references left.\n", refcount); -- 2.16.1
On 01/03/18 06:14, Nikolay Sivov wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/d3dx9_36/effect.c | 9 +++++ dlls/d3dx9_36/tests/effect.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+)
Just for the record, assuming this patch was pulled from staging, it's apparently needed for the Black and White 2 demo: https://botbot.me/freenode/wine-staging/search/?q=cloneeffect
On 3/1/2018 8:02 PM, Zebediah Figura wrote:
On 01/03/18 06:14, Nikolay Sivov wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/d3dx9_36/effect.c | 9 +++++ dlls/d3dx9_36/tests/effect.c | 86 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+)
Just for the record, assuming this patch was pulled from staging, it's apparently needed for the Black and White 2 demo: https://botbot.me/freenode/wine-staging/search/?q=cloneeffect
Yes, it's actually mentioned in existing bug report, https://bugs.winehq.org/show_bug.cgi?id=25138#c9.
2018-03-01 13:14 GMT+01:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c index 1900f3eac1..28dcf47cd3 100644 --- a/dlls/d3dx9_36/tests/effect.c +++ b/dlls/d3dx9_36/tests/effect.c @@ -7294,6 +7294,91 @@ static void test_effect_null_shader(void) DestroyWindow(window); }
+static void test_effect_clone(IDirect3DDevice9 *device)
Could you make the test create its own device instead, like the last two tests added to the file?
+ memset(&present_parameters, 0, sizeof(present_parameters)); + present_parameters.Windowed = TRUE; + present_parameters.SwapEffect = D3DSWAPEFFECT_DISCARD; + hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, window, + D3DCREATE_HARDWARE_VERTEXPROCESSING, &present_parameters, &device2); + ok(SUCCEEDED(hr), "Failed to create IDirect3DDevice9 object, hr %#x\n", hr); + + hr = effect->lpVtbl->CloneEffect(effect, device2, &cloned); +todo_wine + ok(hr == D3D_OK, "Got result %#x.\n", hr); +if (hr == D3D_OK) +{ + ok(cloned != effect, "Expected new effect instance.\n"); + cloned->lpVtbl->Release(cloned); +}
Here you could also check (via the GetDevice() method) that the cloned effect actually uses the new device. I don't expect surprises though.
On 3/2/2018 3:31 AM, Matteo Bruni wrote:
2018-03-01 13:14 GMT+01:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c index 1900f3eac1..28dcf47cd3 100644 --- a/dlls/d3dx9_36/tests/effect.c +++ b/dlls/d3dx9_36/tests/effect.c @@ -7294,6 +7294,91 @@ static void test_effect_null_shader(void) DestroyWindow(window); }
+static void test_effect_clone(IDirect3DDevice9 *device)
Could you make the test create its own device instead, like the last two tests added to the file?
Does it mean other tests should not reuse it either?
+ memset(&present_parameters, 0, sizeof(present_parameters)); + present_parameters.Windowed = TRUE; + present_parameters.SwapEffect = D3DSWAPEFFECT_DISCARD; + hr = IDirect3D9_CreateDevice(d3d, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, window, + D3DCREATE_HARDWARE_VERTEXPROCESSING, &present_parameters, &device2); + ok(SUCCEEDED(hr), "Failed to create IDirect3DDevice9 object, hr %#x\n", hr); + + hr = effect->lpVtbl->CloneEffect(effect, device2, &cloned); +todo_wine + ok(hr == D3D_OK, "Got result %#x.\n", hr); +if (hr == D3D_OK) +{ + ok(cloned != effect, "Expected new effect instance.\n"); + cloned->lpVtbl->Release(cloned); +}
Here you could also check (via the GetDevice() method) that the cloned effect actually uses the new device. I don't expect surprises though.
Sure, but yes, looks unlikely that it doesn't.
2018-03-02 7:28 GMT+01:00 Nikolay Sivov <bunglehead(a)gmail.com>:
On 3/2/2018 3:31 AM, Matteo Bruni wrote:
2018-03-01 13:14 GMT+01:00 Nikolay Sivov <nsivov(a)codeweavers.com>:
diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c index 1900f3eac1..28dcf47cd3 100644 --- a/dlls/d3dx9_36/tests/effect.c +++ b/dlls/d3dx9_36/tests/effect.c @@ -7294,6 +7294,91 @@ static void test_effect_null_shader(void) DestroyWindow(window); }
+static void test_effect_clone(IDirect3DDevice9 *device)
Could you make the test create its own device instead, like the last two tests added to the file?
Does it mean other tests should not reuse it either?
Ideally they shouldn't, yeah.
participants (4)
-
Matteo Bruni -
Nikolay Sivov -
Nikolay Sivov -
Zebediah Figura