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

Re: [PATCH 3/4] quartz: fix crash in FilterGraph2_Render
by Andrew Eikum June 2, 2017

June 2, 2017

Can you add a test for this? Andrew 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 > > >

3 2

Re: [PATCH 1/4] quartz: AsyncReader is always expected to have output pin
by Andrew Eikum June 1, 2017

June 1, 2017

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. Is this something you could write a test for? E.g. create the filter and then query for its output pin immediately after creation. 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. > > 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? > 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? > /* IAsyncReader */ > > static HRESULT WINAPI FileAsyncReader_QueryInterface(IAsyncReader * iface, REFIID riid, LPVOID * ppv) > -- > 2.11.0 > > >

2 1

Re: PATCH 1/7] msvcrt: Translate SEH exception on each call to __CxxFrameHandler.
by Piotr Caban May 31, 2017

May 31, 2017

On 05/24/17 02:54, Daniel Lehman wrote: > +extern void __wine_rtl_unwind( EXCEPTION_REGISTRATION_RECORD* frame, EXCEPTION_RECORD *record, > + void (*target)(void) ); It's already defined in wine/exception.h. > +extern void CDECL __DestructExceptionObject(EXCEPTION_RECORD *rec); This is also already defined (in msvcrt.h) > + > +static void DECLSPEC_NORETURN seh_unwind_target(void) > +{ > + seh_frame_info *seh_frame = (seh_frame_info *)__wine_get_frame(); > + __wine_pop_frame( &seh_frame->frame ); There's no need to call __wine_pop_frame here, it will be done after the jump anyway. I think that the whole series will be cleaner/easier to read if we add the __EXCEPT_CTX/__FINALLY_CTX macros and use them here. I'm attaching a patch with proposition of changes for this functions. With such changes your patch will look like this (this is not tested): typedef struct { ULONG64 dest_frame; ULONG64 orig_frame; DISPATCHER_CONTEXT *dispatch; const cxx_function_descr *descr; } se_translator_ctx; static DWORD se_translation_filter(EXCEPTION_POINTERS *ep, void *c) { se_translator_ctx *ctx = (se_translator_ctx*)c; /* this is copied from original patch but I think that */ /* terminate should be called on non C++ exceptions here */ if (ep->rec->ExceptionCode != CXX_EXCEPTION) return ExceptionContinueSearch; find_catch_block(...); __DestructExceptionObject(ep->rec); return ExceptionExecuteHandler; } in cxx_frame_handler: if (data->se_translator) { se_translator_ctx ctx; //initialize ctx __TRY { except_ptrs.ExceptionRecord = rec; except_ptrs.ContextRecord = context; data->se_translator(rec->ExceptionCode, &except_ptrs); } __EXCEPT_CTX(se_translation_filter, &ctx) { } __END_TRY } With such changes second patch can be also changed to use __TRY/__FINALLY_CTX around "ret_addr = handler(0, frame);" file. What do you think about it? Thanks, Piotr

2 1

Re: [PATCH 6/6] d3dx9: Merge constant setting for child parameters when possible.
by Matteo Bruni May 31, 2017

May 31, 2017

