Fix GH-17658: COMPersistHelper::LoadFromStream() can segfault (GH-17659)

* Fix GH-17658: COMPersistHelper::LoadFromStream() can segfault

The actual fix is trivial, but to be able to test the behavior we have
to introduce an own COM object, since existing persistable objects
likely implement `IPersistInit`, not only `IPersist`.  We also want to
avoid further test dependencies on possibly unavailable objects, such
as `Word.Application`.

To this purposes, we add a small COM in-process server, which may be
extended for other testing purposes.  We keep it simple by implementing
it in C++, but without using any more sophisticated frameworks like ATL.

This component needs to be built explicitly (`nmake comtest.dll`), and
also needs to be explicitly registered (`nmake register_comtest`).
When no longer needed, it is possible to unregister the component
(`nmake unregister_comtest`).

.github/scripts/windows/build_task.bat

@@ -45,6 +45,7 @@ cmd /c configure.bat ^
 if %errorlevel% neq 0 exit /b 3
 
 nmake /NOLOGO
+nmake /NOLOGO comtest.dll
 if %errorlevel% neq 0 exit /b 3
 
 exit /b 0

.github/scripts/windows/test_task.bat

@@ -118,6 +118,9 @@ hMailServer.exe /verysilent
 cd %APPVEYOR_BUILD_FOLDER%
 %PHP_BUILD_DIR%\php.exe -dextension_dir=%PHP_BUILD_DIR% -dextension=com_dotnet .github\setup_hmailserver.php
 
+rem prepare for com_dotnet
+nmake register_comtest
+
 mkdir %PHP_BUILD_DIR%\test_file_cache
 rem generate php.ini
 echo extension_dir=%PHP_BUILD_DIR% > %PHP_BUILD_DIR%\php.ini
@@ -146,6 +149,7 @@ nmake test TESTS="%OPCACHE_OPTS% -g FAIL,BORK,LEAK,XLEAK %ASAN_OPTS% --no-progre
 
 set EXIT_CODE=%errorlevel%
 
+nmake unregister_comtest
 taskkill /f /im snmpd.exe
 
 exit /b %EXIT_CODE%

ext/com_dotnet/Makefile.frag.w32

@@ -0,0 +1,30 @@
+$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest_i.c: ext\com_dotnet\tests\comtest\comtest.idl
+	-md $(BUILD_DIR)\ext\com_dotnet\tests\comtest
+	midl /nologo /h $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.h /iid $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest_i.c /tlb $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.tlb ext\com_dotnet\tests\comtest\comtest.idl
+
+$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.obj $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest_i.obj: ext\com_dotnet\tests\comtest\comtest.cpp $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest_i.c
+	$(PHP_CL) /nologo /c /Fo$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.obj /I $(BUILD_DIR)\ext\com_dotnet\tests\comtest ext\com_dotnet\tests\comtest\comtest.cpp
+	$(PHP_CL) /nologo /c /Fo$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest_i.obj $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest_i.c
+
+$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.dll: $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.obj $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest_i.obj ext\com_dotnet\tests\comtest\comtest.def
+	"$(LINK)" /nologo /dll /out:$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.dll $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.obj $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest_i.obj /def:ext\com_dotnet\tests\comtest\comtest.def OleAut32.lib
+
+comtest.dll: $(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.dll
+
+register_comtest:
+	@reg add HKCU\SOFTWARE\Classes\TypeLib\{AE8685BE-3758-4BDA-91DB-1459EBA24747} /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\TypeLib\{AE8685BE-3758-4BDA-91DB-1459EBA24747}\1.0 /d "PHP COM Test Library" /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\TypeLib\{AE8685BE-3758-4BDA-91DB-1459EBA24747}\1.0\0 /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\TypeLib\{AE8685BE-3758-4BDA-91DB-1459EBA24747}\1.0\0\win32 /d "$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.tlb" /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\TypeLib\{AE8685BE-3758-4BDA-91DB-1459EBA24747}\1.0\0\win64 /d "$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.tlb" /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\TypeLib\{AE8685BE-3758-4BDA-91DB-1459EBA24747}\1.0\FLAGS /d 0 /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\TypeLib\{AE8685BE-3758-4BDA-91DB-1459EBA24747}\1.0\HELPDIR /d "$(BUILD_DIR)\ext\com_dotnet\tests\comtest" /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\CLSID\{B13FE324-D595-44C7-97D7-82CE20EDF878} /d "PHP COM Test Document" /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\CLSID\{B13FE324-D595-44C7-97D7-82CE20EDF878}\InprocServer32 /d "$(BUILD_DIR)\ext\com_dotnet\tests\comtest\comtest.dll" /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\PHPTest.Document /d "PHP COM Test Document" /f > NUL
+	@reg add HKCU\SOFTWARE\Classes\PHPTest.Document\CLSID /d "{B13FE324-D595-44C7-97D7-82CE20EDF878}" /f > NUL
+
+unregister_comtest:
+	-@reg delete HKCU\SOFTWARE\Classes\PHPTest.Document /f > NUL
+	-@reg delete HKCU\SOFTWARE\Classes\CLSID\{B13FE324-D595-44C7-97D7-82CE20EDF878} /f > NUL
+	-@reg delete HKCU\SOFTWARE\Classes\TypeLib\{AE8685BE-3758-4BDA-91DB-1459EBA24747} /f > NUL

ext/com_dotnet/com_persist.c

@@ -557,7 +557,7 @@ CPH_METHOD(LoadFromStream)
 		} else {
 			res = get_persist_stream(helper);
 			if (helper->ips) {
-				res = IPersistStreamInit_Load(helper->ipsi, stm);
+				res = IPersistStream_Load(helper->ips, stm);
 			}
 		}
 	}

