Patch: fix for bug #4436 with associated fixes for stream reference
counting
Dr J A Gow
J.A.Gow at furrybubble.co.uk
Sun Feb 19 11:22:22 CST 2006
FROM: Dr J A Gow
PATCH: Provides fix for bug 4436, some reference counting fixes, and
now the IStorage correctly handles the following:
- Stream opening when storage was opened in transacted mode
- Stream methods called after parent object has been closed
correctly return STG_E_REVERTED
- Adds conformance test for stream opening in transacted
mode
Index: wine/dlls/ole32/stg_stream.c
===================================================================
RCS file: /home/wine/wine/dlls/ole32/stg_stream.c,v
retrieving revision 1.29
diff -u -p -r1.29 stg_stream.c
--- wine/dlls/ole32/stg_stream.c 3 Jan 2006 12:40:12 -0000 1.29
+++ wine/dlls/ole32/stg_stream.c 19 Feb 2006 16:42:43 -0000
@@ -58,8 +58,21 @@ static void StgStreamImpl_Destroy(StgStr
/*
* Release the reference we are holding on the parent storage.
+ * IStorage_Release((IStorage*)This->parentStorage);
+ *
+ * No, don't do this. Some apps call IStorage_Release without
+ * calling IStream_Release first. If we grab a reference the
+ * file is not closed, and the app fails when it tries to
+ * reopen the file (Easy-PC, for example). Just inform the
+ * storage that we have closed the stream
*/
- IStorage_Release((IStorage*)This->parentStorage);
+
+ if(This->parentStorage) {
+
+ StorageBaseImpl_RemoveStream(This->parentStorage, This);
+
+ }
+
This->parentStorage = 0;
/*
@@ -447,6 +460,14 @@ static HRESULT WINAPI StgStreamImpl_Seek
iface, dlibMove.u.LowPart, dwOrigin, plibNewPosition);
/*
+ * fail if the stream has no parent (as does windows)
+ */
+
+ if(!(This->parentStorage)) {
+ return STG_E_REVERTED;
+ }
+
+ /*
* The caller is allowed to pass in NULL as the new position return value.
* If it happens, we assign it to a dynamic variable to avoid special cases
* in the code below.
@@ -506,6 +527,10 @@ static HRESULT WINAPI StgStreamImpl_SetS
TRACE("(%p, %ld)\n", iface, libNewSize.u.LowPart);
+ if(!This->parentStorage) {
+ return STG_E_REVERTED;
+ }
+
/*
* As documented.
*/
@@ -609,6 +634,8 @@ static HRESULT WINAPI StgStreamImpl_Copy
ULARGE_INTEGER* pcbRead, /* [out] */
ULARGE_INTEGER* pcbWritten) /* [out] */
{
+ StgStreamImpl* const This=(StgStreamImpl*)iface;
+
HRESULT hr = S_OK;
BYTE tmpBuffer[128];
ULONG bytesRead, bytesWritten, copySize;
@@ -621,6 +648,11 @@ static HRESULT WINAPI StgStreamImpl_Copy
/*
* Sanity check
*/
+
+ if(!This->parentStorage) {
+ return STG_E_REVERTED;
+ }
+
if ( pstm == 0 )
return STG_E_INVALIDPOINTER;
@@ -691,6 +723,11 @@ static HRESULT WINAPI StgStreamImpl_Comm
IStream* iface,
DWORD grfCommitFlags) /* [in] */
{
+ StgStreamImpl* const This=(StgStreamImpl*)iface;
+
+ if(!This->parentStorage) {
+ return STG_E_REVERTED;
+ }
return S_OK;
}
@@ -714,6 +751,12 @@ static HRESULT WINAPI StgStreamImpl_Lock
ULARGE_INTEGER cb, /* [in] */
DWORD dwLockType) /* [in] */
{
+ StgStreamImpl* const This=(StgStreamImpl*)iface;
+
+ if(!This->parentStorage) {
+ return STG_E_REVERTED;
+ }
+
FIXME("not implemented!\n");
return E_NOTIMPL;
}
@@ -724,6 +767,12 @@ static HRESULT WINAPI StgStreamImpl_Unlo
ULARGE_INTEGER cb, /* [in] */
DWORD dwLockType) /* [in] */
{
+ StgStreamImpl* const This=(StgStreamImpl*)iface;
+
+ if(!This->parentStorage) {
+ return STG_E_REVERTED;
+ }
+
FIXME("not implemented!\n");
return E_NOTIMPL;
}
@@ -747,6 +796,14 @@ static HRESULT WINAPI StgStreamImpl_Stat
BOOL readSucessful;
/*
+ * if stream has no parent, return STG_E_REVERTED
+ */
+
+ if(!This->parentStorage) {
+ return STG_E_REVERTED;
+ }
+
+ /*
* Read the information from the property.
*/
readSucessful = StorageImpl_ReadProperty(This->parentStorage->ancestorStorage,
@@ -791,6 +848,11 @@ static HRESULT WINAPI StgStreamImpl_Clon
/*
* Sanity check
*/
+
+ if(!This->parentStorage) {
+ return STG_E_REVERTED;
+ }
+
if ( ppstm == 0 )
return STG_E_INVALIDPOINTER;
@@ -858,12 +920,19 @@ StgStreamImpl* StgStreamImpl_Construct(
newStream->lpVtbl = &StgStreamImpl_Vtbl;
newStream->ref = 0;
+ newStream->parentStorage = parentStorage;
+
/*
* We want to nail-down the reference to the storage in case the
* stream out-lives the storage in the client application.
+ *
+ * -- IStorage_AddRef((IStorage*)newStream->parentStorage);
+ *
+ * No, don't do this. Some apps call IStorage_Release without
+ * calling IStream_Release first. If we grab a reference the
+ * file is not closed, and the app fails when it tries to
+ * reopen the file (Easy-PC, for example)
*/
- newStream->parentStorage = parentStorage;
- IStorage_AddRef((IStorage*)newStream->parentStorage);
newStream->grfMode = grfMode;
newStream->ownerProperty = ownerProperty;
Index: wine/dlls/ole32/storage32.c
===================================================================
RCS file: /home/wine/wine/dlls/ole32/storage32.c,v
retrieving revision 1.99
diff -u -p -r1.99 storage32.c
--- wine/dlls/ole32/storage32.c 6 Jan 2006 20:51:54 -0000 1.99
+++ wine/dlls/ole32/storage32.c 19 Feb 2006 16:42:50 -0000
@@ -342,13 +342,16 @@ HRESULT WINAPI StorageBaseImpl_OpenStrea
}
/*
- * Check that we're compatible with the parent's storage mode
+ * Check that we're compatible with the parent's storage mode, but
+ * only if we are not in transacted mode
*/
parent_grfMode = STGM_ACCESS_MODE( This->ancestorStorage->base.openFlags );
- if ( STGM_ACCESS_MODE( grfMode ) > STGM_ACCESS_MODE( parent_grfMode ) )
- {
- res = STG_E_ACCESSDENIED;
- goto end;
+ if(!(This->ancestorStorage->base.openFlags & STGM_TRANSACTED)) {
+ if ( STGM_ACCESS_MODE( grfMode ) > STGM_ACCESS_MODE( parent_grfMode ) )
+ {
+ res = STG_E_ACCESSDENIED;
+ goto end;
+ }
}
/*
@@ -390,6 +393,12 @@ HRESULT WINAPI StorageBaseImpl_OpenStrea
*/
IStream_AddRef(*ppstm);
+ /*
+ * add us to the storage's list of active streams
+ */
+
+ StorageBaseImpl_AddStream(This,newStream);
+
res = S_OK;
goto end;
}
@@ -472,13 +481,16 @@ HRESULT WINAPI StorageBaseImpl_OpenStora
}
/*
- * Check that we're compatible with the parent's storage mode
+ * Check that we're compatible with the parent's storage mode,
+ * but only if we are not transacted
*/
parent_grfMode = STGM_ACCESS_MODE( This->ancestorStorage->base.openFlags );
- if ( STGM_ACCESS_MODE( grfMode ) > STGM_ACCESS_MODE( parent_grfMode ) )
- {
- res = STG_E_ACCESSDENIED;
- goto end;
+ if(!(This->ancestorStorage->base.openFlags & STGM_TRANSACTED)) {
+ if ( STGM_ACCESS_MODE( grfMode ) > STGM_ACCESS_MODE( parent_grfMode ) )
+ {
+ res = STG_E_ACCESSDENIED;
+ goto end;
+ }
}
/*
@@ -869,10 +881,13 @@ HRESULT WINAPI StorageBaseImpl_CreateStr
/*
* Check that we're compatible with the parent's storage mode
+ * if not in transacted mode
*/
parent_grfMode = STGM_ACCESS_MODE( This->ancestorStorage->base.openFlags );
- if ( STGM_ACCESS_MODE( grfMode ) > STGM_ACCESS_MODE( parent_grfMode ) )
- return STG_E_ACCESSDENIED;
+ if(!(parent_grfMode & STGM_TRANSACTED)) {
+ if ( STGM_ACCESS_MODE( grfMode ) > STGM_ACCESS_MODE( parent_grfMode ) )
+ return STG_E_ACCESSDENIED;
+ }
/*
* Initialize the out parameter
@@ -970,6 +985,11 @@ HRESULT WINAPI StorageBaseImpl_CreateStr
* the reference.
*/
IStream_AddRef(*ppstm);
+
+ /* add us to the storage's list of active streams
+ */
+ StorageBaseImpl_AddStream(This,newStream);
+
}
else
{
@@ -1788,8 +1808,35 @@ HRESULT WINAPI StorageImpl_Stat( IStorag
return result;
}
+ /******************************************************************************
+ * Internal stream list handlers
+ */
+void StorageBaseImpl_AddStream(StorageBaseImpl * stg, StgStreamImpl * strm)
+{
+ TRACE("Stream added (stg=%p strm=%p)\n", stg, strm);
+ list_add_tail(&stg->strmHead,&strm->StrmListEntry);
+}
+void StorageBaseImpl_RemoveStream(StorageBaseImpl * stg, StgStreamImpl * strm)
+{
+ TRACE("Stream removed (stg=%p strm=%p)\n", stg,strm);
+ list_remove(&(strm->StrmListEntry));
+}
+
+void StorageBaseImpl_DeleteAll(StorageBaseImpl * stg)
+{
+ struct list * cur;
+ struct list * cur2;
+ StgStreamImpl * strm=NULL;
+
+ LIST_FOR_EACH_SAFE(cur, cur2, &stg->strmHead) {
+ strm = LIST_ENTRY(cur,StgStreamImpl,StrmListEntry);
+ TRACE("Streams deleted (stg=%p strm=%p next=%p prev=%p)\n", stg,strm,cur->next,cur->prev);
+ strm->parentStorage = NULL;
+ list_remove(cur);
+ }
+}
/*********************************************************************
*
* Internal Method
@@ -2247,6 +2294,12 @@ HRESULT StorageImpl_Construct(
memset(This, 0, sizeof(StorageImpl));
/*
+ * Initialize stream list
+ */
+
+ list_init(&This->base.strmHead);
+
+ /*
* Initialize the virtual function table.
*/
This->base.lpVtbl = &Storage32Impl_Vtbl;
@@ -2439,6 +2492,8 @@ void StorageImpl_Destroy(StorageBaseImpl
StorageImpl *This = (StorageImpl*) iface;
TRACE("(%p)\n", This);
+ StorageBaseImpl_DeleteAll(&This->base);
+
HeapFree(GetProcessHeap(), 0, This->pwcsName);
BlockChainStream_Destroy(This->smallBlockRootChain);
@@ -4104,6 +4159,12 @@ StorageInternalImpl* StorageInternalImpl
memset(newStorage, 0, sizeof(StorageInternalImpl));
/*
+ * Initialize the stream list
+ */
+
+ list_init(&newStorage->base.strmHead);
+
+ /*
* Initialize the virtual function table.
*/
newStorage->base.lpVtbl = &Storage32InternalImpl_Vtbl;
Index: wine/dlls/ole32/storage32.h
===================================================================
RCS file: /home/wine/wine/dlls/ole32/storage32.h,v
retrieving revision 1.27
diff -u -p -r1.27 storage32.h
--- wine/dlls/ole32/storage32.h 22 Dec 2005 17:12:52 -0000 1.27
+++ wine/dlls/ole32/storage32.h 19 Feb 2006 16:42:51 -0000
@@ -37,6 +37,7 @@
#include "objbase.h"
#include "winreg.h"
#include "winternl.h"
+#include <wine/list.h>
/*
* Definitions for the file format offsets.
@@ -220,6 +221,12 @@ struct StorageBaseImpl
const IPropertySetStorageVtbl *pssVtbl; /* interface for adding a properties stream */
/*
+ * Stream tracking list
+ */
+
+ struct list strmHead;
+
+ /*
* Reference count of this object
*/
LONG ref;
@@ -246,6 +253,13 @@ struct StorageBaseImpl
DWORD openFlags;
};
+/****************************************************************************
+ * StorageBaseImpl stream list handlers
+ */
+
+void StorageBaseImpl_AddStream(StorageBaseImpl * stg, StgStreamImpl * strm);
+void StorageBaseImpl_RemoveStream(StorageBaseImpl * stg, StgStreamImpl * strm);
+void StorageBaseImpl_DeleteAll(StorageBaseImpl * stg);
/****************************************************************************
* Storage32Impl definitions.
@@ -485,6 +499,12 @@ struct StgStreamImpl
* since we want to cast this to an IStream pointer */
/*
+ * We are an entry in the storage object's stream handler list
+ */
+
+ struct list StrmListEntry;
+
+ /*
* Reference count
*/
LONG ref;
Index: wine/dlls/ole32/tests/storage32.c
===================================================================
RCS file: /home/wine/wine/dlls/ole32/tests/storage32.c,v
retrieving revision 1.15
diff -u -p -r1.15 storage32.c
--- wine/dlls/ole32/tests/storage32.c 15 Feb 2006 13:03:12 -0000 1.15
+++ wine/dlls/ole32/tests/storage32.c 19 Feb 2006 16:42:52 -0000
@@ -593,7 +593,13 @@ static void test_storage_refcount(void)
r = IStorage_CreateStream(stg, stmname, STGM_SHARE_EXCLUSIVE | STGM_READWRITE, 0, 0, &stm );
ok(r==S_OK, "IStorage->CreateStream failed\n");
- todo_wine {
+ /* Win needs the Commit to ensure stream is created, otherwise the grfMode open
+ * tests below don't work
+ */
+
+ IStorage_Commit(stg,STGC_DEFAULT);
+ ok(r==S_OK, "IStorage->Commit failed\n");
+
r = IStorage_Release( stg );
ok (r == 0, "storage not released\n");
@@ -603,11 +609,23 @@ static void test_storage_refcount(void)
r = IStream_Stat( stm, &stat, STATFLAG_DEFAULT );
ok (r == STG_E_REVERTED, "stat should fail\n");
- }
-
+
r = IStream_Release(stm);
ok (r == 0, "stream not released\n");
+ /* test for grfMode open issue */
+
+ r = StgOpenStorage( filename, NULL, 0x00010020, NULL, 0, &stg);
+ ok(r==S_OK, "StgOpenStorage failed\n");
+ if(stg)
+ {
+ r = IStorage_OpenStream( stg, stmname, 0, STGM_SHARE_EXCLUSIVE|STGM_READWRITE, 0, &stm );
+ ok(r == S_OK, "OpenStream should succeed - "); printf("rc from OpenStream : %lx\n",r);
+
+ r = IStorage_Release(stg);
+ ok(r == 0, "wrong ref count\n");
+ }
+
DeleteFileW(filename);
}
More information about the wine-patches
mailing list