[PATCH v2 0/2] MR8646: dmcompos: Simplify the DMChordMap IPersistStream_Load() method

[Michael Stefaniuc (＠mstefani)]

After the fixes from MR!8644 Chicken Tournament parses more of its DMusic files and spits out hundreds of these messages:
```
0024:warn:dmfileraw:IDirectMusicLoaderResourceStream_IStream_Read : requested size out of range
0024:warn:dmfileraw:IDirectMusicLoaderResourceStream_IStream_Read : requested size out of range
0024:warn:dmfileraw:IDirectMusicLoaderResourceStream_IStream_Seek : requested offset out of range
0024:warn:dmfileraw:IDirectMusicLoaderResourceStream_IStream_Read : requested size out of range
```

This MR doesn't depends on MR!8644.

--
  v2: dmcompos: Simplify the DMChordMap IPersistStream_Load() method
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8646

[PATCH v2 1/2] dmcompos/tests: Fix the expected value in an ok() message.

[Michael Stefaniuc]

From: Michael Stefaniuc <mstefani(a)winehq.org>

---
 dlls/dmcompos/tests/dmcompos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/dmcompos/tests/dmcompos.c b/dlls/dmcompos/tests/dmcompos.c
index 2e0047c19c6..b2b45073cb9 100644
--- a/dlls/dmcompos/tests/dmcompos.c
+++ b/dlls/dmcompos/tests/dmcompos.c
@@ -572,7 +572,7 @@ static void test_parsedescriptor(void)
     /* Nothing loaded */
     hr = IDirectMusicObject_GetDescriptor(dmo, &desc);
     ok(hr == S_OK, "GetDescriptor failed: %#lx, expected S_OK\n", hr);
