[PATCH 0/1] MR4768: ole32: Fix a possible use-after-free (Coverity).
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4768
From: Zhiyi Zhang <zzhang(a)codeweavers.com> 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4768
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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4768#note_62045
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? 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? You're right. Thanks. Closing.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4768#note_62050
This merge request was closed by Zhiyi Zhang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4768
participants (3)
-
Rémi Bernon -
Zhiyi Zhang
-
Zhiyi Zhang (＠zhiyi)