[PATCH] dmloader: Only use the GUID when looking for cached objects.

[Alistair Leslie-Hughes]

---
 dlls/dmloader/loader.c | 50 +-----------------------------------------
 1 file changed, 1 insertion(+), 49 deletions(-)

diff --git a/dlls/dmloader/loader.c b/dlls/dmloader/loader.c
index 0da8323e7e5..c4c54412e12 100644
--- a/dlls/dmloader/loader.c
+++ b/dlls/dmloader/loader.c
@@ -208,55 +208,7 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_GetObject(IDirectMusicLoader8 *ifac
    			TRACE(": not loaded yet\n");
    			pObjectEntry = pExistingEntry;
    		}
-		}
-		else if ((pDesc->dwValidData & (DMUS_OBJ_FILENAME | DMUS_OBJ_FULLPATH)) &&
-				(pExistingEntry->Desc.dwValidData & (DMUS_OBJ_FILENAME | DMUS_OBJ_FULLPATH)) &&
-				!wcsncmp (pDesc->wszFileName, pExistingEntry->Desc.wszFileName, DMUS_MAX_FILENAME)) {
-			TRACE(": found it by fullpath filename\n");
-			if (pExistingEntry->Desc.dwValidData & DMUS_OBJ_LOADED) {
-				TRACE(": already loaded\n");
-				return IDirectMusicObject_QueryInterface (pExistingEntry->pObject, riid, ppv);
-			} else {
-				TRACE(": not loaded yet\n");
-				pObjectEntry = pExistingEntry;
-			}
-		}
-		else if ((pDesc->dwValidData & (DMUS_OBJ_NAME | DMUS_OBJ_CATEGORY)) &&
-				(pExistingEntry->Desc.dwValidData & (DMUS_OBJ_NAME | DMUS_OBJ_CATEGORY)) &&
-				!wcsncmp (pDesc->wszName, pExistingEntry->Desc.wszName, DMUS_MAX_NAME) &&
-				!wcsncmp (pDesc->wszCategory, pExistingEntry->Desc.wszCategory, DMUS_MAX_CATEGORY)) {
-			TRACE(": found it by name and category\n");
-			if (pExistingEntry->Desc.dwValidData & DMUS_OBJ_LOADED) {
-				TRACE(": already loaded\n");
-				return IDirectMusicObject_QueryInterface (pExistingEntry->pObject, riid, ppv);
-			} else {
-				TRACE(": not loaded yet\n");
-				pObjectEntry = pExistingEntry;
-			}
-		}
-		else if ((pDesc->dwValidData & DMUS_OBJ_NAME) &&
-				(pExistingEntry->Desc.dwValidData & DMUS_OBJ_NAME) &&
-				!wcsncmp (pDesc->wszName, pExistingEntry->Desc.wszName, DMUS_MAX_NAME)) {
-			TRACE(": found it by name\n");
-			if (pExistingEntry->Desc.dwValidData & DMUS_OBJ_LOADED) {
-				TRACE(": already loaded\n");
-				return IDirectMusicObject_QueryInterface (pExistingEntry->pObject, riid, ppv);
-			} else {
-				TRACE(": not loaded yet\n");
-				pObjectEntry = pExistingEntry;
-			}
-		}
-		else if ((pDesc->dwValidData & DMUS_OBJ_FILENAME) &&
-				(pExistingEntry->Desc.dwValidData & DMUS_OBJ_FILENAME) &&
-				!wcsncmp (pDesc->wszFileName, pExistingEntry->Desc.wszFileName, DMUS_MAX_FILENAME)) {
-			TRACE(": found it by filename\n");				
-			if (pExistingEntry->Desc.dwValidData & DMUS_OBJ_LOADED) {
-				TRACE(": already loaded\n");
-				return IDirectMusicObject_QueryInterface (pExistingEntry->pObject, riid, ppv);
-			} else {
-				TRACE(": not loaded yet\n");
-				pObjectEntry = pExistingEntry;
-			}
+			break;
    	}
    }

[Michael Stefaniuc]

Hello Alistair,

any reason you remove that code?

I do see that code being hit by some apps (Ludwig3, the DirectMusic
samples from the DirectX 8.1b SDK).

Also we have similar code in, IDirectMusicLoaderImpl_CacheObject() and
IDirectMusicLoaderImpl_ReleaseObject().

thanks
bye
    michael

