[PATCH 0/3] MR4073: msxml3: Stop possible typelib out of bounds access (Coverity)
typelib has an array size of 2 (eg LibXml_Last), so a lookup of IID_NULL will result in a lookup of the third index. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4073
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/msxml3/dispex.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/msxml3/dispex.c b/dlls/msxml3/dispex.c index cd7c2820133..6212fc2976c 100644 --- a/dlls/msxml3/dispex.c +++ b/dlls/msxml3/dispex.c @@ -294,9 +294,9 @@ static dispex_data_t *preprocess_dispex_data(DispatchEx *This) data->funcs = heap_realloc(data->funcs, data->func_cnt * sizeof(func_info_t)); } - qsort(data->funcs, data->func_cnt, sizeof(func_info_t), dispid_cmp); - if(data->funcs) { + qsort(data->funcs, data->func_cnt, sizeof(func_info_t), dispid_cmp); + data->name_table = heap_alloc(data->func_cnt * sizeof(func_info_t*)); for(i=0; i < data->func_cnt; i++) data->name_table[i] = data->funcs+i; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4073
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> typelib has an array size of 2 (eg LibXml_Last), so a lookup of IID_NULL will result in a lookup of the third index. --- dlls/msxml3/dispex.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/msxml3/dispex.c b/dlls/msxml3/dispex.c index 6212fc2976c..ef68cb74745 100644 --- a/dlls/msxml3/dispex.c +++ b/dlls/msxml3/dispex.c @@ -82,7 +82,7 @@ static lib_id_t lib_ids[] = { }; static tid_id_t tid_ids[] = { - { &IID_NULL, LibXml_Last }, + { &IID_NULL, LibXml2 }, { &IID_IXMLDOMAttribute, LibXml2 }, { &IID_IXMLDOMCDATASection, LibXml2 }, { &IID_IXMLDOMComment, LibXml2 }, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4073
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/msxml3/httprequest.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/msxml3/httprequest.c b/dlls/msxml3/httprequest.c index 459466a1234..953f49f1eee 100644 --- a/dlls/msxml3/httprequest.c +++ b/dlls/msxml3/httprequest.c @@ -736,6 +736,7 @@ static HRESULT BindStatusCallback_create(httprequest* This, BindStatusCallback * if (!(ptr = heap_alloc(size))) { heap_free(bsc); + IBindCtx_Release(pbc); return E_OUTOFMEMORY; } WideCharToMultiByte(cp, 0, str, len, ptr, size, NULL, NULL); @@ -754,6 +755,7 @@ static HRESULT BindStatusCallback_create(httprequest* This, BindStatusCallback * { SafeArrayUnaccessData(sa); heap_free(bsc); + IBindCtx_Release(pbc); return hr; } size++; @@ -781,6 +783,7 @@ static HRESULT BindStatusCallback_create(httprequest* This, BindStatusCallback * SafeArrayUnaccessData(sa); heap_free(bsc); + IBindCtx_Release(pbc); return E_OUTOFMEMORY; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4073
Nikolay Sivov (@nsivov) commented about dlls/msxml3/dispex.c:
};
static tid_id_t tid_ids[] = { - { &IID_NULL, LibXml_Last }, + { &IID_NULL, LibXml2 }, { &IID_IXMLDOMAttribute, LibXml2 }, { &IID_IXMLDOMCDATASection, LibXml2 }, { &IID_IXMLDOMComment, LibXml2 }, How can this happen in practice?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4073#note_48320
On Wed Oct 11 10:03:13 2023 +0000, Nikolay Sivov wrote:
How can this happen in practice? I'm not sure if it's possible but it will remove the covering warning.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4073#note_48323
On Wed Oct 11 10:10:41 2023 +0000, Alistair Leslie-Hughes wrote:
I'm not sure if it's possible but it will remove the covering warning. It doesn't appear it can happen in practice. There isn't any calls to use NULL_tid directly. Though NULL_tid is used as a end of loop for various tid_t structures (cdata.c/docfrag.c/etc)
We could just return an error in get_typeinfo for NULL_tid? What would you prefer? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4073#note_49236
On Wed Oct 18 21:10:10 2023 +0000, Alistair Leslie-Hughes wrote:
It doesn't appear it can happen in practice. There isn't any calls to use NULL_tid directly. Though NULL_tid is used as a end of loop for various tid_t structures (cdata.c/docfrag.c/etc) We could just return an error in get_typeinfo for NULL_tid? What would you prefer? You could change the first element of tid_ids from { &IID_NULL, LibXml_Last } to { NULL } to make it more clear that it is never used. Or, if we can't come to a consensus, you could mark the warning as a false positive in Coverity with a link to this discussion.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4073#note_49248
participants (4)
-
Alex Henrie (＠alexhenrie) -
Alistair Leslie-Hughes
-
Alistair Leslie-Hughes (＠alesliehughes)
-
Nikolay Sivov (＠nsivov)