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

Re: [PATCH] quartz/tests: Skip test_render_run() if test file doesn't exist.

by Andrew Eikum

On Wed, Jan 04, 2017 at 10:52:28AM +0800, Jactry Zeng wrote: > + h = CreateFileW(file, 0, 0, NULL, OPEN_EXISTING, 0, NULL); > + if (h == INVALID_HANDLE_VALUE) { > + skip("Could not read test file \"%s\", skipping test\n", wine_dbgstr_w(file)); > + return; > + } > + CloseHandle(h); > + I like this, but please remove the quote characters: filtergraph.c:359: Tests skipped: Could not read test file "L"test.avi"", skipping test Andrew

Jan. 4, 2017

Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils

by Ricardo Neri

On Tue, 2017-01-03 at 08:44 -0800, Dave Hansen wrote: > On 12/23/2016 05:37 PM, Ricardo Neri wrote: > > Other kernel submodules can benefit from using the utility functions > > defined in mpx.c to obtain the addresses and values of operands contained > > in the general purpose registers. An instance of this is the emulation code > > used for instructions protected by the Intel User-Mode Instruction > > Prevention feature. > > I haven't looked at this in detail, but as long as this is pretty much a > straight code move, I don't see any issues with it from an MPX > perspective. I'm glad to see it getting reused. Yes, this is only a relocation of code. > > Feel free to add my Acked-by on it if you like. Great! Thanks! Ricardo

Jan. 4, 2017

Re: [v2 2/7] x86/mpx: Fail when implicit zero-displacement is used along with R/EBP

by Ricardo Neri

On Tue, 2017-01-03 at 08:41 -0800, Dave Hansen wrote: > On 12/27/2016 02:33 PM, Ricardo Neri wrote: > >>> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c > >>> index 6a75a75..71681d0 100644 > >>> --- a/arch/x86/mm/mpx.c > >>> +++ b/arch/x86/mm/mpx.c > >>> @@ -120,6 +120,13 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, > >>> > >>> case REG_TYPE_BASE: > >>> regno = X86_SIB_BASE(insn->sib.value); > >>> + if (regno == 5 && X86_MODRM_RM(insn->modrm.value) == 0) { > >>> + WARN_ONCE(1, "An explicit displacement is required when %sBP used as SIB base.", > >>> + (IS_ENABLED(CONFIG_X86_64) && insn->x86_64) ? > >>> + "R13 or R" : "E"); > >>> + return -EINVAL; > >>> + } > >>> + > >> Now that I've read the cover letter, I see what's going on. This > >> should not warn -- user code can easily trigger this deliberately. > > OK, I'll remove it. Are you concerned about the warning printing the > > calltrace, even only once? > > Yes. We don't let userspace spam the kernel, even once. If we have a > couple thousand "only once" places, then userspace can overwhelm the > kernel log. This makes sense. I was not looking at it this way. > > Also, this needs a much better description of what's going on in the > code. Could you add a comment explaining what's going on, and why > regno==5, etc...? I will add more comments. Thanks! Ricardo

Jan. 4, 2017

Re: [v2 5/7] x86: Add emulation code for UMIP instructions

by Ricardo Neri

On Fri, 2016-12-30 at 18:07 -0800, Andy Lutomirski wrote: > On Thu, Dec 29, 2016 at 9:23 PM, Ricardo Neri > <ricardo.neri-calderon(a)linux.intel.com> wrote: > > On Tue, 2016-12-27 at 16:48 -0800, Andy Lutomirski wrote: > >> > >> >> > + if (nr_copied > 0) > >> >> > + return -EFAULT; > >> >> > >> >> This should be the only EFAULT case. > >> > Should this be EFAULT event if the caller cares only about successful > >> > (return 0) vs failed (return non-0) emulation? > >> > >> In theory this particular error would be a page fault not a general > >> protection fault (in the UMIP off case). If you were emulating it > >> extra carefully, you could change the signal accordingly. But, as I > >> said, I really doubt this matters. > > > > If simple enough and for the sake of accuracy, I could try to issue the > > page fault. It seems to me that this entitles calling > > force_sig_info_fault in this particular case as opposed to the > > force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk) that do_general_protection > > calls. > > Sure. You could even do it by sending the signal in the emulation > code and returning true. Will do. Thanks! Ricardo > > --Andy

Jan. 4, 2017

Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils

by Dave Hansen

On 12/23/2016 05:37 PM, Ricardo Neri wrote: > Other kernel submodules can benefit from using the utility functions > defined in mpx.c to obtain the addresses and values of operands contained > in the general purpose registers. An instance of this is the emulation code > used for instructions protected by the Intel User-Mode Instruction > Prevention feature. I haven't looked at this in detail, but as long as this is pretty much a straight code move, I don't see any issues with it from an MPX perspective. I'm glad to see it getting reused. Feel free to add my Acked-by on it if you like.

Jan. 3, 2017

Re: [v2 2/7] x86/mpx: Fail when implicit zero-displacement is used along with R/EBP

by Dave Hansen

