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

Re: [PATCH v7 24/26] x86: Enable User-Mode Instruction Prevention

by Ricardo Neri

On Fri, 2017-06-09 at 18:10 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:22AM -0700, Ricardo Neri wrote: > > User_mode Instruction Prevention (UMIP) is enabled by setting/clearing a > > bit in %cr4. > > > > It makes sense to enable UMIP at some point while booting, before user > > spaces come up. Like SMAP and SMEP, is not critical to have it enabled > > very early during boot. This is because UMIP is relevant only when there is > > a userspace to be protected from. Given the similarities in relevance, it > > makes sense to enable UMIP along with SMAP and SMEP. > > > > UMIP is enabled by default. It can be disabled by adding clearcpuid=514 > > to the kernel parameters. > > > > Cc: Andy Lutomirski <luto(a)kernel.org> > > Cc: Andrew Morton <akpm(a)linux-foundation.org> > > Cc: H. Peter Anvin <hpa(a)zytor.com> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Brian Gerst <brgerst(a)gmail.com> > > Cc: Chen Yucong <slaoub(a)gmail.com> > > Cc: Chris Metcalf <cmetcalf(a)mellanox.com> > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Fenghua Yu <fenghua.yu(a)intel.com> > > Cc: Huang Rui <ray.huang(a)amd.com> > > Cc: Jiri Slaby <jslaby(a)suse.cz> > > Cc: Jonathan Corbet <corbet(a)lwn.net> > > Cc: Michael S. Tsirkin <mst(a)redhat.com> > > Cc: Paul Gortmaker <paul.gortmaker(a)windriver.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: Shuah Khan <shuah(a)kernel.org> > > Cc: Vlastimil Babka <vbabka(a)suse.cz> > > Cc: Tony Luck <tony.luck(a)intel.com> > > Cc: Paolo Bonzini <pbonzini(a)redhat.com> > > Cc: Liang Z. Li <liang.z.li(a)intel.com> > > Cc: Alexandre Julliard <julliard(a)winehq.org> > > Cc: Stas Sergeev <stsp(a)list.ru> > > Cc: x86(a)kernel.org > > Cc: linux-msdos(a)vger.kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/Kconfig | 10 ++++++++++ > > arch/x86/kernel/cpu/common.c | 16 +++++++++++++++- > > 2 files changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > index 702002b..1b1bbeb 100644 > > --- a/arch/x86/Kconfig > > +++ b/arch/x86/Kconfig > > @@ -1745,6 +1745,16 @@ config X86_SMAP > > > > If unsure, say Y. > > > > +config X86_INTEL_UMIP > > + def_bool y > > That's a bit too much. It makes sense on distro kernels but how many > machines out there actually have UMIP? So would this become a y when more machines have UMIP? > > > + depends on CPU_SUP_INTEL > > + prompt "Intel User Mode Instruction Prevention" if EXPERT > > + ---help--- > > + The User Mode Instruction Prevention (UMIP) is a security > > + feature in newer Intel processors. If enabled, a general > > + protection fault is issued if the instructions SGDT, SLDT, > > + SIDT, SMSW and STR are executed in user mode. > > + > > config X86_INTEL_MPX > > prompt "Intel MPX (Memory Protection Extensions)" > > def_bool n > > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > > index 8ee3211..66ebded 100644 > > --- a/arch/x86/kernel/cpu/common.c > > +++ b/arch/x86/kernel/cpu/common.c > > @@ -311,6 +311,19 @@ static __always_inline void setup_smap(struct cpuinfo_x86 *c) > > } > > } > > > > +static __always_inline void setup_umip(struct cpuinfo_x86 *c) > > +{ > > + if (cpu_feature_enabled(X86_FEATURE_UMIP) && > > + cpu_has(c, X86_FEATURE_UMIP)) > > Hmm, so if UMIP is not build-time disabled, the cpu_feature_enabled() > will call static_cpu_has(). > > Looks like you want to call cpu_has() too because alternatives haven't > run yet and static_cpu_has() will reply wrong. Please state that in a > comment. Why would static_cpu_has() reply wrong if alternatives are not in place? Because it uses the boot CPU data? When it calls _static_cpu_has() it would do something equivalent to testb test_bit, boot_cpu_data.x86_capability[bit]. I am calling cpu_has because cpu_feature_enabled(), via static_cpu_has(), will use the boot CPU data while cpu_has would use the local CPU data. Is this what you meant? I can definitely add a comment with this explanation, if it makes sense. Thanks and BR, Ricardo

