[PATCH v2 0/2] MR4699: wined3d: Store the resource heap memory pointer directly.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55773 -- v2: wined3d: Store the resource heap memory pointer directly. wined3d: Introduce a new wined3d_resource_get_sysmem helper. https://gitlab.winehq.org/wine/wine/-/merge_requests/4699
From: Rémi Bernon <rbernon(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55773 --- dlls/wined3d/buffer.c | 24 +++++++++++++----------- dlls/wined3d/context_gl.c | 2 +- dlls/wined3d/swapchain.c | 2 +- dlls/wined3d/texture.c | 2 +- dlls/wined3d/wined3d_private.h | 5 +++++ 5 files changed, 21 insertions(+), 14 deletions(-) diff --git a/dlls/wined3d/buffer.c b/dlls/wined3d/buffer.c index 57c6085c10a..219e11b2186 100644 --- a/dlls/wined3d/buffer.c +++ b/dlls/wined3d/buffer.c @@ -536,10 +536,11 @@ static void buffer_conversion_upload(struct wined3d_buffer *buffer, struct wined for (range_idx = 0; range_idx < buffer->dirty_range_count; ++range_idx) { + BYTE *memory = wined3d_resource_get_sysmem(&buffer->resource); start = buffer->dirty_ranges[range_idx].offset; end = start + buffer->dirty_ranges[range_idx].size; - memcpy(data + start, (BYTE *)buffer->resource.heap_memory + start, end - start); + memcpy(data + start, memory + start, end - start); for (i = start / buffer->stride; i < min((end / buffer->stride) + 1, vertex_count); ++i) { for (j = 0; j < buffer->stride;) @@ -626,12 +627,12 @@ BOOL wined3d_buffer_load_location(struct wined3d_buffer *buffer, case WINED3D_LOCATION_SYSMEM: if (buffer->locations & WINED3D_LOCATION_CLEARED) { - memset(buffer->resource.heap_memory, 0, buffer->resource.size); + memset(wined3d_resource_get_sysmem(&buffer->resource), 0, buffer->resource.size); } else { dst.buffer_object = NULL; - dst.addr = buffer->resource.heap_memory; + dst.addr = wined3d_resource_get_sysmem(&buffer->resource); src.buffer_object = buffer->buffer_object; src.addr = NULL; range.offset = 0; @@ -646,13 +647,13 @@ BOOL wined3d_buffer_load_location(struct wined3d_buffer *buffer, /* FIXME: Clear the buffer on the GPU if possible. */ if (!wined3d_buffer_prepare_location(buffer, context, WINED3D_LOCATION_SYSMEM)) return FALSE; - memset(buffer->resource.heap_memory, 0, buffer->resource.size); + memset(wined3d_resource_get_sysmem(&buffer->resource), 0, buffer->resource.size); } dst.buffer_object = buffer->buffer_object; dst.addr = NULL; src.buffer_object = NULL; - src.addr = buffer->resource.heap_memory; + src.addr = wined3d_resource_get_sysmem(&buffer->resource); if (!buffer->conversion_map) { @@ -688,7 +689,7 @@ void *wined3d_buffer_load_sysmem(struct wined3d_buffer *buffer, struct wined3d_c { if (wined3d_buffer_load_location(buffer, context, WINED3D_LOCATION_SYSMEM)) buffer->resource.pin_sysmem = 1; - return buffer->resource.heap_memory; + return wined3d_resource_get_sysmem(&buffer->resource); } DWORD wined3d_buffer_get_memory(struct wined3d_buffer *buffer, struct wined3d_context *context, @@ -718,7 +719,7 @@ DWORD wined3d_buffer_get_memory(struct wined3d_buffer *buffer, struct wined3d_co if (locations & WINED3D_LOCATION_SYSMEM) { data->buffer_object = 0; - data->addr = buffer->resource.heap_memory; + data->addr = wined3d_resource_get_sysmem(&buffer->resource); return WINED3D_LOCATION_SYSMEM; } @@ -1062,14 +1063,14 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc wined3d_buffer_load_location(buffer, context, WINED3D_LOCATION_SYSMEM); buffer->resource.pin_sysmem = 1; } - TRACE("New pointer is %p.\n", resource->heap_memory); + TRACE("New pointer is %p.\n", wined3d_resource_get_sysmem(resource)); } } context_release(context); } - base = buffer->map_ptr ? buffer->map_ptr : resource->heap_memory; + base = buffer->map_ptr ? buffer->map_ptr : wined3d_resource_get_sysmem(resource); *map_ptr = base + offset; TRACE("Returning memory at %p (base %p, offset %u).\n", *map_ptr, base, offset); @@ -1295,7 +1296,7 @@ static void wined3d_buffer_init_data(struct wined3d_buffer *buffer, } else { - memcpy(buffer->resource.heap_memory, data->data, resource->size); + memcpy(wined3d_resource_get_sysmem(&buffer->resource), data->data, resource->size); wined3d_buffer_validate_location(buffer, WINED3D_LOCATION_SYSMEM); wined3d_buffer_invalidate_location(buffer, ~WINED3D_LOCATION_SYSMEM); } @@ -1380,7 +1381,8 @@ static HRESULT wined3d_buffer_init(struct wined3d_buffer *buffer, struct wined3d buffer->locations = WINED3D_LOCATION_CLEARED; TRACE("buffer %p, size %#x, usage %#x, memory @ %p.\n", - buffer, buffer->resource.size, buffer->resource.usage, buffer->resource.heap_memory); + buffer, buffer->resource.size, buffer->resource.usage, + wined3d_resource_get_sysmem(&buffer->resource)); if (device->create_parms.flags & WINED3DCREATE_SOFTWARE_VERTEXPROCESSING || (desc->usage & WINED3DUSAGE_MANAGED)) diff --git a/dlls/wined3d/context_gl.c b/dlls/wined3d/context_gl.c index 55546130d35..00d05af4c5d 100644 --- a/dlls/wined3d/context_gl.c +++ b/dlls/wined3d/context_gl.c @@ -5277,7 +5277,7 @@ void draw_primitive(struct wined3d_device *device, const struct wined3d_state *s struct wined3d_bo *bo = index_buffer->buffer_object; if (!bo || !stream_info->all_vbo) - idx_data = index_buffer->resource.heap_memory; + idx_data = wined3d_resource_get_sysmem(&index_buffer->resource); else idx_data = (void *)bo->buffer_offset; idx_data = (const BYTE *)idx_data + state->index_offset; diff --git a/dlls/wined3d/swapchain.c b/dlls/wined3d/swapchain.c index 8a02847c840..2661fb38023 100644 --- a/dlls/wined3d/swapchain.c +++ b/dlls/wined3d/swapchain.c @@ -426,7 +426,7 @@ static void swapchain_blit_gdi(struct wined3d_swapchain *swapchain, wined3d_texture_load_location(back_buffer, 0, context, WINED3D_LOCATION_SYSMEM); wined3d_texture_get_pitch(back_buffer, 0, &row_pitch, &slice_pitch); - create_desc.pMemory = back_buffer->resource.heap_memory; + create_desc.pMemory = wined3d_resource_get_sysmem(&back_buffer->resource); create_desc.Format = format->ddi_format; create_desc.Width = wined3d_texture_get_level_width(back_buffer, 0); create_desc.Height = wined3d_texture_get_level_height(back_buffer, 0); diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c index 8a7f9455bb0..cac6628687c 100644 --- a/dlls/wined3d/texture.c +++ b/dlls/wined3d/texture.c @@ -754,7 +754,7 @@ void wined3d_texture_get_bo_address(const struct wined3d_texture *texture, } else { - data->addr = texture->resource.heap_memory; + data->addr = wined3d_resource_get_sysmem(&texture->resource); data->addr += sub_resource->offset; } data->buffer_object = 0; diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index c17e29cddf2..9fc5b9b2f16 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -3264,6 +3264,11 @@ void wined3d_resource_memory_colour_fill(struct wined3d_resource *resource, #define RESOURCE_ALIGNMENT 16 #define WINED3D_CONSTANT_BUFFER_ALIGNMENT 16 +static inline void *wined3d_resource_get_sysmem(const struct wined3d_resource *resource) +{ + return resource->heap_memory; +} + #define WINED3D_LOCATION_DISCARDED 0x00000001 #define WINED3D_LOCATION_SYSMEM 0x00000002 #define WINED3D_LOCATION_CLEARED 0x00000004 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4699
From: Rémi Bernon <rbernon(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55773 --- dlls/wined3d/resource.c | 14 ++++---------- dlls/wined3d/wined3d_private.h | 3 ++- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/dlls/wined3d/resource.c b/dlls/wined3d/resource.c index 9d3fd0a426d..c7bf989286c 100644 --- a/dlls/wined3d/resource.c +++ b/dlls/wined3d/resource.c @@ -331,8 +331,7 @@ void CDECL wined3d_resource_preload(struct wined3d_resource *resource) static BOOL wined3d_resource_allocate_sysmem(struct wined3d_resource *resource) { - void **p; - SIZE_T align = RESOURCE_ALIGNMENT - 1 + sizeof(*p); + static const SIZE_T align = RESOURCE_ALIGNMENT - 1; void *mem; if (!(mem = heap_alloc_zero(resource->size + align))) @@ -341,10 +340,7 @@ static BOOL wined3d_resource_allocate_sysmem(struct wined3d_resource *resource) return FALSE; } - p = (void **)(((ULONG_PTR)mem + align) & ~(RESOURCE_ALIGNMENT - 1)) - 1; - *p = mem; - - resource->heap_memory = ++p; + resource->heap_memory = mem; return TRUE; } @@ -359,12 +355,10 @@ BOOL wined3d_resource_prepare_sysmem(struct wined3d_resource *resource) void wined3d_resource_free_sysmem(struct wined3d_resource *resource) { - void **p = resource->heap_memory; - - if (!p) + if (!resource->heap_memory) return; - heap_free(*(--p)); + heap_free(resource->heap_memory); resource->heap_memory = NULL; } diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 9fc5b9b2f16..031e3d7abd6 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -3266,7 +3266,8 @@ void wined3d_resource_memory_colour_fill(struct wined3d_resource *resource, static inline void *wined3d_resource_get_sysmem(const struct wined3d_resource *resource) { - return resource->heap_memory; + static const SIZE_T align = RESOURCE_ALIGNMENT - 1; + return (void *)(((ULONG_PTR)resource->heap_memory + align) & ~align); } #define WINED3D_LOCATION_DISCARDED 0x00000001 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4699
On Fri Dec 15 09:29:00 2023 +0000, Rémi Bernon wrote:
Will have a look at the test failures, I probably messed something up. Fixed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4699#note_56060
This merge request was approved by Jan Sikorski. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4699
Maybe I'm being overly paranoid, but this does introduce some nontrivial calculations in what can be rather fast paths. I'd rather store the pointer separately. Or, at this point, it'd probably be fine to use aligned_alloc(). More to the point, though... is this going to reliably solve the bug? What are we storing in the heap header that's unimportant enough that it can be overwritten? If so, we should have a comment explicitly saying "this has to be a heap allocation so that it can be corrupted", so nobody tries to change this again in the future. If not, then I think we should explicitly work around this somehow. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4699#note_56120
Or, at this point, it'd probably be fine to use aligned_alloc().
But `aligned_malloc` has the same issue, it hides its pointer right before the data.
What are we storing in the heap header that's unimportant enough that it can be overwritten?
It's not unimportant, but it's more robust and it's not as bad as passing a completely invalid pointer to heap_free. The block header might be corrupted, but that's checked and heap_free probably rejects it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4699#note_56124
Could we also reserve a couple bytes at the beginning of the allocation to reliably work around this particular bug? I would think that +/& is trivial enough to not matter outside a really tight loop, but storing separately is fine too as far as I'm concerned. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4699#note_56126
participants (3)
-
Jan Sikorski (＠jsikorski) -
Rémi Bernon
-
Zebediah Figura (＠zfigura)