On 12/27/2016 02:33 PM, Ricardo Neri wrote: >>> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c >>> index 6a75a75..71681d0 100644 >>> --- a/arch/x86/mm/mpx.c >>> +++ b/arch/x86/mm/mpx.c >>> @@ -120,6 +120,13 @@ static int get_reg_offset(struct insn *insn, struct pt_regs *regs, >>> >>> case REG_TYPE_BASE: >>> regno = X86_SIB_BASE(insn->sib.value); >>> + if (regno == 5 && X86_MODRM_RM(insn->modrm.value) == 0) { >>> + WARN_ONCE(1, "An explicit displacement is required when %sBP used as SIB base.", >>> + (IS_ENABLED(CONFIG_X86_64) && insn->x86_64) ? >>> + "R13 or R" : "E"); >>> + return -EINVAL; >>> + } >>> + >> Now that I've read the cover letter, I see what's going on. This >> should not warn -- user code can easily trigger this deliberately. > OK, I'll remove it. Are you concerned about the warning printing the > calltrace, even only once? Yes. We don't let userspace spam the kernel, even once. If we have a couple thousand "only once" places, then userspace can overwhelm the kernel log. Also, this needs a much better description of what's going on in the code. Could you add a comment explaining what's going on, and why regno==5, etc...?

Jan. 3, 2017

Re: [1/3] ntdll: Define more file information classes.

by Marcus Meissner

Hi, We are currently in feature freeze for 2.0, only bugfixes are accepted. Your patchset probably fits development after the 2.0 release in Mid January. Ciao, Marcus On Tue, Jan 03, 2017 at 06:33:27AM +0200, Jonathan Doron wrote: > Hello I would like to understand how to proceed with this. > I tried to keep the patchset to minimum amount of changes so it wont affect > anything else. > > Thanks, > -- Jon. > > On Mon, Jan 2, 2017 at 10:04 PM, Marvin <testbot(a)winehq.org> wrote: > > > Thank you for your contribution to Wine! > > > > This is an automated notification to let you know that your patch has > > been reviewed and its status set to "Deferred". > > > > This means that the patch is deferred because of code freeze for an > > upcoming release. You need to either resend after the release, make > > the patch less intrusive, or make a convincing argument that it needs > > to be included in the release. > > > > If you do not understand the reason for this status, disagree with our > > assessment, or are simply not sure how to proceed next, please ask for > > clarification by replying to this email. > > >

Jan. 3, 2017

Re: [1/3] ntdll: Define more file information classes.

by Jonathan Doron

Hello I would like to understand how to proceed with this. I tried to keep the patchset to minimum amount of changes so it wont affect anything else. Thanks, -- Jon. On Mon, Jan 2, 2017 at 10:04 PM, Marvin <testbot(a)winehq.org> wrote: > Thank you for your contribution to Wine! > > This is an automated notification to let you know that your patch has > been reviewed and its status set to "Deferred". > > This means that the patch is deferred because of code freeze for an > upcoming release. You need to either resend after the release, make > the patch less intrusive, or make a convincing argument that it needs > to be included in the release. > > If you do not understand the reason for this status, disagree with our > assessment, or are simply not sure how to proceed next, please ask for > clarification by replying to this email. >

Jan. 3, 2017

Re: [PATCH] riched20: Check for NULL in fnTextSrv_TxSetText and add test

by Fabian Maurer

On Monday, January 2, 2017 11:24:31 PM CET Nikolay Sivov wrote: > It's important to test if NULL text actually discards existing content > or just does nothing. Yeah, you're probably right. Thanks, I'll add checks for that. > Maybe it's better to add a check to ME_InsertTextFromCursor? I'm not sure if only this function needs it, or if others do, too. But I could move the check.

Jan. 2, 2017

Re: [PATCH] riched20: Check for NULL in fnTextSrv_TxSetText and add test

by Nikolay Sivov

On 02.01.2017 23:19, Fabian Maurer wrote: > Fixes https://bugs.winehq.org/show_bug.cgi?id=12185 > > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > dlls/riched20/tests/txtsrv.c | 3 +++ > dlls/riched20/txtsrv.c | 7 +++---- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/dlls/riched20/tests/txtsrv.c b/dlls/riched20/tests/txtsrv.c > index 1a6ca61052..6fd77d1b59 100644 > --- a/dlls/riched20/tests/txtsrv.c > +++ b/dlls/riched20/tests/txtsrv.c > @@ -681,6 +681,9 @@ static void test_TxSetText(void) > ok(memcmp(rettext,settext,SysStringByteLen(rettext)) == 0, > "String returned differs\n"); > > + /* Test if we crash when using NULL as text */ > + ITextServices_TxSetText(txtserv, NULL); > + > SysFreeString(rettext); > ITextServices_Release(txtserv); > ITextHost_Release(host); It's important to test if NULL text actually discards existing content or just does nothing. > diff --git a/dlls/riched20/txtsrv.c b/dlls/riched20/txtsrv.c > index 3df779834b..307b484352 100644 > --- a/dlls/riched20/txtsrv.c > +++ b/dlls/riched20/txtsrv.c > @@ -286,10 +286,9 @@ DECLSPEC_HIDDEN HRESULT WINAPI fnTextSrv_TxSetText(ITextServices *iface, LPCWSTR > ME_Cursor cursor; > > ME_SetCursorToStart(This->editor, &cursor); > - ME_InternalDeleteText(This->editor, &cursor, > - ME_GetTextLength(This->editor), FALSE); > - ME_InsertTextFromCursor(This->editor, 0, pszText, -1, > - This->editor->pBuffer->pDefaultStyle); > + ME_InternalDeleteText(This->editor, &cursor, ME_GetTextLength(This->editor), FALSE); > + if(pszText) > + ME_InsertTextFromCursor(This->editor, 0, pszText, -1, This->editor->pBuffer->pDefaultStyle); > ME_SetSelection(This->editor, 0, 0); > This->editor->nModifyStep = 0; > OleFlushClipboard(); Maybe it's better to add a check to ME_InsertTextFromCursor?

Jan. 2, 2017