ext/com_dotnet/config.w32

@@ -10,4 +10,5 @@ if (PHP_COM_DOTNET == "yes") {
 		null, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
 	AC_DEFINE('HAVE_COM_DOTNET', 1, "Define to 1 if the PHP extension 'com_dotnet' is available.");
 	CHECK_HEADER_ADD_INCLUDE('mscoree.h', 'CFLAGS_COM_DOTNET');
+	ADD_MAKEFILE_FRAGMENT();
 }

ext/com_dotnet/tests/comtest/comtest.cpp

@@ -0,0 +1,317 @@
+/*
+   +----------------------------------------------------------------------+
+   | Copyright (c) The PHP Group                                          |
+   +----------------------------------------------------------------------+
+   | This source file is subject to version 3.01 of the PHP license,      |
+   | that is bundled with this package in the file LICENSE, and is        |
+   | available through the world-wide-web at the following url:           |
+   | https://www.php.net/license/3_01.txt                                 |
+   | If you did not receive a copy of the PHP license and are unable to   |
+   | obtain it through the world-wide-web, please send a note to          |
+   | license@php.net so we can mail you a copy immediately.               |
+   +----------------------------------------------------------------------+
+   | Author: Christoph M. Becker  <cmb@php.net>                           |
+   | Based on: <https://thrysoee.dk/InsideCOM+/>                          |
+   +----------------------------------------------------------------------+
+ */
+
+#include "comtest.h" // Generated by MIDL
+
+long g_cComponents = 0;
+long g_cLocks = 0;
+
+class CDocument : public IDocument, IPersistStream
+{
+public:
+	// IUnknown
+	STDMETHODIMP_(ULONG) AddRef();
+	STDMETHODIMP_(ULONG) Release();
+	STDMETHODIMP QueryInterface(REFIID riid, void** ppv);
+
+	// IDispatch
+	STDMETHODIMP GetTypeInfoCount(UINT* pCountTypeInfo);
+	STDMETHODIMP GetTypeInfo(UINT iTypeInfo, LCID lcid, ITypeInfo** ppITypeInfo);
+	STDMETHODIMP GetIDsOfNames(REFIID riid, LPOLESTR* rgszNames, UINT cNames, LCID lcid, DISPID* rgDispId);
+	STDMETHODIMP Invoke(DISPID dispIdMember, REFIID riid, LCID lcid, WORD wFlags, DISPPARAMS* pDispParams,
+		VARIANT* pVarResult, EXCEPINFO* pExcepInfo, UINT* puArgErr);
+
+	// IDocument
+	STDMETHODIMP get_Content(BSTR* retvalue);
+	STDMETHODIMP put_Content(BSTR newvalue);
+
+	// IPersist
+	STDMETHODIMP GetClassID(CLSID *pClassID);
+
+	// IPersistStream
+	STDMETHODIMP IsDirty( void);
+	STDMETHODIMP Load(IStream *pStm);
+	STDMETHODIMP Save(IStream *pStm, BOOL fClearDirty);
+	STDMETHODIMP GetSizeMax(ULARGE_INTEGER *pcbSize);
+
+	CDocument() : m_content(SysAllocString(L"")), m_cRef(1) { g_cComponents++; }
+	~CDocument() { SysFreeString(m_content); g_cComponents--; }
+	HRESULT Init(void);
+
+private:
+	BSTR m_content;
+	ULONG m_cRef;
+	ITypeInfo* m_pTypeInfo;
+	BOOL m_dirty;
+};
+
+ULONG CDocument::AddRef()
+{
+	return ++m_cRef;
+}
+
+ULONG CDocument::Release()
+{
+	if (--m_cRef != 0) {
+		return m_cRef;
+	}
+	m_pTypeInfo->Release();
+	delete this;
+	return 0;
+}
+
+HRESULT CDocument::QueryInterface(REFIID riid, void** ppv)
+{
+	if (riid == IID_IUnknown) {
+		*ppv = static_cast<IDocument*>(this);
+	} else if (riid == IID_IDocument) {
+		*ppv = static_cast<IDocument*>(this);
+	} else if (riid == IID_IDispatch) {
+		*ppv = static_cast<IDispatch*>(this);
+	} else if (riid == IID_IPersistStream) {
+		*ppv = static_cast<IPersistStream*>(this);
+	} else {
+		*ppv = NULL;
+		return E_NOINTERFACE;
+	}
+	AddRef();
+	return S_OK;
+}
+
+HRESULT CDocument::GetTypeInfoCount(UINT* pCountTypeInfo)
+{
+	*pCountTypeInfo = 1;
+	return S_OK;
+}
+
+HRESULT CDocument::GetTypeInfo(UINT iTypeInfo, LCID lcid, ITypeInfo** ppITypeInfo)
+{
+	(void) lcid;
+	*ppITypeInfo = NULL;
+	if (iTypeInfo != 0) {
+		return DISP_E_BADINDEX;
+	}
+	m_pTypeInfo->AddRef();
+	*ppITypeInfo = m_pTypeInfo;
+	return S_OK;
+}
+
+HRESULT CDocument::GetIDsOfNames(REFIID riid, LPOLESTR* rgszNames, UINT cNames, LCID lcid, DISPID* rgDispId)
+{
+	(void) lcid;
+	if (riid != IID_NULL) {
+		return DISP_E_UNKNOWNINTERFACE;
+	}
+	return DispGetIDsOfNames(m_pTypeInfo, rgszNames, cNames, rgDispId);
+}
+
+HRESULT CDocument::Invoke(DISPID dispIdMember, REFIID riid, LCID lcid, WORD wFlags, DISPPARAMS* pDispParams,
+	VARIANT* pVarResult, EXCEPINFO* pExcepInfo, UINT* puArgErr)
+{
+	(void) lcid;
+	if (riid != IID_NULL) {
+		return DISP_E_UNKNOWNINTERFACE;
+	}
+	return DispInvoke(this, m_pTypeInfo, dispIdMember, wFlags, pDispParams, pVarResult, pExcepInfo, puArgErr); 
+}
+
+HRESULT CDocument::get_Content(BSTR* retvalue)
+{
+	*retvalue = SysAllocString(m_content);
+	return S_OK;
+}
+
+HRESULT CDocument::put_Content(BSTR newvalue)
+{
+	SysFreeString(m_content);
+	m_content = SysAllocString(newvalue);
+	return S_OK;
+}
+
+HRESULT CDocument::Init(void)
+{
+	ITypeLib* pTypeLib;
+	if (FAILED(LoadRegTypeLib(LIBID_ComTest, 1, 0, LANG_NEUTRAL, &pTypeLib))) {
+		return E_FAIL;
+	}
+	HRESULT hr = pTypeLib->GetTypeInfoOfGuid(IID_IDocument, &m_pTypeInfo);
+	if (FAILED(hr)) {
+		pTypeLib->Release();
+		return hr;
+	}
+
+	pTypeLib->Release();
+	return S_OK;
+}
+
+HRESULT CDocument::GetClassID(CLSID *pClassID)
+{
+	*pClassID = CLSID_Document;
+	return S_OK;
+}
+
+HRESULT CDocument::IsDirty(void)
+{
+	return m_dirty ? S_OK : S_FALSE;
+}
+
+HRESULT CDocument::Load(IStream *pStm)
+{
+	ULONG read = 0;
+	ULONG cbSize;
+
+	if (FAILED(pStm->Read(&cbSize, sizeof cbSize, &read))) {
+		return S_FALSE;
+	}
+	if (!SysReAllocStringLen(&m_content, NULL, cbSize)) {
+		return S_FALSE;
+	}
+	if (FAILED(pStm->Read(m_content, cbSize, &read))) {
+		// we may have garbage in m_content, but don't mind
+		return S_FALSE;
+	}
+	m_dirty = FALSE;
+	return S_OK;
+}
+
+HRESULT CDocument::Save(IStream *pStm, BOOL fClearDirty)
+{
+	ULONG written = 0;
+	ULONG cbSize = SysStringByteLen(m_content);
+	HRESULT hr;
+	if (FAILED(hr = pStm->Write(&cbSize, sizeof cbSize, &written))) {
+		return S_FALSE;
+	}
+	if (FAILED(hr = pStm->Write(m_content, cbSize, &written))) {
+		return S_FALSE;
+	}
+
+	if (fClearDirty) {
+		m_dirty = FALSE;
+	}
+
+	return S_OK;
+}
+
+HRESULT CDocument::GetSizeMax(ULARGE_INTEGER *pcbSize)
+{
+	(*pcbSize).QuadPart = sizeof(ULONG) + SysStringByteLen(m_content);
+	return S_OK;
+}
+
+class CDocumentFactory : public IClassFactory
+{
+public:
+	// IUnknown
+	STDMETHODIMP_(ULONG) AddRef();
+	STDMETHODIMP_(ULONG) Release();
+	STDMETHODIMP QueryInterface(REFIID riid, void** ppv);
+
+	// IClassFactory
+	STDMETHODIMP CreateInstance(IUnknown* pUnknownOuter, REFIID riid, void** ppv);
+	STDMETHODIMP LockServer(BOOL bLock);
+
+	CDocumentFactory() : m_cRef(1) { g_cLocks++; }
+	~CDocumentFactory() { g_cLocks--; }
+
+private:
+	ULONG m_cRef;
+};
+
+ULONG CDocumentFactory::AddRef()
+{
+	return ++m_cRef;
+}
+
+ULONG CDocumentFactory::Release()
+{
+	if (--m_cRef != 0) {
+		return m_cRef;
+	}
+	delete this;
+	return 0;
+}
+
+HRESULT CDocumentFactory::QueryInterface(REFIID riid, void** ppv)
+{
+	if (riid == IID_IUnknown) {
+		*ppv = (IUnknown*)this;
+	} else if (riid == IID_IClassFactory) {
+		*ppv = (IClassFactory*)this;
+	} else {
+		*ppv = NULL;
+		return E_NOINTERFACE;
+	}
+	AddRef();
+	return S_OK;
+}
+
+HRESULT CDocumentFactory::CreateInstance(IUnknown *pUnknownOuter, REFIID riid, void** ppv)
+{
+	if (pUnknownOuter != NULL) {
+		return CLASS_E_NOAGGREGATION;
+	}
+
+	CDocument *pDocument = new CDocument;
+	if (pDocument == NULL) {
+		return E_OUTOFMEMORY;
+	}
+
+	HRESULT hr;
+	if (FAILED(hr = pDocument->Init())) {
+		return hr;
+	}
+	hr = pDocument->QueryInterface(riid, ppv);
+	pDocument->Release();
+	return hr;
+}
+
+HRESULT CDocumentFactory::LockServer(BOOL bLock)
+{
+	if (bLock) {
+		g_cLocks++;
+	} else {
+		g_cLocks --;
+	}
+	return S_OK;
+}
+
+HRESULT __stdcall DllCanUnloadNow()
+{
+	if (g_cLocks == 0) {
+		return S_OK;
+	} else {
+		return S_FALSE;
+	}
+}
+
+HRESULT __stdcall DllGetClassObject(REFCLSID clsid, REFIID riid, void** ppv)
+{
+	if (clsid != CLSID_Document) {
+		return CLASS_E_CLASSNOTAVAILABLE;
+	}
+
+	CDocumentFactory* pFactory = new CDocumentFactory;
+	if (pFactory == NULL) {
+		return E_OUTOFMEMORY;
+	}
+
+	// riid is probably IID_IClassFactory.
+	HRESULT hr = pFactory->QueryInterface(riid, ppv);
+	pFactory->Release();
+	return hr;
+}