July 26, 2017

Re: [PATCH v7 23/26] x86/traps: Fixup general protection faults caused by UMIP

by Ricardo Neri

I am sorry Boris, I also missed this feedback. On Fri, 2017-06-09 at 15:02 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:21AM -0700, Ricardo Neri wrote: > > If the User-Mode Instruction Prevention CPU feature is available and > > enabled, a general protection fault will be issued if the instructions > > sgdt, sldt, sidt, str or smsw are executed from user-mode context > > (CPL > 0). If the fault was caused by any of the instructions protected > > by UMIP, fixup_umip_exception will emulate dummy results for these > > Please end function names with parentheses. I have audited my commit messages to remove all instances of this error. > > > instructions. If emulation is successful, the result is passed to the > > user space program and no SIGSEGV signal is emitted. > > > > Please note that fixup_umip_exception also caters for the case when > > the fault originated while running in virtual-8086 mode. > > > > Cc: Andy Lutomirski <luto(a)kernel.org> > > Cc: Andrew Morton <akpm(a)linux-foundation.org> > > Cc: H. Peter Anvin <hpa(a)zytor.com> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Brian Gerst <brgerst(a)gmail.com> > > Cc: Chen Yucong <slaoub(a)gmail.com> > > Cc: Chris Metcalf <cmetcalf(a)mellanox.com> > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Fenghua Yu <fenghua.yu(a)intel.com> > > Cc: Huang Rui <ray.huang(a)amd.com> > > Cc: Jiri Slaby <jslaby(a)suse.cz> > > Cc: Jonathan Corbet <corbet(a)lwn.net> > > Cc: Michael S. Tsirkin <mst(a)redhat.com> > > Cc: Paul Gortmaker <paul.gortmaker(a)windriver.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: Shuah Khan <shuah(a)kernel.org> > > Cc: Vlastimil Babka <vbabka(a)suse.cz> > > Cc: Tony Luck <tony.luck(a)intel.com> > > Cc: Paolo Bonzini <pbonzini(a)redhat.com> > > Cc: Liang Z. Li <liang.z.li(a)intel.com> > > Cc: Alexandre Julliard <julliard(a)winehq.org> > > Cc: Stas Sergeev <stsp(a)list.ru> > > Cc: x86(a)kernel.org > > Cc: linux-msdos(a)vger.kernel.org > > Reviewed-by: Andy Lutomirski <luto(a)kernel.org> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/kernel/traps.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > > index 3995d3a..cec548d 100644 > > --- a/arch/x86/kernel/traps.c > > +++ b/arch/x86/kernel/traps.c > > @@ -65,6 +65,7 @@ > > #include <asm/trace/mpx.h> > > #include <asm/mpx.h> > > #include <asm/vm86.h> > > +#include <asm/umip.h> > > > > #ifdef CONFIG_X86_64 > > #include <asm/x86_init.h> > > @@ -526,6 +527,9 @@ do_general_protection(struct pt_regs *regs, long error_code) > > RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); > > cond_local_irq_enable(regs); > > > > Almost definitely: > > if (static_cpu_has(X86_FEATURE_UMIP)) { > if (... I will make this update. > > > + if (user_mode(regs) && fixup_umip_exception(regs)) > > + return; > > We don't want to punish !UMIP machines. I will add this check. Thanks and BR, Ricardo

