[PATCH v2 0/1] MR8214: d3dx9_36: Set shared parameters pointer to NULL when freeing it.
This pointer might later be passed to _realloc in d3dx_pool_sync_shared_parameter. -- v2: d3dx9_36: Set shared parameters pointer to NULL when freeing it. https://gitlab.winehq.org/wine/wine/-/merge_requests/8214
From: Yuxuan Shui <yshui(a)codeweavers.com> This pointer might later be passed to _realloc in d3dx_pool_sync_shared_parameter. --- dlls/d3dx9_36/effect.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 4c655a99736..72e216d3fff 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -1943,6 +1943,7 @@ static void d3dx_pool_release_shared_parameter(struct d3dx_top_level_parameter * else { free(param->shared_data->parameters); + param->shared_data->parameters = NULL; /* Zeroing table size is required as the entry in pool parameters table can be reused. */ param->shared_data->size = 0; param->shared_data = NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8214
Panayiotis Talianos (@panintended) commented about dlls/d3dx9_36/effect.c:
else { free(param->shared_data->parameters); + param->shared_data->parameters = NULL; /* Zeroing table size is required as the entry in pool parameters table can be reused. */ param->shared_data->size = 0; param->shared_data = NULL;
@yshui Shouldn't the below also be `free`d and set to `NULL`? ```c free(param->shared_data->data); param->shared_data->data = NULL; free(param->shared_data); param->shared_data = NULL; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8214#note_105525
I was going to commit this change today :smile: FYI, it addresses these bugs: https://bugs.winehq.org/show_bug.cgi?id=55593 https://bugs.winehq.org/show_bug.cgi?id=57230 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8214#note_105526
On Thu Jun 5 14:55:39 2025 +0000, Panayiotis Talianos wrote:
@yshui Shouldn't the below also be `free`d and set to `NULL`? ```c free(param->shared_data->data); param->shared_data->data = NULL; free(param->shared_data); param->shared_data = NULL; ``` i'll admit i don't fully understand the code. but i think this shared_data may be reused later? that's why the comment is talking about zeroing the table size.
i am probably wrong though. cc @Mystral -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8214#note_105555
On Thu Jun 5 18:40:10 2025 +0000, Yuxuan Shui wrote:
i'll admit i don't fully understand the code. but i think this shared_data may be reused later? that's why the comment is talking about zeroing the table size. i am probably wrong though. cc @Mystral I had another look:
1. You are right about `param->shared_data`; it should not be `free()`d while the pool is still in use. It can be accessed next time `d3dx_pool_sync_shared_parameter()` reaches the line `if (!pool->shared_data[i].count)` which will cause a crash. 2. `param->shared_data->data` also should not be `free()`d here. Currently, only the line `pool->shared_data[i].data = param->param.data;` is setting its value. So, `free()`ing that bit of memory should be left to `free_parameter()`. This is called right after `d3dx_pool_release_shared_parameter()` returns. They are both called by `free_top_level_parameter()`, which is also the only function that calls `d3dx_pool_release_shared_parameter()`. So `param->shared_data->data` can be set to `NULL` here. 3. I've noticed, however, that even `free_parameter()` will not set `param->param.data` to `NULL`. The only place I see `d3dx_parameter::data` being set to `NULL` is `param_zero_data_func()` which is only called under certain conditions. That being said, I guess this is a story for another ~~day~~ MR :smile: Looking forward to @Mystral 's input. Good night. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8214#note_105556
On Tue Jun 10 12:36:47 2025 +0000, Panayiotis Talianos wrote:
I had another look: 1. You are right about `param->shared_data`; it should not be `free()`d while the pool is still in use. It can be accessed next time `d3dx_pool_sync_shared_parameter()` reaches the line `if (!pool->shared_data[i].count)` which will cause a crash. 2. `param->shared_data->data` also should not be `free()`d here. Currently, only the line `pool->shared_data[i].data = param->param.data;` is setting its value. So, `free()`ing that bit of memory should be left to `free_parameter()`. This is called right after `d3dx_pool_release_shared_parameter()` returns. They are both called by `free_top_level_parameter()`, which is also the only function that calls `d3dx_pool_release_shared_parameter()`. So `param->shared_data->data` can be set to `NULL` here. 3. I've noticed, however, that even `free_parameter()` will not set `param->param.data` to `NULL`. The only place I see `d3dx_parameter::data` being set to `NULL` is `param_zero_data_func()` which is only called under certain conditions. That being said, I guess this is a story for another ~~day~~ MR :smile: Looking forward to @Mystral 's input. Good night. I think Yuxuan is correct. It's surely convoluted, but yeah, over a number of effect creations and releases, table entries could become unused and eventually be reused. The conditions for that to happen are pretty specific (basically, all effects with the same `pool` that have some `shared` parameter are released, then an effect is created on the same `pool` with a new `shared` parameter) but not impossible, as those WineHQ bugs demonstrate...
We do certainly need to set the parameter to `NULL` though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8214#note_106038
Thanks for the MR! Could you please humor me for a couple of touch ups: - Add the two bug references as `Wine-Bug` - Use `d3dx9: ` in the patch subject -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8214#note_106040
participants (4)
-
Matteo Bruni (＠Mystral) -
Panayiotis Talianos (＠panintended)
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)