ext/com_dotnet/tests/comtest/comtest.def

@@ -0,0 +1,4 @@
+LIBRARY comtest.dll
+EXPORTS
+DllGetClassObject PRIVATE
+DllCanUnloadNow   PRIVATE

ext/com_dotnet/tests/comtest/comtest.idl

@@ -0,0 +1,23 @@
+[ uuid(AE8685BE-3758-4BDA-91DB-1459EBA24747),
+  helpstring("PHP COM Test Library"),
+  version(1.0) ]
+library ComTest
+{
+    importlib("stdole32.tlb");
+
+    [ object,
+      uuid(66177FCA-36B3-436B-B475-BE4249DDE0A0),
+      dual ]
+    interface IDocument : IDispatch
+    {
+        [id(1), propget] HRESULT Content([out, retval] BSTR* retvalue);
+        [id(1), propput] HRESULT Content(BSTR newvalue);
+    }
+
+    [ uuid(B13FE324-D595-44C7-97D7-82CE20EDF878) ]
+    coclass Document
+    {
+        interface IDocument;
+        interface IPersistStream;
+    }
+}

ext/com_dotnet/tests/gh17658.phpt

@@ -0,0 +1,36 @@
+--TEST--
+GH-17658 (COMPersistHelper::LoadFromStream() segfaults for IPersistStream)
+--EXTENSIONS--
+com_dotnet
+--SKIPIF--
+<?php
+try {
+    new com("PHPTest.Document");
+} catch (com_exception $ex) {
+    die("skip PHPTest.Document not available (" . dechex($ex->getCode()) . ")");
+}
+?>
+--FILE--
+<?php
+$doc = new com("PHPTest.Document");
+$doc->Content = "GH-17658";
+$ph = new COMPersistHelper($doc);
+$stream = fopen("php://memory", "w+");
+$ph->SaveToStream($stream);
+$doc->Content = "";
+fseek($stream, 0);
+$ph->LoadFromStream($stream);
+fclose($stream);
+echo $doc->Content, "\n";
+
+// verify that PHP.Test.Document does not implement IPersistStreamInit
+try {
+    $ph->InitNew();
+} catch (com_exception $ex) {
+    // supposed to fail with E_NOINTERFACE
+    echo dechex($ex->getCode()), "\n";
+}
+?>
+--EXPECT--
+GH-17658
+80004002