[PATCH v2 0/1] MR8214: d3dx9_36: Set shared parameters pointer to NULL when freeing it.

[Yuxuan Shui (＠yshui)]

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

[PATCH v2 1/1] d3dx9_36: Set shared parameters pointer to NULL when freeing it.

[Yuxuan Shui]

From: Yuxuan Shui

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;

[Panayiotis Talianos (＠panintended)]

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;
```

[Panayiotis Talianos (＠panintended)]

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

[Yuxuan Shui (＠yshui)]

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`?
>         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

[Panayiotis Talianos (＠panintended)]

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.

[Matteo Bruni (＠Mystral)]

On Tue Jun 10 12:36:47 2025 +0000, Panayiotis Talianos wrote:
> I had another look:
> - 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.

[Matteo Bruni (＠Mystral)]

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