[PATCH 0/1] MR4768: ole32: Fix a possible use-after-free (Coverity).

[Zhiyi Zhang (＠zhiyi)]

moniker_tree_get_rightmost(root) can return the same pointer as the root parameter so node can
equal to root. moniker_tree_discard(node) frees node, which could be same as root. Then
moniker_create_from_tree(root) could access the already freed pointer.

[PATCH 1/1] ole32: Fix a possible use-after-free (Coverity).

[Zhiyi Zhang]

From: Zhiyi Zhang

moniker_tree_get_rightmost(root) can return the same pointer as the root parameter so node can
equal to root. moniker_tree_discard(node) frees node, which could be same as root. Then
moniker_create_from_tree(root) could access the already freed pointer.
---
 dlls/ole32/compositemoniker.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/dlls/ole32/compositemoniker.c b/dlls/ole32/compositemoniker.c
index d322604adce..bbdf8975f38 100644
--- a/dlls/ole32/compositemoniker.c
+++ b/dlls/ole32/compositemoniker.c
@@ -1632,7 +1632,8 @@ static HRESULT composite_get_rightmost(CompositeMonikerImpl *composite, IMoniker

*rightmost = node->moniker;
     IMoniker_AddRef(*rightmost);
-    moniker_tree_discard(node, TRUE);
+    if (node != root)
+        moniker_tree_discard(node, TRUE);

hr = moniker_create_from_tree(root, &count, left);
     moniker_tree_release(root);

[Rémi Bernon (＠rbernon)]

I don't know the code at all but this looks like a false positive?

1) `composite_get_rightmost` calls `moniker_get_tree_representation` with the composite iface directly.

2) In `moniker_get_tree_representation` then `unsafe_impl_from_IMoniker(moniker) != NULL` is always true. and so it calls:

```
        moniker_get_tree_representation(comp_moniker->left, node, &node->left);
        moniker_get_tree_representation(comp_moniker->right, node, &node->right);
```

3) This fills both left and right nodes (well unless `calloc` fails but lets ignore it as errors are swallowed anyway).

4) `moniker_tree_get_rightmost` then will always have a left and right node and will always return a node which is different from root?

[Zhiyi Zhang (＠zhiyi)]

On Tue Feb 20 14:13:29 2024 +0000, Rémi Bernon wrote:
> I don't know the code at all but this looks like a false positive?
> - `composite_get_rightmost` calls `moniker_get_tree_representation`
>
> with the composite iface directly.
> 2) In `moniker_get_tree_representation` then
> `unsafe_impl_from_IMoniker(moniker) != NULL` is always true. and so it calls:
>         moniker_get_tree_representation(comp_moniker->left, node, &node->left);
>         moniker_get_tree_representation(comp_moniker->right, node, &node->right);
> - This fills both left and right nodes (well unless `calloc` fails but
>
> lets ignore it as errors are swallowed anyway).
> 4) `moniker_tree_get_rightmost` then will always have a left and right
> node and will always return a node which is different from root?

You're right. Thanks. Closing.

[PATCH 0/1] MR4768: ole32: Fix a possible use-after-free (Coverity). - closed

[Zhiyi Zhang (＠zhiyi)]

This merge request was closed by Zhiyi Zhang.