[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
On Fri Mar 20 16:15:03 2026 +0000, Matteo Bruni wrote:
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. The target it writes to is in the COM object. It has no correlation to ref counting. It's either a bug in the game, or intentional misappropriation of allocated memory. Could be a padding area in the com object or something unused or unrelated. Fact is, with individual allocs the game loads its data as intended. Fact is also that with this patch the game will write into the heap header. So we should either allocate 4 bytes extra and return the data with an offset or keep the code as is but add buffer areas between the elements in the data buffer and then _not_ create individual allocs. I'm curious how Wine usually handles cases like that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_137025
On Sat Apr 18 15:14:18 2026 +0000, Joerg Rueppel wrote:
The target it writes to is in the COM object. It has no correlation to ref counting. It's either a bug in the game, or intentional misappropriation of allocated memory. Could be a padding area in the com object or something unused or unrelated. Fact is, with individual allocs the game loads its data as intended. Fact is also that with this patch the game will write into the heap header. So we should either allocate 4 bytes extra and return the data with an offset or keep the code as is but add buffer areas between the elements in the data buffer and then _not_ create individual allocs. I'm curious how Wine usually handles cases like that. Cool, nice sleuthing!
I don't think that there is a general Wine rule, we have to figure out something that makes sense in this particular instance. At a very high level, we don't need (or want) to replicate all the implementation details that don't matter in practice. In this case I think either option is okay, so the extra 4 bytes allocation + offset should be simpler. At any rate, the new test currently in this MR can stay as a `todo_wine` even if we don't end up fixing it, so that we don't forget in case that particular quirk comes back to bite us at some later point. BTW, this reminded me that sometimes the memory location "before" a returned address can be used to store extra data (see e.g. https://gitlab.winehq.org/wine/wine/-/blob/47ad1f1d5724fabd206d8ea2c198fad2b... for one such use I introduced many years ago). I quickly hacked tracing the value at `data[-1]` in the tests, it looks like the lower 16 bits change every run, while the high 16 bits seem to be consistent and appear to be flags of some sort. I think we can leave it at that :sweat_smile: -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_137032
On Sat Apr 18 19:57:40 2026 +0000, Matteo Bruni wrote:
Cool, nice sleuthing! I don't think that there is a general Wine rule, we have to figure out something that makes sense in this particular instance. At a very high level, we don't need (or want) to replicate all the implementation details that don't matter in practice. In this case I think either option is okay, so the extra 4 bytes allocation + offset should be simpler. At any rate, the new test currently in this MR can stay as a `todo_wine` even if we don't end up fixing it, so that we don't forget in case that particular quirk comes back to bite us at some later point. BTW, this reminded me that sometimes the memory location "before" a returned address can be used to store extra data (see e.g. https://gitlab.winehq.org/wine/wine/-/blob/47ad1f1d5724fabd206d8ea2c198fad2b... for one such use I introduced many years ago). I quickly hacked tracing the value at `data[-1]` in the tests, it looks like the lower 16 bits change every run, while the high 16 bits seem to be consistent and appear to be flags of some sort. I think we can leave it at that :sweat_smile: Of course padding with 4 extra bytes just made it crash, so more digging ensued. SE5 is a Delphi program, and it looks like the ptr that is returned here is being coopted as a Delphi dynamic array, which is refcounted. Layout of these things is ptr-8 = 32bit refcount, ptr-4 = 32bit length. ptr = actual data. Theory: The game takes the ptr we give back, adds 4 bytes and then hands it over to delphis dynamic array system. Our returned data starts with a 4 byte vertex counter, so the game is indeed 'smart' in just mapping that to delphis darray layout and reusing the size data that's already in place. That's why we see refcount access to ptr - 4 and not - 8. We also have a use after free here, because after releasing the data object, delphis array system will decrement the refcount once more. If it reaches 0, bad things happen because Delphi then tries to free the memory block with its own memory manager. It stops crashing and works properly when I allocate 12 extra bytes and set the DW ORD before the actual data to at least 2 (so the refcount doesn't reach 0). 12 bytes are probably needed because the write after free otherwise destroys wine's internal heap management on the freed memory block? A very clean solution should not free the data in IDirectXFileDataImpl_Release and do it sometime later. A solution that works right now is allocating the 12 extra bytes because then the write after free doesn't damage anything important. Is the latter one acceptable? Do you see another way?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_138661
On Sat May 2 18:36:26 2026 +0000, Joerg Rueppel wrote:
Of course padding with 4 extra bytes just made it crash, so more digging ensued. SE5 is a Delphi program, and it looks like the ptr that is returned here is being coopted as a Delphi dynamic array, which is refcounted. Layout of these things is ptr-8 = 32bit refcount, ptr-4 = 32bit length. ptr = actual data. Theory: The game takes the ptr we give back, adds 4 bytes and then hands it over to delphis dynamic array system. Our returned data starts with a 4 byte vertex counter, so the game is indeed 'smart' in just mapping that to delphis darray layout and reusing the size data that's already in place. That's why we see refcount access to ptr - 4 and not - 8. We also have a use after free here, because after releasing the data object, delphis array system will decrement the refcount once more. If it reaches 0, bad things happen because Delphi then tries to free the memory block with its own memory manager. It stops crashing and works properly when I allocate 12 extra bytes and set the DWORD before the actual data to at least 2 (so the refcount doesn't reach 0). 12 bytes are probably needed because the write after free otherwise destroys wine's internal heap management on the freed memory block? A very clean solution should not free the data in IDirectXFileDataImpl_Release and do it sometime later. A solution that works right now is allocating the 12 extra bytes because then the write after free doesn't damage anything important. Is the latter one acceptable? Do you see another way? Given all this, it sounds safer adding padding areas in the internal buffer, and not doing the individual allocs. Then the game can write to it as much as it wants after releasing the object. I haven't looked into how involved that path would get though.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_138662
On Sat May 2 18:42:49 2026 +0000, Joerg Rueppel wrote:
Given all this, it sounds safer adding padding areas in the internal buffer, and not doing the individual allocs. Then the game can write to it as much as it wants after releasing the object. I haven't looked into how involved that path would get though. Interesting. Great investigation, again :smile:
Yeah, I still think that adding some padding before the data is the way to go. If fixing that directly at parse time is too complicated we can maybe make it work with separate allocations if we can also fix `IDirectXFileData` refcounting. It turns out that parent nodes apparently are supposed to keep references to their children (see [d3dxof-tests.txt](/uploads/a839f73f0f51d012d899b81850579663/d3dxof-tests.txt), specifically the refcounting tests at the end). If we're lucky, the game releases the various objects in the "proper" order such that there is no use-after-free on Windows, and we can avoid it too by replicating this implementation detail. We usually don't go about matching native to this level of detail, but when applications depend on it we effectively have to. Also I haven't tested the game, so this might be entirely moot. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10278#note_138789
participants (3)
-
Joerg Rueppel -
Joerg Rueppel (＠jrueppel)
-
Matteo Bruni (＠Mystral)