July 25, 2017

Re: [PATCH v7 22/26] x86/umip: Force a page fault when unable to copy emulated result to user

by Ricardo Neri

On Fri, 2017-06-09 at 13:02 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:20AM -0700, Ricardo Neri wrote: > > fixup_umip_exception() will be called from do_general_protection. If the > ^ > | > Please end function names with parentheses. ---+ > > > former returns false, the latter will issue a SIGSEGV with SEND_SIG_PRIV. > > However, when emulation is successful but the emulated result cannot be > > copied to user space memory, it is more accurate to issue a SIGSEGV with > > SEGV_MAPERR with the offending address. > > A new function is inspired in > > That reads funny. I will correct this. > > > force_sig_info_fault is introduced to model the page fault. > > > > Cc: Andy Lutomirski <luto(a)kernel.org> > > Cc: Andrew Morton <akpm(a)linux-foundation.org> > > Cc: H. Peter Anvin <hpa(a)zytor.com> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Brian Gerst <brgerst(a)gmail.com> > > Cc: Chen Yucong <slaoub(a)gmail.com> > > Cc: Chris Metcalf <cmetcalf(a)mellanox.com> > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Fenghua Yu <fenghua.yu(a)intel.com> > > Cc: Huang Rui <ray.huang(a)amd.com> > > Cc: Jiri Slaby <jslaby(a)suse.cz> > > Cc: Jonathan Corbet <corbet(a)lwn.net> > > Cc: Michael S. Tsirkin <mst(a)redhat.com> > > Cc: Paul Gortmaker <paul.gortmaker(a)windriver.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: Shuah Khan <shuah(a)kernel.org> > > Cc: Vlastimil Babka <vbabka(a)suse.cz> > > Cc: Tony Luck <tony.luck(a)intel.com> > > Cc: Paolo Bonzini <pbonzini(a)redhat.com> > > Cc: Liang Z. Li <liang.z.li(a)intel.com> > > Cc: Alexandre Julliard <julliard(a)winehq.org> > > Cc: Stas Sergeev <stsp(a)list.ru> > > Cc: x86(a)kernel.org > > Cc: linux-msdos(a)vger.kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/kernel/umip.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 43 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c > > index c7c5795..ff7366a 100644 > > --- a/arch/x86/kernel/umip.c > > +++ b/arch/x86/kernel/umip.c > > @@ -148,6 +148,41 @@ static int __emulate_umip_insn(struct insn *insn, enum umip_insn umip_inst, > > } > > > > /** > > + * __force_sig_info_umip_fault() - Force a SIGSEGV with SEGV_MAPERR > > + * @address: Address that caused the signal > > + * @regs: Register set containing the instruction pointer > > + * > > + * Force a SIGSEGV signal with SEGV_MAPERR as the error code. This function is > > + * intended to be used to provide a segmentation fault when the result of the > > + * UMIP emulation could not be copied to the user space memory. > > + * > > + * Return: none > > + */ > > +static void __force_sig_info_umip_fault(void __user *address, > > + struct pt_regs *regs) > > +{ > > + siginfo_t info; > > + struct task_struct *tsk = current; > > + > > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)) { > > Save an indentation level: > > if (!(show_unhandled_signals && unhandled_signal(tsk, SIGSEGV))) > return; > > printk... > I will implement like this. > > > > + printk_ratelimited("%s[%d] umip emulation segfault ip:%lx sp:%lx error:%x in %lx\n", > > + tsk->comm, task_pid_nr(tsk), regs->ip, > > + regs->sp, X86_PF_USER | X86_PF_WRITE, > > + regs->ip); > > + } > > + > > + tsk->thread.cr2 = (unsigned long)address; > > + tsk->thread.error_code = X86_PF_USER | X86_PF_WRITE; > > + tsk->thread.trap_nr = X86_TRAP_PF; > > + > > + info.si_signo = SIGSEGV; > > + info.si_errno = 0; > > + info.si_code = SEGV_MAPERR; > > + info.si_addr = address; > > + force_sig_info(SIGSEGV, &info, tsk); > > +} > > + > > +/** > > * fixup_umip_exception() - Fixup #GP faults caused by UMIP > > * @regs: Registers as saved when entering the #GP trap > > * > > @@ -235,8 +270,14 @@ bool fixup_umip_exception(struct pt_regs *regs) > > if ((unsigned long)uaddr == -1L) > > return false; > > nr_copied = copy_to_user(uaddr, dummy_data, dummy_data_size); > > - if (nr_copied > 0) > > - return false; > > + if (nr_copied > 0) { > > + /* > > + * If copy fails, send a signal and tell caller that > > + * fault was fixed up > > Pls end sentences in the comments with a fullstop. I will correct this. Thanks and BR, Ricardo

July 25, 2017

Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

by Ricardo Neri

I am sorry Boris, while working on this series I missed a few of your feedback comments. On Wed, 2017-06-07 at 17:48 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:14AM -0700, Ricardo Neri wrote: > > The 32-bit and 64-bit address encodings are identical. This means that we > > can use the same function in both cases. In order to reuse the function > > for 32-bit address encodings, we must sign-extend our 32-bit signed > > operands to 64-bit signed variables (only for 64-bit builds). To decide on > > whether sign extension is needed, we rely on the address size as given by > > the instruction structure. > > > > Once the effective address has been computed, a special verification is > > needed for 32-bit processes. If running on a 64-bit kernel, such processes > > can address up to 4GB of memory. Hence, for instance, an effective > > address of 0xffff1234 would be misinterpreted as 0xffffffffffff1234 due to > > the sign extension mentioned above. For this reason, the 4 must be > > Which 4? I meant to say the 4 most significant bytes. In this case, the 64-address 0xffffffffffff1234 would lie in the kernel memory while 0xffff1234 would correctly be in the user space memory. > > > truncated to obtain the true effective address. > > > > Lastly, before computing the linear address, we verify that the effective > > address is within the limits of the segment. The check is kept for long > > mode because in such a case the limit is set to -1L. This is the largest > > unsigned number possible. This is equivalent to a limit-less segment. > > > > Cc: Dave Hansen <dave.hansen(a)linux.intel.com> > > Cc: Adam Buchbinder <adam.buchbinder(a)gmail.com> > > Cc: Colin Ian King <colin.king(a)canonical.com> > > Cc: Lorenzo Stoakes <lstoakes(a)gmail.com> > > Cc: Qiaowei Ren <qiaowei.ren(a)intel.com> > > Cc: Arnaldo Carvalho de Melo <acme(a)redhat.com> > > Cc: Masami Hiramatsu <mhiramat(a)kernel.org> > > Cc: Adrian Hunter <adrian.hunter(a)intel.com> > > Cc: Kees Cook <keescook(a)chromium.org> > > Cc: Thomas Garnier <thgarnie(a)google.com> > > Cc: Peter Zijlstra <peterz(a)infradead.org> > > Cc: Borislav Petkov <bp(a)suse.de> > > Cc: Dmitry Vyukov <dvyukov(a)google.com> > > Cc: Ravi V. Shankar <ravi.v.shankar(a)intel.com> > > Cc: x86(a)kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon(a)linux.intel.com> > > --- > > arch/x86/lib/insn-eval.c | 99 ++++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 88 insertions(+), 11 deletions(-) > > > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 1a5f5a6..c7c1239 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -688,6 +688,62 @@ int insn_get_modrm_rm_off(struct insn *insn, struct pt_regs *regs) > > return get_reg_offset(insn, regs, REG_TYPE_RM); > > } > > > > +/** > > + * _to_signed_long() - Cast an unsigned long into signed long > > + * @val A 32-bit or 64-bit unsigned long > > + * @long_bytes The number of bytes used to represent a long number > > + * @out The casted signed long > > + * > > + * Return: A signed long of either 32 or 64 bits, as per the build configuration > > + * of the kernel. > > + */ > > +static int _to_signed_long(unsigned long val, int long_bytes, long *out) > > +{ > > + if (!out) > > + return -EINVAL; > > + > > +#ifdef CONFIG_X86_64 > > + if (long_bytes == 4) { > > + /* higher bytes should all be zero */ > > + if (val & ~0xffffffff) > > + return -EINVAL; > > + > > + /* sign-extend to a 64-bit long */ > > So this is a 32-bit userspace on a 64-bit kernel, right? Yes. > > If so, how can a memory offset be > 32-bits and we have to extend it to > a 64-bit long?!? Yes, perhaps the check above is not needed. I included that check as part of my argument validation. In a 64-bit kernel, this function could be called with val with non-zero most significant bytes. > > I *think* you want to say that you want to convert it to long so that > you can do the calculation in longs. That is exactly what I meant. More specifically, I want to convert my 32-bit variables into 64-bit signed longs; this is the reason I need the sign extension. > > However! > > If you're a 64-bit kernel running a 32-bit userspace, you need to do > the calculation in 32-bits only so that it overflows, as it would do > on 32-bit hardware. IOW, the clamping to 32-bits at the end is not > something you wanna do but actually let it wrap if it overflows. I have looked into this closely and as far as I can see, the 4 least significant bytes will wrap around when using 64-bit signed numbers as they would when using 32-bit signed numbers. For instance, for two positive numbers we have: 7fff:ffff + 7000:0000 = efff:ffff. The addition above overflows. When sign-extended to 64-bit numbers we would have: 0000:0000:7fff:ffff + 0000:0000:7000:0000 = 0000:0000:efff:ffff. The addition above does not overflow. However, the 4 least significant bytes overflow as we expect. We can clamp the 4 most significant bytes. For a two's complement negative numbers we can have: ffff:ffff + 8000:0000 = 7fff:ffff with a carry flag. The addition above overflows. When sign-extending to 64-bit numbers we would have: ffff:ffff:ffff:ffff + ffff:ffff:8000:0000 = ffff:ffff:7fff:ffff with a carry flag. The addition above does not overflow. However, the 4 least significant bytes overflew and wrapped around as they would when using 32-bit signed numbers. > Or am I missing something? Now, am I missing something? Thanks and BR, Ricardo

July 25, 2017

Re: [PATCH] wmvcore/tests: add tests for WMReader methods

by Jefferson Carpenter

Hello, On Tue, Jul 25, 2017 at 5:48 PM, Alistair Leslie-Hughes < leslie_alistair(a)hotmail.com> wrote: > Hi Jefferson, > > > > +typedef struct { > > + IWMReaderCallback IWMReaderCallback_iface; > > + LONG ref; > > + HRESULT (*onStatusCallback)(WMT_STATUS, HRESULT, > WMT_ATTR_DATATYPE, BYTE*, void*, void*); > > + HRESULT (*onSampleCallback)(DWORD, QWORD, QWORD, DWORD, > INSSBuffer*, void*, void*); > > + void *pvCallbackContext; > > +} TestWMReaderCallback; > > I'm still a little confused as to why you need this structure when the > IWMReaderCallbackVtbl with a few globals would work just as well. > Well, I it could be useful to only implement QueryInterface, AddRef, and Release once, and use dependency injection to supply the other methods to the vtable. I wasn't sure whether I was over-engineering that, however. > > > +static HRESULT onStatus_test_wmreader_play_wma( > > + WMT_STATUS status, > > + HRESULT hr, > > + WMT_ATTR_DATATYPE dwType, > > + BYTE *pValue, > > + void *pvContext, > > + void *pvCallbackContext) > > +{ > > + struct callback_context_test_wmreader_play_wma *pTestContext = > > + (struct callback_context_test_wmreader_play_wma*) > pvCallbackContext; > > + DWORD *dwordValue; > > + > > + CHECK_EXPECT(onStatus); > > + ok(status == onStatus_expected_statuses_test_wmreader_play_wma[ > pTestContext->onStatus_call_count], > > + "onStatus called with unexpected status: %d\n", status); > > + if (status == WMT_OPENED) { > > + ok(pvContext == pTestContext->pvContextOpen, > > + "onStatus was not passed pvContext: expected %p, actual > %p\n", pTestContext->pvContextOpen, pvContext); > > + } > > + else /* status == WMT_STARTED, or later */ { > > + ok(pvContext == pTestContext->pvContextStart, > > + "onStatus was not passed pvContext: expected %p, actual > %p\n", pTestContext->pvContextStart, pvContext); > > + } > > + switch (status) { > > + case WMT_STARTED: > > + // MSDN says dwType is a QWORD, but it is actually a DWORD > > + ok(dwType == WMT_TYPE_DWORD, "onStatus pValue not > WMT_TYPE_QWORD (%d), but %d\n", (int)WMT_TYPE_DWORD, dwType); > WMT_TYPE_QWORD typo? > > > + dwordValue = (DWORD*)pValue; > > + ok(*dwordValue == 0, "onStatus call for WMT_STARTED not passed > correct starting timestamp: %u\n", *dwordValue); > > + SET_EXPECT(onSample); > > + break; > > + default: > > + break; > > + } > The switch and if could be merged > Very well. > > > + /* Get format count. */ > > + formatCount = 0; > > + hr = IWMReader_GetOutputFormatCount(reader, 0, &formatCount); > > + todo_wine ok(hr == S_OK, "Failed to get output format count: > 0x%08x\n", hr); > formatCount should be checked > > Okay - I'll have the test check that formatCount is equal to 37. That's the format count that I get on my windows machine. The number 37 seems oddly specific; I'm not sure how much wiggle room wmvcore implementations have to provide varying sets of formats. However it would be best if wine's were identical to the Windows behavior, obviously. > Best Regards > Alistair Leslie-Hughes >

July 25, 2017

Re: [PATCH] wmvcore/tests: add tests for WMReader methods

by Alistair Leslie-Hughes

Hi Jefferson, > > +typedef struct { > + IWMReaderCallback IWMReaderCallback_iface; > + LONG ref; > + HRESULT (*onStatusCallback)(WMT_STATUS, HRESULT, WMT_ATTR_DATATYPE, BYTE*, void*, void*); > + HRESULT (*onSampleCallback)(DWORD, QWORD, QWORD, DWORD, INSSBuffer*, void*, void*); > + void *pvCallbackContext; > +} TestWMReaderCallback; I'm still a little confused as to why you need this structure when the IWMReaderCallbackVtbl with a few globals would work just as well. > +static HRESULT onStatus_test_wmreader_play_wma( > + WMT_STATUS status, > + HRESULT hr, > + WMT_ATTR_DATATYPE dwType, > + BYTE *pValue, > + void *pvContext, > + void *pvCallbackContext) > +{ > + struct callback_context_test_wmreader_play_wma *pTestContext = > + (struct callback_context_test_wmreader_play_wma*)pvCallbackContext; > + DWORD *dwordValue; > + > + CHECK_EXPECT(onStatus); > + ok(status == onStatus_expected_statuses_test_wmreader_play_wma[pTestContext->onStatus_call_count], > + "onStatus called with unexpected status: %d\n", status); > + if (status == WMT_OPENED) { > + ok(pvContext == pTestContext->pvContextOpen, > + "onStatus was not passed pvContext: expected %p, actual %p\n", pTestContext->pvContextOpen, pvContext); > + } > + else /* status == WMT_STARTED, or later */ { > + ok(pvContext == pTestContext->pvContextStart, > + "onStatus was not passed pvContext: expected %p, actual %p\n", pTestContext->pvContextStart, pvContext); > + } > + switch (status) { > + case WMT_STARTED: > + // MSDN says dwType is a QWORD, but it is actually a DWORD > + ok(dwType == WMT_TYPE_DWORD, "onStatus pValue not WMT_TYPE_QWORD (%d), but %d\n", (int)WMT_TYPE_DWORD, dwType); WMT_TYPE_QWORD typo? > + dwordValue = (DWORD*)pValue; > + ok(*dwordValue == 0, "onStatus call for WMT_STARTED not passed correct starting timestamp: %u\n", *dwordValue); > + SET_EXPECT(onSample); > + break; > + default: > + break; > + } The switch and if could be merged > + /* Get format count. */ > + formatCount = 0; > + hr = IWMReader_GetOutputFormatCount(reader, 0, &formatCount); > + todo_wine ok(hr == S_OK, "Failed to get output format count: 0x%08x\n", hr); formatCount should be checked Best Regards Alistair Leslie-Hughes

July 25, 2017

Re: wined3d: Fix context backup window handling.

by Henri Verbeet

On 25 July 2017 at 22:35, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > - if (dc == context->hdc && !context->hdc_is_private && WindowFromDC(dc) != context->win_handle) > + if (dc == context->hdc && !private && WindowFromDC(dc) != context->win_handle) While that works, I think I'd feel happier about the code if the "dc", "private" and "format" parameters just went away. I.e., the reason they still exist is mostly the backup window path in context_set_gl_context(), and I don't think there's a reason that particular path shouldn't just set the appropriate (hdc, hdc_is_private, hdc_has_format) fields in the context instead.

July 25, 2017

Unicode normalization for Wine

by Artur Świgoń

Dear All, My name is Artur and I'm participating in Google Summer of Code 2017 for Wine. Under Nikolay's supervision, I'm working on implementation of Unicode normalization. I probably should have introduced myself some time ago to share results of my research and my ideas, but I also wanted to wait until I could illustrate my points with some code. I'm not sure whether everyone is familiar with Unicode normalization forms, so here's a short summary of concepts and terms I may use. For detailed information, please see [1]. - Characters that look the same on the screen may be encoded in a few different ways. Accented Latin letters may constitute a familiar example. The letter 'é' can be U+00E9 (LATIN SMALL LETTER E WITH ACUTE), but it also can be represented as the sequence U+0065 U+0301 (LATIN SMALL LETTER E, COMBINING ACUTE ACCENT). Many of 1114112 (if I remember the number correctly) Unicode characters are decomposable, i.e. they can also be represented by a different sequence of characters. - Text in normalization form NFD ('D' stands for 'decomposition') does not contain any decomposable characters, e.g. there is U+0065 U+0301 instead of U+00E9. The combining characters (such as the above-mentioned acute accent) have a well defined order, using the concept of combining classes. [2 §3.11] - Text in normalization form NFKD ('K' stands for 'compatibility') has the properties of NFD, plus characters that are typographical variants of other characters (e.g. superscripts, ligatures, etc.) are mapped to 'plain' characters. - Text that will be normalized into the form NFC and NFKC ('C' stands for 'composition') has to be fully decomposed first (NFD and NFKD, respectively) and starters (characters with a combining class zero) plus following non- starters (combining class greater than zero) can be composed into single characters provided that they exist in Unicode and are not present in [3]. I've developed a draft implementation of NFD & NFKD (mapping characters to decomposed sequences as well as reordering combining characters) and written a test program that uses official Unicode test data. [4] My goal was to provide support for codepoints greater than 0xFFFF, encoded in UTF-16 as surrogate pairs. More on that below. The attachment [gsoc-v2.patch] is a diff of my working branch against master. Most notable changes include: - Necessary changes to tools/make_unicode. - New function wine_decompose_string(). - Draft implementation of two Windows API functions: NormalizeString() and IsNormalizedString() (the latter temporarily returns TRUE). [5][6] Rationale and comments: - tools/make_unicode did not collect all necessary data (most notably, compatibility mappings), so a new data structure, @decomp_table_full has been added. Its unification with @decomp_table is yet to be resolved. - The procedure dump_decompose_table() in that script has been rewritten to accomodate all aspects described above and below. Two helper subroutines have been added. - However, the C code section embedded within the script has grown substantially, so it may be desirable to move it to a separate file. - Adding wine_decompose_string() makes sense since surrogate pairs behave like a string and the Unicode Canonical Ordering Algorithm (for combining characters) is an in-place bubble sort on the string, per Unicode specification. - While the code in libs/port/decompose.c (and embedded in tools/make_unicode) implements normalization, it currently serves the purpose of confirmation that the generated data is correct. It hasn't been yet optimized or cleaned up, because the organization of the data is open to discussion now, so the code may still change substantially. - It has been previously assumed that a decomposition mapping of any character is two characters long. This is certainly not true for NFKD, where a character like U+FDFA decomposes to a string of length 18. Therefore, the tables in libs/port/decompose.c contain pairs of offsets to null-terminated mappings in another table (decomp_data, first in that file), one entry for NFD and one for NFKD, instead of two-character mappings. Moreover, since many NFD and NFKD are identical, they have been placed in the same table, with identical mappings merged and fully expanded (i.e. any decomposable characters in the data have been replaced by fully decomposed sequences), to minimize runtime overhead. - The original idea of multi-stage tables (using the most significant bits of a codepoint to obtain an offset to another table etc.) has been preserved, although the tables are no longer combined into a single table. This is because some sub-tables can use a narrower data type, which saves space, especially when data for all Unicode codepoints is included. - Tables of combining classes have a similar structure. - Mappings for characters above 0xFFFF are encoded as UTF-16 (using surrogate pairs), but a single codepoint (UTF-32 if you like) is used for table indexing. Setting $utflim in make_unicode to 65536 is the simplest way to disable support for such characters, but supporting surrogate pairs should not affect any text-related Wine component in a negative way. - As composition depends directly on decomposition, it is the next thing to work on. The attachment [unorm_test.c] is my test program. It should build fine with mingw-w64 or winegcc without any specific compiler flags. It looks for [4] in its directory or accepts a path to that file as an argument. Option -d makes it run only decomposition tests, option -s limits the range to 0xFFFF. No tests should fail with the -s option and 80 of 187460 should fail without it due to incomplete support for surrogate pairs in the reordering part. For details of failed tests, please run with the -v option. Is there a place for such a program somewhere in the Wine source tree? I'd be more than happy to answer your questions, address your comments, and listen to your opinions regarding Unicode normalization. Regards, Artur Świgoń [1] http://www.unicode.org/reports/tr15/ [2] http://www.unicode.org/versions/Unicode10.0.0/ch03.pdf [3] http://www.unicode.org/Public/10.0.0/ucd/CompositionExclusions.txt [4] http://www.unicode.org/Public/10.0.0/ucd/NormalizationTest.txt [5] https://msdn.microsoft.com/en-us/library/windows/desktop/dd319093(v=vs.85).… [6] https://msdn.microsoft.com/en-us/library/windows/desktop/dd318671(v=vs.85).…

July 25, 2017

Re: [PATCH] gdiplus: Avoid division by zero in SOFTWARE_GdipDrawThinPath

by Vincent Povirk

My test case now works (crashes). I had to enable floating point exceptions with _controlfp_s.

July 25, 2017

Re: [PATCH 4/4 v2] mshtml: Add IElementTraversal::get_firstElementChild implementation

by Marvin

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=32476 Your paranoid android. === w7u (32 bit htmldoc) === htmldoc.c:2682: Test failed: unexpected call GetOverrideKeyPath htmldoc.c:5092: Test failed: unexpected call {d9befc84-f21e-4166-874b-a8d3010dc764}

July 25, 2017