From 7a7849271a8d649dd744c7ceae241b3b8b412094 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Fri, 31 Jan 2025 21:21:04 +0100 Subject: [PATCH] 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 | 1 + .github/scripts/windows/test_task.bat | 4 + ext/com_dotnet/Makefile.frag.w32 | 30 +++ ext/com_dotnet/com_persist.c | 2 +- ext/com_dotnet/config.w32 | 1 + ext/com_dotnet/tests/comtest/comtest.cpp | 318 +++++++++++++++++++++++ ext/com_dotnet/tests/comtest/comtest.def | 4 + ext/com_dotnet/tests/comtest/comtest.idl | 23 ++ ext/com_dotnet/tests/gh17658.phpt | 27 ++ 9 files changed, 409 insertions(+), 1 deletion(-) create mode 100644 ext/com_dotnet/Makefile.frag.w32 create mode 100644 ext/com_dotnet/tests/comtest/comtest.cpp create mode 100644 ext/com_dotnet/tests/comtest/comtest.def create mode 100644 ext/com_dotnet/tests/comtest/comtest.idl create mode 100644 ext/com_dotnet/tests/gh17658.phpt diff --git a/.github/scripts/windows/build_task.bat b/.github/scripts/windows/build_task.bat index fd9a956bd38fb..c4134ecada519 100644 --- a/.github/scripts/windows/build_task.bat +++ b/.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 diff --git a/.github/scripts/windows/test_task.bat b/.github/scripts/windows/test_task.bat index f095260419ebc..e3db0cf8c830d 100644 --- a/.github/scripts/windows/test_task.bat +++ b/.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% diff --git a/ext/com_dotnet/Makefile.frag.w32 b/ext/com_dotnet/Makefile.frag.w32 new file mode 100644 index 0000000000000..f25e350153874 --- /dev/null +++ b/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\PHP.Test.Document /d "PHP COM Test Document" /f > NUL + @reg add HKCU\SOFTWARE\Classes\PHP.Test.Document\CLSID /d "{B13FE324-D595-44C7-97D7-82CE20EDF878}" /f > NUL + +unregister_comtest: + -@reg delete HKCU\SOFTWARE\Classes\PHP.Test.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 diff --git a/ext/com_dotnet/com_persist.c b/ext/com_dotnet/com_persist.c index c2d227eaa377a..742e007a2fba7 100644 --- a/ext/com_dotnet/com_persist.c +++ b/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); } } } diff --git a/ext/com_dotnet/config.w32 b/ext/com_dotnet/config.w32 index ebd55b1b3f066..c4fba4fc539d0 100644 --- a/ext/com_dotnet/config.w32 +++ b/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(); } diff --git a/ext/com_dotnet/tests/comtest/comtest.cpp b/ext/com_dotnet/tests/comtest/comtest.cpp new file mode 100644 index 0000000000000..58584a923934b --- /dev/null +++ b/ext/com_dotnet/tests/comtest/comtest.cpp @@ -0,0 +1,318 @@ +/* + +----------------------------------------------------------------------+ + | 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 | + | Based on: | + +----------------------------------------------------------------------+ + */ + +#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 = (IDocument*) this; + } else if (riid == IID_IDocument) { + *ppv = (IDocument *) this; + } else if (riid == IID_IDispatch) { + *ppv = (IDispatch*) this; + } else if (riid == IID_IPersistStream) { + *ppv = (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) +{ + *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) +{ + 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) +{ + 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; + char *string; + if (FAILED(pStm->Read(&cbSize, sizeof cbSize, &read))) { + return S_FALSE; + } + string = (char *) malloc(cbSize); + if (string == NULL) { + return S_FALSE; + } + if (FAILED(pStm->Read(string, cbSize, &read))) { + return S_FALSE; + } + SysFreeString(m_content); + m_content = SysAllocStringByteLen(string, cbSize); + + 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; +} diff --git a/ext/com_dotnet/tests/comtest/comtest.def b/ext/com_dotnet/tests/comtest/comtest.def new file mode 100644 index 0000000000000..1a8f9f7fc519e --- /dev/null +++ b/ext/com_dotnet/tests/comtest/comtest.def @@ -0,0 +1,4 @@ +LIBRARY comtest.dll +EXPORTS +DllGetClassObject PRIVATE +DllCanUnloadNow PRIVATE diff --git a/ext/com_dotnet/tests/comtest/comtest.idl b/ext/com_dotnet/tests/comtest/comtest.idl new file mode 100644 index 0000000000000..baf5f9ded8825 --- /dev/null +++ b/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; + } +} diff --git a/ext/com_dotnet/tests/gh17658.phpt b/ext/com_dotnet/tests/gh17658.phpt new file mode 100644 index 0000000000000..af0ab3f1acbca --- /dev/null +++ b/ext/com_dotnet/tests/gh17658.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-17658 (COMPersistHelper::LoadFromStream() segfaults for IPersistStream) +--EXTENSIONS-- +com_dotnet +--SKIPIF-- + +--FILE-- +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"; +?> +--EXPECT-- +GH-17658