[PATCH v4 0/1] MR10278: d3dxof: Return isolated buffers from IDirectXFileData_GetData().
The original d3dxof.dll does it. Space Empires V writes into the returned buffers for some reason and overwrites adjacent data. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49722 -- v4: d3dxof: Return isolated buffers from IDirectXFileData_GetData(). https://gitlab.winehq.org/wine/wine/-/merge_requests/10278
From: Joerg Rueppel <joru.ossdev@gmail.com> The original d3dxof.dll does it. Space Empires V writes into the returned buffers for some reason and overwrites adjacent data. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49722 --- dlls/d3dxof/d3dxof.c | 14 +++++++-- dlls/d3dxof/d3dxof_private.h | 1 + dlls/d3dxof/tests/d3dxof.c | 60 ++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 2 deletions(-) diff --git a/dlls/d3dxof/d3dxof.c b/dlls/d3dxof/d3dxof.c index 250af38c895..44b6a93c2cc 100644 --- a/dlls/d3dxof/d3dxof.c +++ b/dlls/d3dxof/d3dxof.c @@ -548,6 +548,8 @@ static ULONG WINAPI IDirectXFileDataImpl_Release(IDirectXFileData *iface) if (!refcount) { + HeapFree(GetProcessHeap(), 0, data->pdata_user_copy); + if (!data->level && !data->from_ref) { HeapFree(GetProcessHeap(), 0, data->pstrings); @@ -615,6 +617,14 @@ static HRESULT WINAPI IDirectXFileDataImpl_GetData(IDirectXFileData* iface, LPCS if (!pcbSize || !ppvData) return DXFILEERR_BADVALUE; + if (!This->pdata_user_copy && This->pobj->root->pdata && This->pobj->size > 0) + { + This->pdata_user_copy = HeapAlloc(GetProcessHeap(), 0, This->pobj->size); + if (!This->pdata_user_copy) + return DXFILEERR_BADALLOC; + memcpy(This->pdata_user_copy, This->pobj->root->pdata + This->pobj->pos_data, This->pobj->size); + } + if (szMember) { ULONG i; @@ -627,12 +637,12 @@ static HRESULT WINAPI IDirectXFileDataImpl_GetData(IDirectXFileData* iface, LPCS return DXFILEERR_BADDATAREFERENCE; } *pcbSize = This->pobj->members[i].size; - *ppvData = This->pobj->root->pdata + This->pobj->members[i].start; + *ppvData = This->pdata_user_copy + (This->pobj->members[i].start - This->pobj->pos_data); } else { *pcbSize = This->pobj->size; - *ppvData = This->pobj->root->pdata + This->pobj->pos_data; + *ppvData = This->pdata_user_copy; } return DXFILE_OK; diff --git a/dlls/d3dxof/d3dxof_private.h b/dlls/d3dxof/d3dxof_private.h index 9f208dfc9cb..d48a4a5d665 100644 --- a/dlls/d3dxof/d3dxof_private.h +++ b/dlls/d3dxof/d3dxof_private.h @@ -104,6 +104,7 @@ typedef struct { BOOL from_ref; ULONG level; LPBYTE pstrings; + LPBYTE pdata_user_copy; } IDirectXFileDataImpl; typedef struct { diff --git a/dlls/d3dxof/tests/d3dxof.c b/dlls/d3dxof/tests/d3dxof.c index b7b1b1cc502..68fb5cea915 100644 --- a/dlls/d3dxof/tests/d3dxof.c +++ b/dlls/d3dxof/tests/d3dxof.c @@ -366,6 +366,8 @@ static char object_complex[] = "3;;;, 0;;;, 1;;;, 2;;;,\n" "3;;;, 1;;;, 2;;;, 3;;;,\n" "3;;;, 3;;;, 1;;;, 2;;;,\n" +/* for heap alloc test */ +"Vector{0.0;;;, 0.0;;;, 0.0;;;}\n" "}\n"; static char template_using_index_color_lower[] = @@ -1005,6 +1007,63 @@ static void test_complex_object(void) IDirectXFile_Release(dxfile); } +static void test_getdata_separate_heap_alloc(void) +{ + IDirectXFile *dxfile = NULL; + IDirectXFileEnumObject *enum_object; + IDirectXFileData *root_data, *child_data; + IDirectXFileObject *child_obj; + DXFILELOADMEMORY load_info; + HRESULT hr; + size_t distance; + + DWORD root_size, child_size; + void *root_ptr, *child_ptr; + + if (!pDirectXFileCreate) + { + win_skip("DirectXFileCreate is not available\n"); + return; + } + + hr = pDirectXFileCreate(&dxfile); + ok(hr == DXFILE_OK, "Unexpected hr %#lx.\n", hr); + + hr = IDirectXFile_RegisterTemplates(dxfile, templates_complex_object, sizeof(templates_complex_object) - 1); + ok(hr == DXFILE_OK, "Unexpected hr %#lx.\n", hr); + + load_info.lpMemory = object_complex; + load_info.dSize = sizeof(object_complex) - 1; + hr = IDirectXFile_CreateEnumObject(dxfile, &load_info, DXFILELOAD_FROMMEMORY, &enum_object); + ok(hr == DXFILE_OK, "Unexpected hr %#lx.\n", hr); + + hr = IDirectXFileEnumObject_GetNextDataObject(enum_object, &root_data); + ok(hr == DXFILE_OK, "Unexpected hr %#lx.\n", hr); + hr = IDirectXFileData_GetData(root_data, NULL, &root_size, &root_ptr); + ok(hr == DXFILE_OK, "Unexpected hr %#lx.\n", hr); + ok(root_size == 104, "Unexpected root size %lu.\n", root_size); + + hr = IDirectXFileData_GetNextObject(root_data, &child_obj); + ok(hr == DXFILE_OK, "Unexpected hr %#lx.\n", hr); + + hr = IDirectXFileObject_QueryInterface(child_obj, &IID_IDirectXFileData, (void** )&child_data); + ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); + + hr = IDirectXFileData_GetData(child_data, NULL, &child_size, &child_ptr); + ok(hr == DXFILE_OK, "Unexpected hr %#lx.\n", hr); + ok(child_size == 12, "Unexpected child size %lu.\n", child_size); + + distance = (root_ptr > child_ptr) ? (BYTE*) root_ptr - (BYTE*) child_ptr : (BYTE*) child_ptr - (BYTE*) root_ptr; + ok(distance > root_size + child_size, "Unexpected distance %Iu.\n", distance); + + IDirectXFileData_Release(child_data); + IDirectXFileObject_Release(child_obj); + + IDirectXFileData_Release(root_data); + IDirectXFileEnumObject_Release(enum_object); + IDirectXFile_Release(dxfile); +} + static void test_standard_templates(void) { IDirectXFile *dxfile = NULL; @@ -1061,6 +1120,7 @@ START_TEST(d3dxof) test_syntax(); test_syntax_semicolon_comma(); test_complex_object(); + test_getdata_separate_heap_alloc(); test_standard_templates(); test_type_index_color(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10278
On Wed Mar 11 15:59:37 2026 +0000, Matteo Bruni wrote:
I haven't tried but it seems preferable to always allocate and copy the data directly in `GetNextObject()` and then remove the `!data->level &&` condition from `Release()`, so you don't need to add another field to the struct. Thanks for the review. I evaluated your suggestion and stumbled upon how to track potential double allocations when the same data object is reached through multiple refs (via Resolve). pobj-\>pdata would then be non NULL already, so we could skip the second alloc, but how do we know when to free it without adding ref counting to that copy, too? This seems more complicated than the lazy copy with the additional field.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_131855
On Mon Mar 16 14:12:15 2026 +0000, Joerg Rueppel wrote:
The returned sizes are correct as far as I can tell. The game seems to have a bounds bug or is doing something "smart" and gets away with it on windows. At some point it writes into the memory adress in the shared buffer where the pointer to the texture file name is stored and increases it by 1 (in some cases, not all). And when that happens, it's looking for the wrong files on disk (missing the first letter - ing instead of ring, p01 instead of rp01). I think before committing to any particular fix we need to figure out what the game is depending on, exactly i.e. it's very possible that with this fix the game is writing out of the allocated heap and only working "by chance". Which is what Nikolay was getting at, I assume.
How does the game manage to overwrite a child object by messing with a parent object data, on current Wine? In theory that shouldn't happen, since the size returned by `GetData()` doesn't include any children. Assuming the sizes are correct, and the game isn't just stomping out of the allocated area, maybe it's a matter of alignment? I'll mention a possibly helpful tool in the toolbox. Running the game with WINEDEBUG=warn+heap will enable some heap checking and validation code. Sometimes it will simply spew useful warnings about bad heap accesses, but often it will change the behavior if the application is doing something fishy. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_132357
On Wed Mar 11 15:59:37 2026 +0000, Joerg Rueppel wrote:
Thanks for the review. I evaluated your suggestion and stumbled upon how to track potential double allocations when the same data object is reached through multiple refs (via Resolve). pobj-\>pdata would then be non NULL already, so we could skip the second alloc, but how do we know when to free it without adding ref counting to that copy, too? This seems more complicated than the lazy copy with the additional field. I don't think it would necessarily require recounting, but yeah, that sounds like it's getting more complicated than necessary.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_132358
On Mon Mar 16 14:12:15 2026 +0000, Matteo Bruni wrote:
I think before committing to any particular fix we need to figure out what the game is depending on, exactly i.e. it's very possible that with this fix the game is writing out of the allocated heap and only working "by chance". Which is what Nikolay was getting at, I assume. How does the game manage to overwrite a child object by messing with a parent object data, on current Wine? In theory that shouldn't happen, since the size returned by `GetData()` doesn't include any children. Assuming the sizes are correct, and the game isn't just stomping out of the allocated area, maybe it's a matter of alignment? I'll mention a possibly helpful tool in the toolbox. Running the game with WINEDEBUG=warn+heap will enable some heap checking and validation code. Sometimes it will simply spew useful warnings about bad heap accesses, but often it will change the behavior if the application is doing something fishy. The game writes into the heap header at returned_ptr - 4 after calling GetData() on the mesh (increases it by 1) and reduces it by 1 after its done with the rest of the file. So during the load the heap header is corrupted. warn+heap doesn't flag it though and through random chance this doesn't seem to have any adverse effect with the way things are implemented right now in wine. We could allocate more and return an offsetted pointer to protect against this, or add padding in the internal buffer between elements and not do individual allocs for each element.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_133057
On Fri Mar 20 15:08:15 2026 +0000, Matteo Bruni wrote:
I don't think it would necessarily require refcounting, but yeah, that sounds like it's getting more complicated than necessary. Ok
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_133060
On Fri Mar 20 15:07:28 2026 +0000, Joerg Rueppel wrote:
The game writes into the heap header at returned_ptr - 4 after calling GetData() on the mesh (increases it by 1) and reduces it by 1 after its done with the rest of the file. So during the load the heap header is corrupted. warn+heap doesn't flag it though and through random chance this doesn't seem to have any adverse effect with the way things are implemented right now in wine. We could allocate more and return an offsetted pointer to protect against this, or add padding in the internal buffer between elements and not do individual allocs for each element. Interesting! I wonder if the game is abusing d3dxof implementation details, like maybe it's expecting the `IDirectXFileData` object refcount in that place and it's being "efficient" / "smart" by manually incrementing and decrementing it instead of calling `AddRef()` and `Release()`.
It should be possible to test that in a clean way by replicating what the game is doing in the d3dxof tests and e.g. use the existing `get_refcount()` helper to check if that affects the refcount of some d3dxof object. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_133079
participants (3)
-
Joerg Rueppel -
Joerg Rueppel (＠jrueppel)
-
Matteo Bruni (＠Mystral)