On 11/21/19 8:40 AM, Alistair Leslie-Hughes wrote:
> dlls/dmloader/loader.c | 50 +-----------------------------------------
>  1 file changed, 1 insertion(+), 49 deletions(-)
>
> diff --git a/dlls/dmloader/loader.c b/dlls/dmloader/loader.c
> index 0da8323e7e5..c4c54412e12 100644
> --- a/dlls/dmloader/loader.c
> +++ b/dlls/dmloader/loader.c
> @@ -208,55 +208,7 @@ static HRESULT WINAPI IDirectMusicLoaderImpl_GetObject(IDirectMusicLoader8 *ifac
>    			TRACE(": not loaded yet\n");
>    			pObjectEntry = pExistingEntry;
>    		}
> -
> }
> -
> else if ((pDesc->dwValidData & (DMUS_OBJ_FILENAME | DMUS_OBJ_FULLPATH)) &&
> -
> 		(pExistingEntry->Desc.dwValidData & (DMUS_OBJ_FILENAME | DMUS_OBJ_FULLPATH)) &&
> -
> 		!wcsncmp (pDesc->wszFileName, pExistingEntry->Desc.wszFileName, DMUS_MAX_FILENAME)) {
> -
> 	TRACE(": found it by fullpath filename\n");
> -
> 	if (pExistingEntry->Desc.dwValidData & DMUS_OBJ_LOADED) {
> -
> 		TRACE(": already loaded\n");
> -
> 		return IDirectMusicObject_QueryInterface (pExistingEntry->pObject, riid, ppv);
> -
> 	} else {
> -
> 		TRACE(": not loaded yet\n");
> -
> 		pObjectEntry = pExistingEntry;
> -
> 	}
> -
> }
> -
> else if ((pDesc->dwValidData & (DMUS_OBJ_NAME | DMUS_OBJ_CATEGORY)) &&
> -
> 		(pExistingEntry->Desc.dwValidData & (DMUS_OBJ_NAME | DMUS_OBJ_CATEGORY)) &&
> -
> 		!wcsncmp (pDesc->wszName, pExistingEntry->Desc.wszName, DMUS_MAX_NAME) &&
> -
> 		!wcsncmp (pDesc->wszCategory, pExistingEntry->Desc.wszCategory, DMUS_MAX_CATEGORY)) {
> -
> 	TRACE(": found it by name and category\n");
> -
> 	if (pExistingEntry->Desc.dwValidData & DMUS_OBJ_LOADED) {
> -
> 		TRACE(": already loaded\n");
> -
> 		return IDirectMusicObject_QueryInterface (pExistingEntry->pObject, riid, ppv);
> -
> 	} else {
> -
> 		TRACE(": not loaded yet\n");
> -
> 		pObjectEntry = pExistingEntry;
> -
> 	}
> -
> }
> -
> else if ((pDesc->dwValidData & DMUS_OBJ_NAME) &&
> -
> 		(pExistingEntry->Desc.dwValidData & DMUS_OBJ_NAME) &&
> -
> 		!wcsncmp (pDesc->wszName, pExistingEntry->Desc.wszName, DMUS_MAX_NAME)) {
> -
> 	TRACE(": found it by name\n");
> -
> 	if (pExistingEntry->Desc.dwValidData & DMUS_OBJ_LOADED) {
> -
> 		TRACE(": already loaded\n");
> -
> 		return IDirectMusicObject_QueryInterface (pExistingEntry->pObject, riid, ppv);
> -
> 	} else {
> -
> 		TRACE(": not loaded yet\n");
> -
> 		pObjectEntry = pExistingEntry;
> -
> 	}
> -
> }
> -
> else if ((pDesc->dwValidData & DMUS_OBJ_FILENAME) &&
> -
> 		(pExistingEntry->Desc.dwValidData & DMUS_OBJ_FILENAME) &&
> -
> 		!wcsncmp (pDesc->wszFileName, pExistingEntry->Desc.wszFileName, DMUS_MAX_FILENAME)) {
> -
> 	TRACE(": found it by filename\n");				
> -
> 	if (pExistingEntry->Desc.dwValidData & DMUS_OBJ_LOADED) {
> -
> 		TRACE(": already loaded\n");
> -
> 		return IDirectMusicObject_QueryInterface (pExistingEntry->pObject, riid, ppv);
> -
> 	} else {
> -
> 		TRACE(": not loaded yet\n");
> -
> 		pObjectEntry = pExistingEntry;
> -
> 	}
> -
> 	break;
> }
> }

[Alistair Leslie-Hughes]

Hi Micheal,

Game LegoLand.

This game loads about 40 objects (sgt,sty) into the cache, and when it's looping
over the cache it's selecting the wrong object.

Simple example, of 3 objects.

1. GUID XX, Name "Trans" category "chord"   (filename trans.sgt)
2. GUID YY, Name "Other" category "tempo"
3. GUID ZZ, Name "Trans" category "chord"   (filename trans.sty)

Using IDirectMusicLoader8_GetObject

Processing element 3, searching for an object with GUID = XX, Name = "Trans" and category = "chord".
We find the object 1 (correct)
We continue searching and find object 3 (wrong that's ourself).
Then we attempt to resolve object 3 by calling _GetObject again, causing a stack overflow.

Snippet of the log.
0030:trace:dmloader:dump_DMUS_OBJECTDESC  - wszFileName = L"WLtran2.sty"
0030:trace:dmloader:IDirectMusicLoaderImpl_GetObject : looking if we have object in the cache or if it can be found via alias
0030:trace:dmloader:IDirectMusicLoaderImpl_GetObject : found it by object GUID
0030:trace:dmloader:IDirectMusicLoaderImpl_GetObject : not loaded yet
0030:trace:dmloader:IDirectMusicLoaderImpl_GetObject : found it by name and category
0030:trace:dmloader:IDirectMusicLoaderImpl_GetObject : not loaded yet

The unique identifiers in this structure are GUID, filename.

Also the loop has bad logic.

These are the same, so the second if will never be hit.
else if ((pDesc->dwValidData & (DMUS_OBJ_FILENAME | DMUS_OBJ_FULLPATH)) &&
...
else if ((pDesc->dwValidData & DMUS_OBJ_FILENAME) &&

If we want to keep all conditions, then we could do something beflow, so
weight is given each flag. (simplify logic).
if (pDesc->dwValidData & DMUS_OBJ_OBJECT)
{
    if(IsEqualGUID (&pDesc->guidObject, &pExistingEntry->Desc.guidObject))
        pObjectEntry = pExistingEntry;
    break;
}

Regards
Alistair.