June 2017 - Wine-Devel - list.winehq.org

Re: [PATCH] fusion: use correct size for ZeroMemory (GCC7)

by Dmitry Timoshkov

Marcus Meissner <meissner(a)suse.de> wrote: > diff --git a/dlls/fusion/tests/asmname.c b/dlls/fusion/tests/asmname.c > index 5fb14a4829..21cd4874ed 100644 > --- a/dlls/fusion/tests/asmname.c > +++ b/dlls/fusion/tests/asmname.c > @@ -358,7 +358,7 @@ static void test_assembly_name_props_line(IAssemblyName *name, > to_widechar(expect, vals[i].val); > > size = MAX_PATH; > - ZeroMemory(str, MAX_PATH); > + ZeroMemory(str, sizeof(str)); > hr = IAssemblyName_GetProperty(name, i, str, &size); > to_multibyte(val, str); Same problem with IAssemblyName_GetProperty() and 'size' argumenet as for the previous patch. -- Dmitry.

June 2, 2017

Re: [PATCH] use correct size for ZeroMemory (GCC7)

by Dmitry Timoshkov

Marcus Meissner <meissner(a)suse.de> wrote: > --- a/dlls/fusion/tests/asmname.c > +++ b/dlls/fusion/tests/asmname.c > @@ -358,7 +358,7 @@ static void test_assembly_name_props_line(IAssemblyName *name, > to_widechar(expect, vals[i].val); > > size = MAX_PATH; > - ZeroMemory(str, MAX_PATH); > + ZeroMemory(str, sizeof(str)); > hr = IAssemblyName_GetProperty(name, i, str, &size); > to_multibyte(val, str); According to the implementation of IAssemblyName_GetProperty() 'size' assignment needs to be fixed as well. And IAssemblyName_GetProperty() implementation should be fixed to actually take 'size' into account. -- Dmitry.

June 2, 2017

Re: [PATCH 3/4] quartz: fix crash in FilterGraph2_Render

by Nikolay Sivov

On 02.06.2017 2:41, Miklós Máté wrote: > On 31/05/17 16:32, Andrew Eikum wrote: >> Can you add a test for this? >> >> Andrew > Hi, > > I see no existing tests for invalid argument handling or for error > handling in general. I could certainly add a line in the filtergraph > test, but it wouldn't be the right thing. The right thing would be to > autogenerate null pointer handling tests for _all_ interfaces that > receive pointer arguments. Unfortunately, I don't have the resources to > do that now. It's done one by one, and most of the time after something crashed. Autogenerated way will cover more definitely, but it's usually not that simple, because some arguments could be optional, and for some Windows crashes as well, so we should too. > > MM > >> >> On Thu, May 25, 2017 at 04:54:49PM +0200, Miklós Máté wrote: >>> Note: there are a lot of other places as well where the null pointer >>> handling >>> is present in the msdn docs but missing in wine. >>> >>> Signed-off-by: Miklós Máté <mtmkls(a)gmail.com> >>> --- >>> dlls/quartz/filtergraph.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/dlls/quartz/filtergraph.c b/dlls/quartz/filtergraph.c >>> index a143b05d83..e26eb199c8 100644 >>> --- a/dlls/quartz/filtergraph.c >>> +++ b/dlls/quartz/filtergraph.c >>> @@ -1279,6 +1279,9 @@ static HRESULT WINAPI >>> FilterGraph2_Render(IFilterGraph2 *iface, IPin *ppinOut) >>> TRACE("(%p/%p)->(%p)\n", This, iface, ppinOut); >>> + if (!ppinOut) >>> + return E_POINTER; >>> + >>> if (TRACE_ON(quartz)) >>> { >>> PIN_INFO PinInfo; >>> -- >>> 2.11.0 >>> >>> >>> > > >

June 2, 2017

Re: [PATCH 3/4] quartz: fix crash in FilterGraph2_Render

by Miklós Máté

On 31/05/17 16:32, Andrew Eikum wrote: > Can you add a test for this? > > Andrew Hi, I see no existing tests for invalid argument handling or for error handling in general. I could certainly add a line in the filtergraph test, but it wouldn't be the right thing. The right thing would be to autogenerate null pointer handling tests for _all_ interfaces that receive pointer arguments. Unfortunately, I don't have the resources to do that now. MM > > On Thu, May 25, 2017 at 04:54:49PM +0200, Miklós Máté wrote: >> Note: there are a lot of other places as well where the null pointer handling >> is present in the msdn docs but missing in wine. >> >> Signed-off-by: Miklós Máté <mtmkls(a)gmail.com> >> --- >> dlls/quartz/filtergraph.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/dlls/quartz/filtergraph.c b/dlls/quartz/filtergraph.c >> index a143b05d83..e26eb199c8 100644 >> --- a/dlls/quartz/filtergraph.c >> +++ b/dlls/quartz/filtergraph.c >> @@ -1279,6 +1279,9 @@ static HRESULT WINAPI FilterGraph2_Render(IFilterGraph2 *iface, IPin *ppinOut) >> >> TRACE("(%p/%p)->(%p)\n", This, iface, ppinOut); >> >> + if (!ppinOut) >> + return E_POINTER; >> + >> if (TRACE_ON(quartz)) >> { >> PIN_INFO PinInfo; >> -- >> 2.11.0 >> >> >>

June 1, 2017

Re: [PATCH 1/4] quartz: AsyncReader is always expected to have output pin

by Miklós Máté

On 31/05/17 16:32, Andrew Eikum wrote: > Thanks for working on this. I tested your patch sequence against a > number of games as well as embedding videos in MSOffice 2007 and 2010 > and found no regressions. I do have some comments, though, in-line > below. Hi, Thanks for the review. > > Is this something you could write a test for? E.g. create the filter > and then query for its output pin immediately after creation. Certainly. > > On Thu, May 25, 2017 at 04:54:47PM +0200, Miklós Máté wrote: >> Even between create() and Load(). This fixes a crash in DSPlayer. >> >> Signed-off-by: Miklós Máté <mtmkls(a)gmail.com> >> --- >> dlls/quartz/filesource.c | 31 ++++++++++++++++++++----------- >> 1 file changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/dlls/quartz/filesource.c b/dlls/quartz/filesource.c >> index ed7e5ee201..200463ff5b 100644 >> --- a/dlls/quartz/filesource.c >> +++ b/dlls/quartz/filesource.c >> @@ -73,7 +73,8 @@ static const IFileSourceFilterVtbl FileSource_Vtbl; >> static const IAsyncReaderVtbl FileAsyncReader_Vtbl; >> static const IAMFilterMiscFlagsVtbl IAMFilterMiscFlags_Vtbl; >> >> -static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin); >> +static HRESULT FileAsyncReader_Construct(IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin); >> +static HRESULT FileAsyncReader_SetFile(IAsyncReader * iface, HANDLE hFile); >> >> static const WCHAR mediatype_name[] = { >> 'M', 'e', 'd', 'i', 'a', ' ', 'T', 'y', 'p', 'e', 0 }; >> @@ -416,6 +417,7 @@ static const BaseFilterFuncTable BaseFuncTable = { >> HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv) >> { >> AsyncReader *pAsyncRead; >> + HRESULT hr; >> >> if( pUnkOuter ) >> return CLASS_E_NOAGGREGATION; >> @@ -429,7 +431,8 @@ HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv) >> >> pAsyncRead->IFileSourceFilter_iface.lpVtbl = &FileSource_Vtbl; >> pAsyncRead->IAMFilterMiscFlags_iface.lpVtbl = &IAMFilterMiscFlags_Vtbl; >> - pAsyncRead->pOutputPin = NULL; >> + hr = FileAsyncReader_Construct(&pAsyncRead->filter.IBaseFilter_iface, &pAsyncRead->filter.csFilter, &pAsyncRead->pOutputPin); >> + BaseFilterImpl_IncrementPinVersion(&pAsyncRead->filter); > If this fails, I think we should free resources and return the error > immediately. Yes, you're right. > >> >> pAsyncRead->pszFileName = NULL; >> pAsyncRead->pmt = NULL; >> @@ -438,7 +441,7 @@ HRESULT AsyncReader_create(IUnknown * pUnkOuter, LPVOID * ppv) >> >> TRACE("-- created at %p\n", pAsyncRead); >> >> - return S_OK; >> + return hr; >> } >> >> /** IUnknown methods **/ >> @@ -606,7 +609,7 @@ static ULONG WINAPI FileSource_Release(IFileSourceFilter * iface) >> >> static HRESULT WINAPI FileSource_Load(IFileSourceFilter * iface, LPCOLESTR pszFileName, const AM_MEDIA_TYPE * pmt) >> { >> - HRESULT hr; >> + HRESULT hr = E_FAIL; >> HANDLE hFile; >> IAsyncReader * pReader = NULL; >> AsyncReader *This = impl_from_IFileSourceFilter(iface); >> @@ -625,16 +628,15 @@ static HRESULT WINAPI FileSource_Load(IFileSourceFilter * iface, LPCOLESTR pszFi >> return HRESULT_FROM_WIN32(GetLastError()); >> } >> >> - /* create pin */ >> - hr = FileAsyncReader_Construct(hFile, &This->filter.IBaseFilter_iface, &This->filter.csFilter, &This->pOutputPin); >> - BaseFilterImpl_IncrementPinVersion(&This->filter); >> >> - if (SUCCEEDED(hr)) >> + if (This->pOutputPin) > Since we now create the output pin during filter creation, this should > never be non-NULL, right? I wasn't entirely sure that the pin can't get deleted somehow. > >> hr = IPin_QueryInterface(This->pOutputPin, &IID_IAsyncReader, (LPVOID *)&pReader); >> >> /* store file name & media type */ >> if (SUCCEEDED(hr)) >> { >> + FileAsyncReader_SetFile(pReader, hFile); >> + >> CoTaskMemFree(This->pszFileName); >> if (This->pmt) >> FreeMediaType(This->pmt); >> @@ -939,7 +941,7 @@ static const BaseOutputPinFuncTable output_BaseOutputFuncTable = { >> BaseOutputPinImpl_BreakConnect >> }; >> >> -static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin) >> +static HRESULT FileAsyncReader_Construct(IBaseFilter * pBaseFilter, LPCRITICAL_SECTION pCritSec, IPin ** ppPin) >> { >> PIN_INFO piOutput; >> HRESULT hr; >> @@ -952,9 +954,8 @@ static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter >> >> if (SUCCEEDED(hr)) >> { >> - FileAsyncReader *pPinImpl = (FileAsyncReader *)*ppPin; >> + FileAsyncReader *pPinImpl = impl_from_IPin(*ppPin); >> pPinImpl->IAsyncReader_iface.lpVtbl = &FileAsyncReader_Vtbl; >> - pPinImpl->hFile = hFile; >> pPinImpl->bFlushing = FALSE; >> pPinImpl->sample_list = NULL; >> pPinImpl->handle_list = NULL; >> @@ -965,6 +966,14 @@ static HRESULT FileAsyncReader_Construct(HANDLE hFile, IBaseFilter * pBaseFilter >> return hr; >> } >> >> +static HRESULT FileAsyncReader_SetFile(IAsyncReader * iface, HANDLE hFile) >> +{ >> + FileAsyncReader *This = impl_from_IAsyncReader(iface); >> + >> + This->hFile = hFile; >> + return S_OK; >> +} >> + > Why is this a separate function instead of just setting hFile in > FileSource_Load? Setting the file handler is at line 643, but struct FileAsyncReader is defined on line 745. I didn't want to rearrange the file for this. MM > >> /* IAsyncReader */ >> >> static HRESULT WINAPI FileAsyncReader_QueryInterface(IAsyncReader * iface, REFIID riid, LPVOID * ppv) >> -- >> 2.11.0 >> >> >>

June 1, 2017

Re: ntdll: On macOS, don't use clock_gettime() even if it's available.

by Ken Thomases

On Jun 1, 2017, at 3:19 PM, Michael Müller <michael(a)fds-team.de> wrote: > > Not sure which one of two commands is causing the lower overhead, but > shouldn't you just build Wine using the 10.11 SDK if you plan to deploy > it on 10.11? No. That's not the way Mac software is typically built. The whole point of a deployment target[1] is that it's often different from the SDK against which you're building. You typically build against the newest SDK. (Apple used to ship a few old SDKs with each version of Xcode but stopped doing that and encourages everyone to use the latest.) [1] https://developer.apple.com/library/content/documentation/DeveloperTools/Co… Ideally, the code would check for the run-time availability of clock_gettime() and use it when it's available (assuming a build against the 10.12 SDK) and fall back to mach_absolute_time() otherwise. Alexandre expressed a preference for the approach in my patch, instead. > This could theoretically also apply to other parts of Wine > and I don't think it makes sense to add backwards compatibility hacks > everywhere. Theoretically, it could. In practice, it's not been a big problem. -Ken

June 1, 2017

Re: ntdll: On macOS, don't use clock_gettime() even if it's available.

by Michael Müller

Not sure which one of two commands is causing the lower overhead, but shouldn't you just build Wine using the 10.11 SDK if you plan to deploy it on 10.11? This could theoretically also apply to other parts of Wine and I don't think it makes sense to add backwards compatibility hacks everywhere. Regards, Michael

June 1, 2017

Re: [PATCH] msacm32: Correctly allocate WAVEFORMATEX.

by Andrew Eikum

This doesn't look right to me. According to MSDN[1], the cbFormatSize field should already be the full size of the format struct. It's possible MSDN is wrong, but we'd need tests to prove that first. Who sets cbFormatSize? Could the error be there? [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd742918(v=vs.85).… Andrew On Wed, May 31, 2017 at 06:03:58PM -0500, Zebediah Figura wrote: > Fixes https://bugs.winehq.org/show_bug.cgi?id=26388 > > Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> > --- > dlls/msacm32/format.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/dlls/msacm32/format.c b/dlls/msacm32/format.c > index de07cba9a3..ec46cae163 100644 > --- a/dlls/msacm32/format.c > +++ b/dlls/msacm32/format.c > @@ -75,7 +75,7 @@ static BOOL CALLBACK MSACM_FillFormatTagsCB(HACMDRIVERID hadid, > > afd.cbStruct = sizeof(afd); > afd.dwFormatTag = paftd->dwFormatTag; > - afd.pwfx = HeapAlloc(MSACM_hHeap, 0, paftd->cbFormatSize); > + afd.pwfx = HeapAlloc(MSACM_hHeap, 0, sizeof(WAVEFORMATEX) + paftd->cbFormatSize); > if (!afd.pwfx) return FALSE; > afd.pwfx->wFormatTag = paftd->dwFormatTag; > afd.pwfx->cbSize = paftd->cbFormatSize; > -- > 2.13.0 > > >

June 1, 2017

Re: [PATCH] user32: Keep tracking if we just created the menu.

by Zebediah Figura

On 06/01/2017 02:51 AM, Huw Davies wrote: > On Wed, May 31, 2017 at 04:13:42PM -0500, Zebediah Figura wrote: >> On 05/31/2017 03:30 PM, Huw Davies wrote: >>> On Wed, May 31, 2017 at 02:34:06PM -0500, Zebediah Figura wrote: >>>> Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> >>>> --- >>>> dlls/user32/menu.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/dlls/user32/menu.c b/dlls/user32/menu.c >>>> index a65f01eaa5..c52b723f76 100644 >>>> --- a/dlls/user32/menu.c >>>> +++ b/dlls/user32/menu.c >>>> @@ -2597,7 +2597,10 @@ static BOOL MENU_ButtonDown( MTRACKER* pmt, HMENU hPtMenu, UINT wFlags ) >>>> /* If the popup menu is not already "popped" */ >>>> if (!(ptmenu->items[pos].fState & MF_MOUSESELECT)) >>>> + { >>>> pmt->hCurrentMenu = MENU_ShowSubPopup( pmt->hOwnerWnd, hPtMenu, FALSE, wFlags ); >>>> + return TRUE; >>>> + } >>>> } >>>> /* A click on an item or anywhere on a popup keeps tracking going */ >>> >>> So under what conditions does the if statement directly below this >>> hunk not get triggered? >>> >>> Huw. >>> >> >> When we clicked outside the menu (hPtMenu == NULL), or when we clicked on >> the menu bar outside of a menu (hPtMenu != NULL, but ht == border && not a >> popup). I might be missing something, so if anything in particular looks >> problematic, please let me know. > > I'm confused. If one clicks on the menu bar outside of a menu, I'd expect > the tracking to stop. What exactly are you trying to fix? Can this be > seen using Wine's notepad? > > Thanks, > Huw. > The problem is that the menu which appears on clicking the title bar, and contains elements such as minimize/restore/move/etc., is destroyed as soon as it is created. To replicate this, start notepad (or anything else) in a virtual window, then try to click the window icon in the upper left hand corner.

June 1, 2017

Re: [PATCH] user32: Keep tracking if we just created the menu.

by Huw Davies

On Wed, May 31, 2017 at 04:13:42PM -0500, Zebediah Figura wrote: > On 05/31/2017 03:30 PM, Huw Davies wrote: > >On Wed, May 31, 2017 at 02:34:06PM -0500, Zebediah Figura wrote: > >>Signed-off-by: Zebediah Figura <z.figura12(a)gmail.com> > >>--- > >> dlls/user32/menu.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >>diff --git a/dlls/user32/menu.c b/dlls/user32/menu.c > >>index a65f01eaa5..c52b723f76 100644 > >>--- a/dlls/user32/menu.c > >>+++ b/dlls/user32/menu.c > >>@@ -2597,7 +2597,10 @@ static BOOL MENU_ButtonDown( MTRACKER* pmt, HMENU hPtMenu, UINT wFlags ) > >> /* If the popup menu is not already "popped" */ > >> if (!(ptmenu->items[pos].fState & MF_MOUSESELECT)) > >>+ { > >> pmt->hCurrentMenu = MENU_ShowSubPopup( pmt->hOwnerWnd, hPtMenu, FALSE, wFlags ); > >>+ return TRUE; > >>+ } > >> } > >> /* A click on an item or anywhere on a popup keeps tracking going */ > > > >So under what conditions does the if statement directly below this > >hunk not get triggered? > > > >Huw. > > > > When we clicked outside the menu (hPtMenu == NULL), or when we clicked on > the menu bar outside of a menu (hPtMenu != NULL, but ht == border && not a > popup). I might be missing something, so if anything in particular looks > problematic, please let me know. I'm confused. If one clicks on the menu bar outside of a menu, I'd expect the tracking to stop. What exactly are you trying to fix? Can this be seen using Wine's notepad? Thanks, Huw.

June 1, 2017