[PATCH 0/3] MR7375: dlls/oleaut32: Consistently use TLB_REF_NOT_FOUND
Trivial, small set of cleanups patches to start with, non-controversial and NOP functionally speaking. These are to help get the review process going in making ways towards the bar being lowered for contributing to the oleaut32 implementation. Signed-off-by: Edward O'Callaghan <edward(a)antitrust.cc> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7375
From: Edward O'Callaghan <edward(a)antitrust.cc> Signed-off-by: Edward O'Callaghan <edward(a)antitrust.cc> --- dlls/oleaut32/typelib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/oleaut32/typelib.c b/dlls/oleaut32/typelib.c index 74fd03cb787..bfe2e6d2930 100644 --- a/dlls/oleaut32/typelib.c +++ b/dlls/oleaut32/typelib.c @@ -1448,7 +1448,7 @@ static void dump_TLBFuncDescOne(const TLBFuncDesc * pfd) MESSAGE("\thelpstring: %s\n", debugstr_w(TLB_get_bstr(pfd->HelpString))); if(pfd->Entry == NULL) MESSAGE("\tentry: (null)\n"); - else if(pfd->Entry == (void*)-1) + else if(pfd->Entry == TLB_REF_NOT_FOUND) MESSAGE("\tentry: invalid\n"); else if(IS_INTRESOURCE(pfd->Entry)) MESSAGE("\tentry: %p\n", pfd->Entry); @@ -7767,7 +7767,7 @@ static HRESULT WINAPI ITypeInfo_fnGetDllEntry( ITypeInfo2 *iface, MEMBERID memid if (pBstrDllName) *pBstrDllName = SysAllocString(TLB_get_bstr(This->DllName)); - if (!IS_INTRESOURCE(pFDesc->Entry) && (pFDesc->Entry != (void*)-1)) + if (!IS_INTRESOURCE(pFDesc->Entry) && (pFDesc->Entry != TLB_REF_NOT_FOUND)) { if (pBstrName) *pBstrName = SysAllocString(TLB_get_bstr(pFDesc->Entry)); if (pwOrdinal) *pwOrdinal = -1; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7375
From: Edward O'Callaghan <edward(a)antitrust.cc> Signed-off-by: Edward O'Callaghan <edward(a)antitrust.cc> --- dlls/oleaut32/typelib.c | 96 +++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 47 deletions(-) diff --git a/dlls/oleaut32/typelib.c b/dlls/oleaut32/typelib.c index bfe2e6d2930..c54bb1e957d 100644 --- a/dlls/oleaut32/typelib.c +++ b/dlls/oleaut32/typelib.c @@ -2020,17 +2020,17 @@ static HRESULT TLB_size_instance(ITypeInfoImpl *info, SYSKIND sys, static inline void MSFT_Seek(TLBContext *pcx, LONG where) { - if (where != DO_NOT_SEEK) + if (where == DO_NOT_SEEK) + return; + + where += pcx->oStart; + if (where > pcx->length) { - where += pcx->oStart; - if (where > pcx->length) - { - /* FIXME */ - ERR("seek beyond end (%ld/%d)\n", where, pcx->length ); - TLB_abort(); - } - pcx->pos = where; + /* FIXME */ + ERR("seek beyond end (%ld/%d)\n", where, pcx->length ); + TLB_abort(); } + pcx->pos = where; } /* read function */ @@ -3215,16 +3215,17 @@ static ULONG WINAPI TLB_Mapping_Release(IUnknown *iface) { TLB_Mapping *This = mapping_impl_from_IUnknown(iface); ULONG refs = InterlockedDecrement(&This->refs); - if (!refs) - { - if (This->typelib_base) - UnmapViewOfFile(This->typelib_base); - if (This->mapping) - CloseHandle(This->mapping); - if (This->file != INVALID_HANDLE_VALUE) - CloseHandle(This->file); - free(This); - } + if (refs) + return refs; + + if (This->typelib_base) + UnmapViewOfFile(This->typelib_base); + if (This->mapping) + CloseHandle(This->mapping); + if (This->file != INVALID_HANDLE_VALUE) + CloseHandle(This->file); + free(This); + return refs; } @@ -5592,21 +5593,20 @@ static const ITypeCompVtbl tlbtcvt = /*================== ITypeInfo(2) Methods ===================================*/ static ITypeInfoImpl* ITypeInfoImpl_Constructor(void) { - ITypeInfoImpl *pTypeInfoImpl; + ITypeInfoImpl *pTypeInfoImpl = calloc(1, sizeof(ITypeInfoImpl)); + if (!pTypeInfoImpl) + return NULL; + + pTypeInfoImpl->ITypeInfo2_iface.lpVtbl = &tinfvt; + pTypeInfoImpl->ITypeComp_iface.lpVtbl = &tcompvt; + pTypeInfoImpl->ICreateTypeInfo2_iface.lpVtbl = &CreateTypeInfo2Vtbl; + pTypeInfoImpl->ref = 0; + pTypeInfoImpl->hreftype = -1; + pTypeInfoImpl->typeattr.memidConstructor = MEMBERID_NIL; + pTypeInfoImpl->typeattr.memidDestructor = MEMBERID_NIL; + pTypeInfoImpl->pcustdata_list = &pTypeInfoImpl->custdata_list; + list_init(pTypeInfoImpl->pcustdata_list); - pTypeInfoImpl = calloc(1, sizeof(ITypeInfoImpl)); - if (pTypeInfoImpl) - { - pTypeInfoImpl->ITypeInfo2_iface.lpVtbl = &tinfvt; - pTypeInfoImpl->ITypeComp_iface.lpVtbl = &tcompvt; - pTypeInfoImpl->ICreateTypeInfo2_iface.lpVtbl = &CreateTypeInfo2Vtbl; - pTypeInfoImpl->ref = 0; - pTypeInfoImpl->hreftype = -1; - pTypeInfoImpl->typeattr.memidConstructor = MEMBERID_NIL; - pTypeInfoImpl->typeattr.memidDestructor = MEMBERID_NIL; - pTypeInfoImpl->pcustdata_list = &pTypeInfoImpl->custdata_list; - list_init(pTypeInfoImpl->pcustdata_list); - } TRACE("(%p)\n", pTypeInfoImpl); return pTypeInfoImpl; } @@ -5633,13 +5633,15 @@ static HRESULT WINAPI ITypeInfo_fnQueryInterface( else if(IsEqualIID(riid, &IID_ITypeComp)) *ppvObject = &This->ITypeComp_iface; - if(*ppvObject){ - IUnknown_AddRef((IUnknown*)*ppvObject); - TRACE("-- Interface: (%p)->(%p)\n",ppvObject,*ppvObject); - return S_OK; + if(!*ppvObject){ + TRACE("-- Interface: E_NOINTERFACE\n"); + return E_NOINTERFACE; } - TRACE("-- Interface: E_NOINTERFACE\n"); - return E_NOINTERFACE; + + IUnknown_AddRef((IUnknown*)*ppvObject); + TRACE("-- Interface: (%p)->(%p)\n",ppvObject,*ppvObject); + + return S_OK; } static ULONG WINAPI ITypeInfo_fnAddRef( ITypeInfo2 *iface) @@ -5713,17 +5715,17 @@ static ULONG WINAPI ITypeInfo_fnRelease(ITypeInfo2 *iface) { ITypeInfoImpl *This = impl_from_ITypeInfo2(iface); ULONG ref = InterlockedDecrement(&This->ref); + BOOL not_attached_to_typelib = This->not_attached_to_typelib; TRACE("%p, refcount %lu.\n", iface, ref); - if (!ref) - { - BOOL not_attached_to_typelib = This->not_attached_to_typelib; - ITypeLib2_Release(&This->pTypeLib->ITypeLib2_iface); - if (not_attached_to_typelib) - free(This); - /* otherwise This will be freed when typelib is freed */ - } + if (ref) + return ref; + + ITypeLib2_Release(&This->pTypeLib->ITypeLib2_iface); + if (not_attached_to_typelib) + free(This); + /* otherwise This will be freed when typelib is freed */ return ref; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7375
From: Edward O'Callaghan <edward(a)antitrust.cc> Handle base condition first, returning as required or else follow normal procedural flow. Signed-off-by: Edward O'Callaghan <edward(a)antitrust.cc> --- dlls/oleaut32/typelib.c | 109 ++++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 55 deletions(-) diff --git a/dlls/oleaut32/typelib.c b/dlls/oleaut32/typelib.c index c54bb1e957d..ada88ce61c7 100644 --- a/dlls/oleaut32/typelib.c +++ b/dlls/oleaut32/typelib.c @@ -4788,6 +4788,10 @@ static ULONG WINAPI ITypeLib2_fnAddRef( ITypeLib2 *iface) static ULONG WINAPI ITypeLib2_fnRelease( ITypeLib2 *iface) { ITypeLibImpl *This = impl_from_ITypeLib2(iface); + TLBImpLib *pImpLib, *pImpLibNext; + TLBRefType *ref_type, *ref_type_next; + TLBString *tlbstr, *tlbstr_next; + TLBGuid *tlbguid, *tlbguid_next; ULONG ref; EnterCriticalSection(&cache_section); @@ -4795,72 +4799,67 @@ static ULONG WINAPI ITypeLib2_fnRelease( ITypeLib2 *iface) TRACE("%p, refcount %lu.\n", iface, ref); - if (!ref) - { - TLBImpLib *pImpLib, *pImpLibNext; - TLBRefType *ref_type, *ref_type_next; - TLBString *tlbstr, *tlbstr_next; - TLBGuid *tlbguid, *tlbguid_next; - int i; - - /* remove cache entry */ - if(This->path) - { - TRACE("removing from cache list\n"); - if(This->entry.next) - list_remove(&This->entry); - free(This->path); - } - TRACE(" destroying ITypeLib(%p)\n",This); + if (ref) { + LeaveCriticalSection(&cache_section); + return ref; + } - LIST_FOR_EACH_ENTRY_SAFE(tlbstr, tlbstr_next, &This->string_list, TLBString, entry) { - list_remove(&tlbstr->entry); - SysFreeString(tlbstr->str); - free(tlbstr); - } + /* remove cache entry */ + if(This->path) { + TRACE("removing from cache list\n"); + if(This->entry.next) + list_remove(&This->entry); + free(This->path); + } + TRACE(" destroying ITypeLib(%p)\n",This); - LIST_FOR_EACH_ENTRY_SAFE(tlbstr, tlbstr_next, &This->name_list, TLBString, entry) { - list_remove(&tlbstr->entry); - SysFreeString(tlbstr->str); - free(tlbstr); - } + LIST_FOR_EACH_ENTRY_SAFE(tlbstr, tlbstr_next, &This->string_list, TLBString, entry) { + list_remove(&tlbstr->entry); + SysFreeString(tlbstr->str); + free(tlbstr); + } - LIST_FOR_EACH_ENTRY_SAFE(tlbguid, tlbguid_next, &This->guid_list, TLBGuid, entry) { - list_remove(&tlbguid->entry); - free(tlbguid); - } + LIST_FOR_EACH_ENTRY_SAFE(tlbstr, tlbstr_next, &This->name_list, TLBString, entry) { + list_remove(&tlbstr->entry); + SysFreeString(tlbstr->str); + free(tlbstr); + } - TLB_FreeCustData(&This->custdata_list); + LIST_FOR_EACH_ENTRY_SAFE(tlbguid, tlbguid_next, &This->guid_list, TLBGuid, entry) { + list_remove(&tlbguid->entry); + free(tlbguid); + } - for (i = 0; i < This->ctTypeDesc; i++) - if (This->pTypeDesc[i].vt == VT_CARRAY) - free(This->pTypeDesc[i].lpadesc); + TLB_FreeCustData(&This->custdata_list); - free(This->pTypeDesc); + for (int i = 0; i < This->ctTypeDesc; i++) { + if (This->pTypeDesc[i].vt == VT_CARRAY) + free(This->pTypeDesc[i].lpadesc); + } + free(This->pTypeDesc); - LIST_FOR_EACH_ENTRY_SAFE(pImpLib, pImpLibNext, &This->implib_list, TLBImpLib, entry) - { - if (pImpLib->pImpTypeLib) - ITypeLib2_Release(&pImpLib->pImpTypeLib->ITypeLib2_iface); - SysFreeString(pImpLib->name); + LIST_FOR_EACH_ENTRY_SAFE(pImpLib, pImpLibNext, &This->implib_list, TLBImpLib, entry) + { + if (pImpLib->pImpTypeLib) + ITypeLib2_Release(&pImpLib->pImpTypeLib->ITypeLib2_iface); + SysFreeString(pImpLib->name); - list_remove(&pImpLib->entry); - free(pImpLib); - } + list_remove(&pImpLib->entry); + free(pImpLib); + } - LIST_FOR_EACH_ENTRY_SAFE(ref_type, ref_type_next, &This->ref_list, TLBRefType, entry) - { - list_remove(&ref_type->entry); - free(ref_type); - } + LIST_FOR_EACH_ENTRY_SAFE(ref_type, ref_type_next, &This->ref_list, TLBRefType, entry) + { + list_remove(&ref_type->entry); + free(ref_type); + } - for (i = 0; i < This->TypeInfoCount; ++i){ - free(This->typeinfos[i]->tdescAlias); - ITypeInfoImpl_Destroy(This->typeinfos[i]); - } - free(This->typeinfos); - free(This); + for (int i = 0; i < This->TypeInfoCount; ++i) { + free(This->typeinfos[i]->tdescAlias); + ITypeInfoImpl_Destroy(This->typeinfos[i]); } + free(This->typeinfos); + free(This); LeaveCriticalSection(&cache_section); return ref; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7375
Please don't change things that don't need changing. Some people like doing an early return, others prefer not to, it's a matter of taste. And in matters of taste, the rule is to not change other people's code. That sort of thing may sometimes be justified if there are too many indentation levels, but that's not the case here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7375#note_95250
@julliard "Please don't change things that don't need changing" - OK so wine is not accepting patches that both improve legibility and reduces complexity while making it easy for review by splitting up into separate patches? Clear. By the way, this particular DLL is beyond broken, I don't see why you're so resistant to changing its code when there are tests that cover all this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7375#note_95289
On Thu Feb 20 09:41:43 2025 +0000, Alexandre Julliard wrote:
Please don't change things that don't need changing. Some people like doing an early return, others prefer not to, it's a matter of taste. And in matters of taste, the rule is to not change other people's code. That sort of thing may sometimes be justified if there are too many indentation levels, but that's not the case here. I agree with Damien here.
Ay? Style, no! It is called cyclomatic complexity and the graph has multiplicative combinatorial complexity for cascading if-stmts whereas the complexity is additive for successive ones handling base conditions.
That sort of thing may sometimes be justified if there are too many indentation levels, but that's not the case here.
Well only because things like heap allocations are not even being validated nor method parameter state space. The work very clearly fits into a larger volume of work to have a functional oleaut32 implementation. It would all be very well arguing from a position of strength however this code, even a cursory inspection, is littered with enumerable bugs. Everything from integer overflows, buffer overflows, use-after-frees and using garbage values in weird char buffers both in the heap and in the stack. To be blunt it is comical and I suspect it only "works" in the sense that most of the Windows API surface really doesn't actually amount to anything operationally speaking so you have the illusion of working code. "Don't make changes to other peoples code" is a extremely odd theology for a free software project. Considering these are the least controversial changes I plucked from a long series working towards logically sound code I really don't know what I can possibly say beyond the feeling that no one is welcome here to contribute. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7375#note_95297
participants (4)
-
Alexandre Julliard (＠julliard) -
Damien Zammit (＠zamaudio)
-
Edward O'Callaghan
-
Edward OC (＠funfunctor)