[PATCH 0/1] MR618: d3d11: Validate layout description in d3d_input_layout_create().

[Paul Gofman (＠gofman)]

Fixes black screen in Sweet Transit game.

The game depends on ID3D11Device_CreateInputLayout to fail properly when mandatory VS input is not filled. It is coming from MonoGame engine available as source, here is the place of interest: https://github.com/MonoGame/MonoGame/blob/158c0154ac18ed6102c65e24665c6a080c...

As far as my testing goes:
- missing mandatory inputs cause failure, extra elements in input layout are fine;
- shader type is not checked;
- what is mandatory is unrelated to sysval semantics value in dxbc, it probably just checks for sv_ special names with position being a special case which is still mandatory;
- there are more validation checks which are not implemented in this patch, e. g., for InputSlotClass validity. There is a single todo_wine test for that, but it doesn't cover all the specifics.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/618

[PATCH 1/1] d3d11: Validate layout description in d3d_input_layout_create().

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/d3d11/inputlayout.c |  52 ++++++-----
 dlls/d3d11/tests/d3d11.c | 184 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 216 insertions(+), 20 deletions(-)

diff --git a/dlls/d3d11/inputlayout.c b/dlls/d3d11/inputlayout.c
index 88976785e68..faea1c549f6 100644
--- a/dlls/d3d11/inputlayout.c
+++ b/dlls/d3d11/inputlayout.c
@@ -22,20 +22,10 @@
 
 WINE_DEFAULT_DEBUG_CHANNEL(d3d11);
 
-static struct wined3d_shader_signature_element *shader_find_signature_element(const struct wined3d_shader_signature *s,
-        const char *semantic_name, unsigned int semantic_idx, unsigned int stream_idx)
+static BOOL is_mandatory_shader_input(const struct wined3d_shader_signature_element *e)
 {
-    struct wined3d_shader_signature_element *e = s->elements;
-    unsigned int i;
-
-    for (i = 0; i < s->element_count; ++i)
-    {
-        if (!stricmp(e[i].semantic_name, semantic_name) && e[i].semantic_idx == semantic_idx
-                && e[i].stream_idx == stream_idx)
-            return &e[i];
-    }
-
-    return NULL;
+    /* As tests show, sysval semantic is irrelevant here. */
+    return strnicmp(e->semantic_name, "sv_", 3) || !stricmp(e->semantic_name, "sv_position");
 }
 
 static enum wined3d_input_classification wined3d_input_classification_from_d3d11(D3D11_INPUT_CLASSIFICATION slot_class)