-    ok(desc.dwValidData == DMUS_OBJ_CLASS, "Got valid data %#lx, expected DMUS_OBJ_OBJECT\n",
+    ok(desc.dwValidData == DMUS_OBJ_CLASS, "Got valid data %#lx, expected DMUS_OBJ_CLASS\n",
             desc.dwValidData);
     ok(IsEqualGUID(&desc.guidClass, &CLSID_DirectMusicChordMap),
             "Got class guid %s, expected CLSID_DirectMusicChordMap\n",
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/8646

[PATCH v2 2/2] dmcompos: Simplify the DMChordMap IPersistStream_Load() method

[Michael Stefaniuc]

From: Michael Stefaniuc <mstefani(a)winehq.org>

Fixes reads / seeks past the end of the IStream.
---
 dlls/dmcompos/chordmap.c | 189 +++++++++------------------------------
 1 file changed, 43 insertions(+), 146 deletions(-)

diff --git a/dlls/dmcompos/chordmap.c b/dlls/dmcompos/chordmap.c
index 2ddddd3fd57..f0a5567122c 100644
--- a/dlls/dmcompos/chordmap.c
+++ b/dlls/dmcompos/chordmap.c
@@ -21,7 +21,6 @@
 #include "dmobject.h"
 
 WINE_DEFAULT_DEBUG_CHANNEL(dmcompos);
-WINE_DECLARE_DEBUG_CHANNEL(dmfile);
 
 /*****************************************************************************
  * IDirectMusicChordMapImpl implementation
@@ -147,151 +146,49 @@ static inline IDirectMusicChordMapImpl *impl_from_IPersistStream(IPersistStream
     return CONTAINING_RECORD(iface, IDirectMusicChordMapImpl, dmobj.IPersistStream_iface);
 }
 
-static HRESULT WINAPI IPersistStreamImpl_Load(IPersistStream *iface, IStream *pStm)
+static HRESULT WINAPI chord_persist_stream_Load(IPersistStream *iface, IStream *stream)
 {
-        IDirectMusicChordMapImpl *This = impl_from_IPersistStream(iface);
-	FOURCC chunkID;
-	DWORD chunkSize, StreamSize, StreamCount, ListSize[3], ListCount[3];
-	LARGE_INTEGER liMove; /* used when skipping chunks */
-
-	FIXME("(%p, %p): Loading not implemented yet\n", This, pStm);
-	IStream_Read (pStm, &chunkID, sizeof(FOURCC), NULL);
-	IStream_Read (pStm, &chunkSize, sizeof(DWORD), NULL);
-	TRACE_(dmfile)(": %s chunk (size = %ld)", debugstr_fourcc (chunkID), chunkSize);
-	switch (chunkID) {	
-		case FOURCC_RIFF: {
-			IStream_Read (pStm, &chunkID, sizeof(FOURCC), NULL);				
-			TRACE_(dmfile)(": RIFF chunk of type %s", debugstr_fourcc(chunkID));
-			StreamSize = chunkSize - sizeof(FOURCC);
-			StreamCount = 0;
-			switch (chunkID) {
-				case DMUS_FOURCC_CHORDMAP_FORM: {
-					TRACE_(dmfile)(": chordmap form\n");
-					do {
-						IStream_Read (pStm, &chunkID, sizeof(FOURCC), NULL);
-						IStream_Read (pStm, &chunkSize, sizeof(FOURCC), NULL);
-						StreamCount += sizeof(FOURCC) + sizeof(DWORD) + chunkSize;
-						TRACE_(dmfile)(": %s chunk (size = %ld)", debugstr_fourcc (chunkID), chunkSize);
-						switch (chunkID) {
-							case DMUS_FOURCC_GUID_CHUNK: {
-								TRACE_(dmfile)(": GUID chunk\n");
-								This->dmobj.desc.dwValidData |= DMUS_OBJ_OBJECT;
-								IStream_Read (pStm, &This->dmobj.desc.guidObject, chunkSize, NULL);
-								break;
-							}
-							case DMUS_FOURCC_VERSION_CHUNK: {
-								TRACE_(dmfile)(": version chunk\n");
-								This->dmobj.desc.dwValidData |= DMUS_OBJ_VERSION;
-								IStream_Read (pStm, &This->dmobj.desc.vVersion, chunkSize, NULL);
-								break;
-							}
-							case DMUS_FOURCC_CATEGORY_CHUNK: {
-								TRACE_(dmfile)(": category chunk\n");
-								This->dmobj.desc.dwValidData |= DMUS_OBJ_CATEGORY;
-								IStream_Read (pStm, This->dmobj.desc.wszCategory, chunkSize, NULL);
-								break;
-							}
-							case FOURCC_LIST: {
-								IStream_Read (pStm, &chunkID, sizeof(FOURCC), NULL);				
-								TRACE_(dmfile)(": LIST chunk of type %s", debugstr_fourcc(chunkID));
-								ListSize[0] = chunkSize - sizeof(FOURCC);
-								ListCount[0] = 0;
-								switch (chunkID) {
-									case DMUS_FOURCC_UNFO_LIST: {
-										TRACE_(dmfile)(": UNFO list\n");
-										do {
-											IStream_Read (pStm, &chunkID, sizeof(FOURCC), NULL);
-											IStream_Read (pStm, &chunkSize, sizeof(FOURCC), NULL);
-											ListCount[0] += sizeof(FOURCC) + sizeof(DWORD) + chunkSize;
-											TRACE_(dmfile)(": %s chunk (size = %ld)", debugstr_fourcc (chunkID), chunkSize);
-											switch (chunkID) {
-												/* don't ask me why, but M$ puts INFO elements in UNFO list sometimes
-                                              (though strings seem to be valid unicode) */
-												case mmioFOURCC('I','N','A','M'):
-												case DMUS_FOURCC_UNAM_CHUNK: {
-													TRACE_(dmfile)(": name chunk\n");
-													This->dmobj.desc.dwValidData |= DMUS_OBJ_NAME;
-													IStream_Read (pStm, This->dmobj.desc.wszName, chunkSize, NULL);
-													break;
-												}
-												case mmioFOURCC('I','A','R','T'):
-												case DMUS_FOURCC_UART_CHUNK: {
-													TRACE_(dmfile)(": artist chunk (ignored)\n");
-													liMove.QuadPart = chunkSize;
-													IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL);
-													break;
-												}
-												case mmioFOURCC('I','C','O','P'):
-												case DMUS_FOURCC_UCOP_CHUNK: {
-													TRACE_(dmfile)(": copyright chunk (ignored)\n");
-													liMove.QuadPart = chunkSize;
-													IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL);
-													break;
-												}
-												case mmioFOURCC('I','S','B','J'):
-												case DMUS_FOURCC_USBJ_CHUNK: {
-													TRACE_(dmfile)(": subject chunk (ignored)\n");
-													liMove.QuadPart = chunkSize;
-													IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL);
-													break;
-												}
-												case mmioFOURCC('I','C','M','T'):
-												case DMUS_FOURCC_UCMT_CHUNK: {
-													TRACE_(dmfile)(": comment chunk (ignored)\n");
-													liMove.QuadPart = chunkSize;
-													IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL);
-													break;
-												}
-												default: {
-													TRACE_(dmfile)(": unknown chunk (irrelevant & skipping)\n");
-													liMove.QuadPart = chunkSize;
-													IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL);
-													break;						
-												}
-											}
-											TRACE_(dmfile)(": ListCount[0] = %ld < ListSize[0] = %ld\n", ListCount[0], ListSize[0]);
-										} while (ListCount[0] < ListSize[0]);
-										break;
-									}
-									default: {
-										TRACE_(dmfile)(": unknown (skipping)\n");
-										liMove.QuadPart = chunkSize - sizeof(FOURCC);
-										IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL);
-										break;						
-									}
-								}
-								break;
-							}	
-							default: {
-								TRACE_(dmfile)(": unknown chunk (irrelevant & skipping)\n");
-								liMove.QuadPart = chunkSize;
-								IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL);
-								break;						
-							}
-						}
-						TRACE_(dmfile)(": StreamCount[0] = %ld < StreamSize[0] = %ld\n", StreamCount, StreamSize);
-					} while (StreamCount < StreamSize);
-					break;
-				}
-				default: {
-					TRACE_(dmfile)(": unexpected chunk; loading failed)\n");
-					liMove.QuadPart = StreamSize;
-					IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL); /* skip the rest of the chunk */
-					return E_FAIL;
-				}
-			}
-			TRACE_(dmfile)(": reading finished\n");
-			break;
-		}
-		default: {
-			TRACE_(dmfile)(": unexpected chunk; loading failed)\n");
-			liMove.QuadPart = chunkSize;
-			IStream_Seek (pStm, liMove, STREAM_SEEK_CUR, NULL); /* skip the rest of the chunk */
-			return E_FAIL;
-		}
-	}
-
-	return S_OK;
+    IDirectMusicChordMapImpl *This = impl_from_IPersistStream(iface);
+    struct chunk_entry chordmap = {0};
+    struct chunk_entry chunk = {.parent = &chordmap};
+    HRESULT hr;
+
+    FIXME("(%p, %p): Loading not implemented yet\n", This, stream);
+
+    if (!stream)
+        return E_POINTER;
+
+    if (stream_get_chunk(stream, &chordmap) != S_OK || chordmap.id != FOURCC_RIFF ||
+            chordmap.type != DMUS_FOURCC_CHORDMAP_FORM)
+    {
+        WARN("Invalid chunk %s\n", debugstr_chunk(&chordmap));
+        return DMUS_E_UNSUPPORTED_STREAM;
+    }
+
+    if (FAILED(hr = dmobj_parsedescriptor(stream, &chordmap, &This->dmobj.desc, DMUS_OBJ_OBJECT))
+                || FAILED(hr = stream_reset_chunk_data(stream, &chordmap)))
+        return hr;
+
+    while ((hr = stream_next_chunk(stream, &chunk)) == S_OK)
+    {
+        switch (MAKE_IDTYPE(chunk.id, chunk.type))
+        {
+            case DMUS_FOURCC_GUID_CHUNK:
+            case DMUS_FOURCC_VERSION_CHUNK:
+            case MAKE_IDTYPE(FOURCC_LIST, DMUS_FOURCC_UNFO_LIST):
+                /* already parsed/ignored by dmobj_parsedescriptor */
+                break;
+
+            default:
+                FIXME("Ignoring chunk %s\n", debugstr_chunk(&chunk));
+                break;
+        }
+
+        if (FAILED(hr))
+            return hr;
+    }
+
+    return S_OK;
 }
 
 static const IPersistStreamVtbl persiststream_vtbl = {
@@ -300,7 +197,7 @@ static const IPersistStreamVtbl persiststream_vtbl = {
     dmobj_IPersistStream_Release,
     dmobj_IPersistStream_GetClassID,
     unimpl_IPersistStream_IsDirty,
-    IPersistStreamImpl_Load,
+    chord_persist_stream_Load,
     unimpl_IPersistStream_Save,
     unimpl_IPersistStream_GetSizeMax
 };
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8646