2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > + if (!(const_set->table == current_table && current_start_offset == start_offset > + && const_set->direct_copy == first_const->direct_copy > + && current_data == const_set->param->data > + && (const_set->direct_copy || (first_const->param->type == const_set->param->type > + && first_const->param->class == const_set->param->class > + && first_const->param->columns == const_set->param->columns > + && first_const->param->rows == const_set->param->rows > + && first_const->register_count == const_set->register_count > + && (i == const_tab->const_set_count - 1 > + || first_const->param->element_count == const_set->param->element_count))))) > + { > + TRACE("direct_copy %u, i %u, index %u, param %s.%s, current_data %p, const_set->param->data %p, "\ > + "current_start_offset %u, start_offset %u, const_set->table %u, current_table %u.\n", > + const_set->direct_copy, i, index, debugstr_a(param->name), > + debugstr_a(const_set->param->name), current_data, const_set->param->data, > + current_start_offset, start_offset, const_set->table, current_table); > + break; > + } This looks like a debug trace. > + TRACE("Merging %u child parameters for %s, not merging %u, direct_copy %u.\n", i - index, > + debugstr_a(param->name), const_tab->const_set_count - i, first_const->direct_copy); I guess it's intentional that you're printing this even when not merging anything, in that case the TRACE sounds a bit awkward though. Maybe have a separate TRACE for that case? BTW, please format the BOOL with %#x. > + if (i > index + 1) > + { > + first_const->element_count = element_count; > + if (first_const->direct_copy) > + { > + first_const->element_count = 1; > + if (index == start_index > + && !(param->type == D3DXPT_VOID && param->class == D3DXPC_STRUCT)) > + { > + if (param_type_to_table_type(param->type) == PRES_VT_MAX) > + return D3DERR_INVALIDCALL; > + first_const->param = param; > + } > + first_const->register_count = get_reg_offset(current_table, current_start_offset) > + - first_const->register_index; > + } > + memmove(&const_tab->const_set[index + 1], &const_tab->const_set[i], > + sizeof(*const_tab->const_set) * (const_tab->const_set_count - i)); > + const_tab->const_set_count -= i - index - 1; > + } Just for my own curiosity, do you think it's significant to merge non-direct_copy entries? Not that it hurts anything to have it.

2 4

Re: [v2 2/6] d3dx9: Factor out get_const_upload_info() function.
by Matteo Bruni May 31, 2017

May 31, 2017

2017-05-30 11:38 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > v2: > - renamed 'n' to 'major_count'; major_count is still a bit meh, since "major" is really already a count and there is an unspecified "count" too. I don't have any good suggestion and this is an improvement over the current naming though, so...

1 0

Re: [PATCH 1/6] d3dx9: Get rid of table lookup for converting between register indexes and offsets.
by Matteo Bruni May 31, 2017

May 31, 2017

2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > static unsigned int get_reg_offset(unsigned int table, unsigned int offset) > { > - return offset / table_info[table].reg_component_count; > + return table == PRES_REGTAB_OBCONST ? offset : offset >> 2; > +} > + > +static unsigned int get_offset_reg(unsigned int table, unsigned int reg_idx) > +{ > + return table == PRES_REGTAB_OBCONST ? reg_idx : reg_idx << 2; > } I suppose using an explicit division and multiplication here wouldn't change the generated code, in which case I'd slightly prefer that. It doesn't matter right now, just mentioning it for potentially similar cases in the future. Also this patch probably should have been split in two, with the first patch introducing get_offset_reg() with no functional changes and the second hardcoding the table check / dropping the table lookup. Not worth a rework at this point I guess. > @@ -1033,9 +1035,9 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const > unsigned int offset; > > offset = start_offset + i * major_stride + j; > - if (offset / table_info[table].reg_component_count >= rs->table_sizes[table]) > + if (get_reg_offset(table, offset) >= rs->table_sizes[table]) > { > - if (table_info[table].reg_component_count != 1) > + if (table != PRES_REGTAB_OBCONST) > FIXME("Output offset exceeds table size, name %s, component %u.\n", > debugstr_a(param->name), i); > break; Not new and probably I just forgot the details, but, why would the register component count matter for the FIXME? > - offset = reg_index * table_info[table].reg_component_count > - + offset % table_info[table].reg_component_count; > + offset = get_offset_reg(table, reg_index) + offset % get_offset_reg(table, 1); That "get_offset_reg(table, 1)" comes up a number of times. Maybe worth making an extra get_reg_components(table) (or similar) helper? Simply for even greater clarity.

2 5

Re: [PATCH 3/6] d3d9: Validate adapter format in CheckDeviceFormat().
by Marvin May 31, 2017