@@ -47,8 +37,10 @@ static HRESULT d3d11_input_layout_to_wined3d_declaration(const D3D11_INPUT_ELEME
         UINT element_count, const void *shader_byte_code, SIZE_T shader_byte_code_length,
         struct wined3d_vertex_element **wined3d_elements)
 {
+    struct wined3d_shader_signature_element *ise;
     struct wined3d_shader_signature is;
-    unsigned int i;
+    const D3D11_INPUT_ELEMENT_DESC *f;
+    unsigned int i, j;
     HRESULT hr;
 
     if (FAILED(hr = wined3d_extract_shader_input_signature_from_dxbc(&is, shader_byte_code, shader_byte_code_length)))
@@ -67,8 +59,8 @@ static HRESULT d3d11_input_layout_to_wined3d_declaration(const D3D11_INPUT_ELEME
     for (i = 0; i < element_count; ++i)
     {
         struct wined3d_vertex_element *e = &(*wined3d_elements)[i];
-        const D3D11_INPUT_ELEMENT_DESC *f = &element_descs[i];
-        struct wined3d_shader_signature_element *element;
+
+        f = &element_descs[i];
 
         e->format = wined3dformat_from_dxgi_format(f->Format);
         e->input_slot = f->InputSlot;
@@ -79,11 +71,29 @@ static HRESULT d3d11_input_layout_to_wined3d_declaration(const D3D11_INPUT_ELEME
         e->method = WINED3D_DECL_METHOD_DEFAULT;
         e->usage = 0;
         e->usage_idx = 0;
+    }
+
+    ise = is.elements;
 
-        if ((element = shader_find_signature_element(&is, f->SemanticName, f->SemanticIndex, 0)))
-            e->output_slot = element->register_idx;
-        else
-            WARN("Unused input element %u.\n", i);
+    for (i = 0; i < is.element_count; ++i)
+    {
+        for (j = 0; j < element_count; ++j)
+        {
+            f = &element_descs[j];
+            if (!stricmp(ise[i].semantic_name, f->SemanticName) && ise[i].semantic_idx == f->SemanticIndex
+                    && !ise[i].stream_idx)
+            {
+                (*wined3d_elements)[j].output_slot = ise[i].register_idx;
+                break;
+            }
+        }
+        if (j == element_count && is_mandatory_shader_input(&ise[i]))
+        {
+            WARN("Input element %s%u not found in shader signature.\n", ise[i].semantic_name, ise[i].semantic_idx);
+            free(*wined3d_elements);
+            free(is.elements);
+            return E_INVALIDARG;
+        }
     }
 
     free(is.elements);
@@ -367,6 +377,8 @@ HRESULT d3d_input_layout_create(struct d3d_device *device,
     struct d3d_input_layout *object;
     HRESULT hr;
 
+    if (!element_descs) return E_INVALIDARG;
+
     if (!(object = calloc(1, sizeof(*object))))
         return E_OUTOFMEMORY;
 
diff --git a/dlls/d3d11/tests/d3d11.c b/dlls/d3d11/tests/d3d11.c
index 5ba6bb7ce14..752a241b7ce 100644
--- a/dlls/d3d11/tests/d3d11.c
+++ b/dlls/d3d11/tests/d3d11.c
@@ -18691,12 +18691,52 @@ static void test_sm5_swapc_instruction(void)
     release_test_context(&test_context);
 }
 
+static void check_layout_element_exclusion(ID3D11Device *device, const D3D11_INPUT_ELEMENT_DESC *elements,
+        unsigned int element_count, const DWORD *shader_code, unsigned int shader_code_size, unsigned int sysval_index,
+        const char *test_context)
+{
+    D3D11_INPUT_ELEMENT_DESC elemets_temp[32];
+    ID3D11InputLayout *layout;
+    unsigned int i;
+    HRESULT hr;
+
+    winetest_push_context(test_context);
+
+    hr = ID3D11Device_CreateInputLayout(device, elements, element_count, shader_code, shader_code_size, &layout);
+    ok(hr == S_OK, "Got hr %#lx.\n", hr);
+    ID3D11InputLayout_Release(layout);
+
+    for (i = 0; i < element_count; ++i)
+    {
+        winetest_push_context("%s excluded", elements[i].SemanticName);
+        memcpy(elemets_temp, elements, sizeof(*elemets_temp) * i);
+        memcpy(elemets_temp + i, elements + i + 1, sizeof(*elemets_temp) * (element_count - i - 1));
+        hr = ID3D11Device_CreateInputLayout(device, elemets_temp, element_count - 1,
+                shader_code, shader_code_size, &layout);
+        if (sysval_index == ~0u || i == sysval_index)
+        {
+            /* sysval semantic. */
+            ok(hr == S_OK, "Got hr %#lx.\n", hr);
+            ID3D11InputLayout_Release(layout);
+        }
+        else
+        {
+            ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr);
+        }
+        winetest_pop_context();
+    }
+
+    winetest_pop_context();
+}
+
 static void test_create_input_layout(void)
 {
     D3D11_INPUT_ELEMENT_DESC layout_desc[] =
     {
         {"POSITION", 0, DXGI_FORMAT_UNKNOWN, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0},
+        {"COLOR", 0, DXGI_FORMAT_B8G8R8A8_UNORM, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0},
     };
+
     ULONG refcount, expected_refcount;
     ID3D11InputLayout *input_layout;
     ID3D11Device *device;
@@ -18719,6 +18759,104 @@ static void test_create_input_layout(void)
         0x0000000f, 0x0300005f, 0x001010f2, 0x00000000, 0x04000067, 0x001020f2, 0x00000000, 0x00000001,
         0x05000036, 0x001020f2, 0x00000000, 0x00101e46, 0x00000000, 0x0100003e,
     };
+
+    static D3D11_INPUT_ELEMENT_DESC vs2_layout_desc[] =
+    {
+        {"CUSTOMDATA", 0, DXGI_FORMAT_R32G32_FLOAT, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+        {"POSITION", 0, DXGI_FORMAT_R32G32_FLOAT, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+        {"SV_Position", 0, DXGI_FORMAT_R32G32_FLOAT, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+        {"COLOR", 0, DXGI_FORMAT_B8G8R8A8_UNORM, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+        {"SV_InstanceID", 0, DXGI_FORMAT_R32_UINT, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+    };
+#if 0
+    float4 main(float4 d : CUSTOMDATA, float4 p1 : POSITION, float4 p2 : SV_Position, float4 c : COLOR,
+                uint iid : SV_InstanceID) : SV_POSITION
+    {
+        return p1;
+    }
+#endif
+    static const DWORD vs2_code[] =
+    {
+        0x43425844, 0xc9f2fb4a, 0x020b2cdc, 0x37111099, 0xa1afa56f, 0x00000001, 0x00000160, 0x00000003,
+        0x0000002c, 0x000000e8, 0x0000011c, 0x4e475349, 0x000000b4, 0x00000005, 0x00000008, 0x00000080,
+        0x00000000, 0x00000000, 0x00000003, 0x00000000, 0x0000000f, 0x0000008b, 0x00000000, 0x00000000,
+        0x00000003, 0x00000001, 0x00000f0f, 0x00000094, 0x00000000, 0x00000000, 0x00000003, 0x00000002,
+        0x0000000f, 0x000000a0, 0x00000000, 0x00000000, 0x00000003, 0x00000003, 0x0000000f, 0x000000a6,
+        0x00000000, 0x00000008, 0x00000001, 0x00000004, 0x00000001, 0x54535543, 0x41444d4f, 0x50004154,
+        0x5449534f, 0x004e4f49, 0x505f5653, 0x7469736f, 0x006e6f69, 0x4f4c4f43, 0x56530052, 0x736e495f,
+        0x636e6174, 0x00444965, 0x4e47534f, 0x0000002c, 0x00000001, 0x00000008, 0x00000020, 0x00000000,
+        0x00000001, 0x00000003, 0x00000000, 0x0000000f, 0x505f5653, 0x5449534f, 0x004e4f49, 0x52444853,
+        0x0000003c, 0x00010040, 0x0000000f, 0x0300005f, 0x001010f2, 0x00000001, 0x04000067, 0x001020f2,
+        0x00000000, 0x00000001, 0x05000036, 0x001020f2, 0x00000000, 0x00101e46, 0x00000001, 0x0100003e,
+    };
+
+#if 0
+float4 main() : SV_POSITION
+{
+    return float4(0, 0, 0, 1);
+}
+#endif
+    static const DWORD vs3_code[] =
+    {
+        0x43425844, 0xcf477c49, 0x343fc59b, 0x52dff555, 0x73ebaa82, 0x00000001, 0x000000b4, 0x00000003,
+        0x0000002c, 0x0000003c, 0x00000070, 0x4e475349, 0x00000008, 0x00000000, 0x00000008, 0x4e47534f,
+        0x0000002c, 0x00000001, 0x00000008, 0x00000020, 0x00000000, 0x00000001, 0x00000003, 0x00000000,
+        0x0000000f, 0x505f5653, 0x5449534f, 0x004e4f49, 0x52444853, 0x0000003c, 0x00010040, 0x0000000f,
+        0x04000067, 0x001020f2, 0x00000000, 0x00000001, 0x08000036, 0x001020f2, 0x00000000, 0x00004002,
+        0x00000000, 0x00000000, 0x00000000, 0x3f800000, 0x0100003e,
+    };
+
+    static D3D11_INPUT_ELEMENT_DESC cs_layout_desc[] =
+    {
+        {"SV_GroupID", 0, DXGI_FORMAT_R32_UINT, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0},
+        {"SV_GroupThreadID", 0, DXGI_FORMAT_R32_UINT, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0},
+        {"SV_DispatchThreadID", 0, DXGI_FORMAT_R32_UINT, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0},
+        {"SV_GroupIndex", 0, DXGI_FORMAT_R32_UINT, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0},
+    };
+#if 0
+    [numthreads(256, 1, 1)]
+    void main(uint p1 : SV_GroupID, uint p2 : SV_GroupThreadID, uint p3 : SV_DispatchThreadID, uint p4 : SV_GroupIndex)
+    {
+    }
+#endif
+    static const DWORD cs_code[] =
+    {
+        0x43425844, 0x4ea5ca3f, 0xc6e47bfe, 0x5409d22b, 0xff4f75b7, 0x00000001, 0x00000074, 0x00000003,
+        0x0000002c, 0x0000003c, 0x0000004c, 0x4e475349, 0x00000008, 0x00000000, 0x00000008, 0x4e47534f,
+        0x00000008, 0x00000000, 0x00000008, 0x58454853, 0x00000020, 0x00050040, 0x00000008, 0x0100086a,
+        0x0400009b, 0x00000100, 0x00000001, 0x00000001, 0x0100003e,
+    };
+
+    static D3D11_INPUT_ELEMENT_DESC ps_layout_desc[] =
+    {
+        {"CUSTOMDATA", 0, DXGI_FORMAT_R32G32_FLOAT, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+        {"POSITION", 0, DXGI_FORMAT_R32G32_FLOAT, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+        {"COLOR", 0, DXGI_FORMAT_B8G8R8A8_UNORM, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+        {"SV_Position", 0, DXGI_FORMAT_R32G32_FLOAT, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+        {"SV_InstanceID", 0, DXGI_FORMAT_R32_UINT, 0, 0, D3D11_INPUT_PER_INSTANCE_DATA, 0},
+    };
+    static const DWORD ps_code[] =
+    {
+#if 0
+    float4 main(float4 d : CUSTOMDATA, float4 p1 : POSITION, float4 p2 : SV_Position, float4 c : COLOR,
+                uint iid : SV_InstanceID) : SV_Target
+    {
+        return c;
+    }
+#endif
+        0x43425844, 0x0138f525, 0x4a391fec, 0xaf43a6b1, 0xab08068f, 0x00000001, 0x0000015c, 0x00000003,
+        0x0000002c, 0x000000e8, 0x0000011c, 0x4e475349, 0x000000b4, 0x00000005, 0x00000008, 0x00000080,
+        0x00000000, 0x00000000, 0x00000003, 0x00000000, 0x0000000f, 0x0000008b, 0x00000000, 0x00000000,
+        0x00000003, 0x00000001, 0x0000000f, 0x00000094, 0x00000000, 0x00000001, 0x00000003, 0x00000002,
+        0x0000000f, 0x000000a0, 0x00000000, 0x00000000, 0x00000003, 0x00000003, 0x00000f0f, 0x000000a6,
+        0x00000000, 0x00000000, 0x00000001, 0x00000004, 0x00000001, 0x54535543, 0x41444d4f, 0x50004154,
+        0x5449534f, 0x004e4f49, 0x505f5653, 0x7469736f, 0x006e6f69, 0x4f4c4f43, 0x56530052, 0x736e495f,
+        0x636e6174, 0x00444965, 0x4e47534f, 0x0000002c, 0x00000001, 0x00000008, 0x00000020, 0x00000000,
+        0x00000000, 0x00000003, 0x00000000, 0x0000000f, 0x545f5653, 0x65677261, 0xabab0074, 0x52444853,
+        0x00000038, 0x00000040, 0x0000000e, 0x03001062, 0x001010f2, 0x00000003, 0x03000065, 0x001020f2,
+        0x00000000, 0x05000036, 0x001020f2, 0x00000000, 0x00101e46, 0x00000003, 0x0100003e,
+    };
+
     static const DXGI_FORMAT vertex_formats[] =
     {
         DXGI_FORMAT_R32G32_FLOAT,
@@ -18758,6 +18896,52 @@ static void test_create_input_layout(void)
         ID3D11InputLayout_Release(input_layout);
     }
 
+    layout_desc->Format = vertex_formats[0];
+
+    hr = ID3D11Device_CreateInputLayout(device, layout_desc, 0, vs_code, sizeof(vs_code), &input_layout);
+    ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr);
+
+    hr = ID3D11Device_CreateInputLayout(device, layout_desc, 0, vs3_code, sizeof(vs3_code), &input_layout);
+    ID3D11InputLayout_Release(input_layout);
+    ok(hr == S_OK, "Got hr %#lx.\n", hr);
+
+    hr = ID3D11Device_CreateInputLayout(device, NULL, 0, vs3_code, sizeof(vs3_code), &input_layout);
+    ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr);
+
+    layout_desc->SemanticIndex = 1;
+    hr = ID3D11Device_CreateInputLayout(device, layout_desc, ARRAY_SIZE(layout_desc),
+            vs_code, sizeof(vs_code), &input_layout);
+    ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr);
+    layout_desc->SemanticIndex = 0;
+
+    layout_desc->InputSlotClass = D3D11_INPUT_PER_INSTANCE_DATA;
+    hr = ID3D11Device_CreateInputLayout(device, layout_desc, ARRAY_SIZE(layout_desc),
+            vs_code, sizeof(vs_code), &input_layout);
+    todo_wine ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr);
+    if (SUCCEEDED(hr))
+        ID3D11InputLayout_Release(input_layout);
+    hr = ID3D11Device_CreateInputLayout(device, layout_desc, 1, vs_code, sizeof(vs_code), &input_layout);
+    ok(hr == S_OK, "Got hr %#lx.\n", hr);
+    ID3D11InputLayout_Release(input_layout);
+    layout_desc->InputSlotClass = D3D11_INPUT_PER_VERTEX_DATA;
+
+    layout_desc->SemanticName = "SV_POSITION";
+    hr = ID3D11Device_CreateInputLayout(device, layout_desc, ARRAY_SIZE(layout_desc),
+            vs_code, sizeof(vs_code), &input_layout);
+    ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr);
+
+    layout_desc->SemanticName = "POSITION2";
+    hr = ID3D11Device_CreateInputLayout(device, layout_desc, ARRAY_SIZE(layout_desc),
+            vs_code, sizeof(vs_code), &input_layout);
+    ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr);
+
+    check_layout_element_exclusion(device, vs2_layout_desc, ARRAY_SIZE(vs2_layout_desc), vs2_code,
+            sizeof(vs2_code), ARRAY_SIZE(vs2_layout_desc) - 1, "vs2");
+    check_layout_element_exclusion(device, cs_layout_desc, ARRAY_SIZE(cs_layout_desc), cs_code,
+            sizeof(cs_code), ~0u, "cs");
+    check_layout_element_exclusion(device, ps_layout_desc, ARRAY_SIZE(ps_layout_desc), ps_code,
+            sizeof(ps_code), ARRAY_SIZE(ps_layout_desc) - 1, "ps");
+
     refcount = ID3D11Device_Release(device);
     ok(!refcount, "Device has %lu references left.\n", refcount);
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/618

[PATCH 1/1] d3d11: Validate layout description in d3d_input_layout_create().

[Zebediah Figura (she/her)]

On 8/9/22 13:13, Paul Gofman wrote:
> +static BOOL is_mandatory_shader_input(const struct wined3d_shader_signature_element *e)

Nitpick, but this feels like a slightly awkward name—it doesn't really 
tell you what it's mandatory for (or what kind of input it's measuring).

I kind of want to invert it and call it "is_sysval_semantic", since 
that's the idea, even though in practice it's not using the criteria you 
would expect.
> +        for (j = 0; j < element_count; ++j)
> +        {
> +            f = &element_descs[j];
> +            if (!stricmp(ise[i].semantic_name, f->SemanticName) && ise[i].semantic_idx == f->SemanticIndex
> +                    && !ise[i].stream_idx)
> +            {
> +                (*wined3d_elements)[j].output_slot = ise[i].register_idx;
> +                break;
> +            }
> +        }
> +        if (j == element_count && is_mandatory_shader_input(&ise[i]))
> +        {
> +            WARN("Input element %s%u not found in shader signature.\n", ise[i].semantic_name, ise[i].semantic_idx);
> +            free(*wined3d_elements);
> +            free(is.elements);
> +            return E_INVALIDARG;
> +        }

A matter of personal style, but I prefer to use a helper function rather 
than this for-if pattern.
> +    D3D11_INPUT_ELEMENT_DESC elemets_temp[32];

Typo, "elemets".
> +    ID3D11InputLayout *layout;
> +    unsigned int i;
> +    HRESULT hr;
> +
> +    winetest_push_context(test_context);
> +
> +    hr = ID3D11Device_CreateInputLayout(device, elements, element_count, shader_code, shader_code_size, &layout);
> +    ok(hr == S_OK, "Got hr %#lx.\n", hr);
> +    ID3D11InputLayout_Release(layout);
> +
> +    for (i = 0; i < element_count; ++i)
> +    {
> +        winetest_push_context("%s excluded", elements[i].SemanticName);
> +        memcpy(elemets_temp, elements, sizeof(*elemets_temp) * i);
> +        memcpy(elemets_temp + i, elements + i + 1, sizeof(*elemets_temp) * (element_count - i - 1));
> +        hr = ID3D11Device_CreateInputLayout(device, elemets_temp, element_count - 1,
> +                shader_code, shader_code_size, &layout);
> +        if (sysval_index == ~0u || i == sysval_index)
> +        {
> +            /* sysval semantic. */
> +            ok(hr == S_OK, "Got hr %#lx.\n", hr);
> +            ID3D11InputLayout_Release(layout);

This is a bit weird, because my natural reaction is, "well, of course 
you get S_OK; those are sysval semantics and you're not supposed to find 
them in the input layout". If anything it's interesting to test that 
it's legal to specify them anyway.

That said, the "sysval_index" thing is slightly harder to read, perhaps 
you could just key this on the input element name and copy the d3d11 
function here?
> +    layout_desc->Format = vertex_formats[0];

The existing code does this, but if it's an array with multiple elements 
could you please use "layout_desc[0]." rather than "layout_desc->"?