May 31, 2017

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=31280 Your paranoid android. === wvistau64 (32 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w2008s64 (32 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w7u (32 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w7pro64 (32 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w8 (32 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w864 (32 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w1064 (32 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === wvistau64 (64 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w2008s64 (64 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w7pro64 (64 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w864 (64 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4. === w1064 (64 bit device) === device.c:11206: Test failed: Got unexpected hr 0, device type 0x4. device.c:11209: Test failed: Got unexpected hr 0, device type 0x4. device.c:11212: Test failed: Got unexpected hr 0, device type 0x4. device.c:11215: Test failed: Got unexpected hr 0, device type 0x4.

2 1

Re: [PATCH 2/6] d3d8: Validate adapter format in CheckDeviceFormat().
by Marvin May 31, 2017

May 31, 2017

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=31279 Your paranoid android. === w8 (32 bit device) === device.c:2945: Test failed: Received WM_WINDOWPOSCHANGED but did not expect it.

1 0

RE: [PATCH 2/7] msvcrt: Clean up registered C++ object in handler.
by Daniel Lehman May 31, 2017

May 31, 2017

On 05/24/17 02:55, Daniel Lehman wrote: > +static DWORD cxx_catch_cleanup(EXCEPTION_RECORD *rec, EXCEPTION_REGISTRATION_RECORD *frame, > + CONTEXT *context, > +EXCEPTION_REGISTRATION_RECORD **pdispatcher) { > + if (rec->ExceptionFlags & (EH_UNWINDING | EH_EXIT_UNWIND)) > + { > + thread_data_t *data = msvcrt_get_thread_data(); > + frame_info *cur; > + > + if (cxx_is_consolidate(rec)) > Is this condition really needed? Shouldn't we clean the object no matter what's the reason of unwind? Yeah. That's covered by patch 7/7. The original code only cleaned up if consolidating. Since this 2/7 patch was somewhat of a refactoring for the later patches, I kept the consolidate-only cleanup logic. I can merge them if you want >> + if ((ULONG64)cur <= (ULONG64)frame) > This condition is not working. It's making assumption about order of catch_frame and frame_info variables on stack while they are declared this way: I see what you mean. If I forcefully reverse them, my tests crash; my version of gcc was always putting them in the same place on the stack, regardless of where they were declared >> + EXCEPTION_REGISTRATION_RECORD catch_frame; >> cxx_frame_info frame_info; > Shouldn't the cxx_catch_cleanup just unregister the object that was registered in call_catch_block? I didn't find a way to be call __CxxUnregisterObject on that one specifically (I'll try suggestions if you got em) but the loop in cxx_catch_cleanup effective does this. when unwinding a nested exception, cxx_catch_cleanup will be called when unwinding call_catch_block, so all objects registered in call_catch_block and below will be cleaned up at that point. Unless the catch block itself explicitly calls __CxxRegisterObject, the only object on the list up to that frame will be the one from call_catch_block > Here's a test case that demonstrate the problem with cur <= frame > comparison: > > try { > try { int *p = NULL; *p = 0x42; } > catch (klass x) { throw 1; } > } catch (int i) { } > > try { throw 1; } > catch(...) {} This crashes for me even on Windows because the SEGV is uncaught. It 'works' if I set an seh translator that throws int, but I get identical results on Wine with my series applied. Do I need to add something? Thanks daniel

2 1

Re: [PATCH 7/7] regedit: Do not get text for items with an index less than one
by Alexandre Julliard May 31, 2017

May 31, 2017

Hugh McMaster <hugh.mcmaster(a)outlook.com> writes: > @@ -59,7 +59,7 @@ LPWSTR GetItemText(HWND hwndLV, UINT item) > { > LPWSTR newStr, curStr; > unsigned int maxLen = 128; > - if (item == 0) return NULL; /* first item is ALWAYS a default */ > + if (item < 1) return NULL; /* first item is ALWAYS a default */ How is this different? -- Alexandre Julliard julliard(a)winehq.org

3 3