December 2016 - Wine-Devel - list.winehq.org

Emitting aligned SSE movs in gcc with force_align_arg_pointer

by Daniel Santos

I'm working on a few improvements in GCC targeting 64-bit Wine. I've got a patch set that gives us back aligned SSE movs for 64-bit ms_abi functions that call System V functions. However, it looks like I'll have to wait until GCC 8's stage 1 comes around (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01924.html) but I thought I should post this here to prevent any duplication of effort. (Come to think of it, I should probably open a gcc bug report for this.) Following this patch set I have the out-of-lining patch set for the msabi/sysv prologues & epilogues as well that I'm still refining and cleaning up a bit. Happy New Year all! Daniel

Dec. 31, 2016

Re: [2/3] ntdll: Implement FileIdInformation class support in NtQueryInformationFile.

by Jon Doron

I was referring to ntifs.h because this has to do with ntdll, do you still want me to move it to winbase? this will probably cause compilation errors in the NTDLL part that implements the functionality On Sat, Dec 31, 2016 at 4:04 PM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > On 31.12.2016 16:50, Jonathan Doron wrote: >> Signed-off-by: Jonathan Doron <jond(a)wizery.com> >> --- >> dlls/ntdll/file.c | 19 ++++++++++++++++++- >> include/winternl.h | 9 +++++++++ >> 2 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c >> index 6eddd19..fbadfbd 100644 >> --- a/dlls/ntdll/file.c >> +++ b/dlls/ntdll/file.c >> @@ -2379,7 +2379,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io, >> 0, /* FileRenameInformationBypassAccessCheck */ >> 0, /* FileLinkInformationBypassAccessCheck */ >> 0, /* FileVolumeNameInformation */ >> - 0, /* FileIdInformation */ >> + sizeof(FILE_ID_INFORMATION), /* FileIdInformation */ >> 0, /* FileIdExtdDirectoryInformation */ >> 0, /* FileReplaceCompletionInformation */ >> 0, /* FileHardLinkFullIdInformation */ >> @@ -2621,6 +2621,23 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io, >> } >> } >> break; >> + case FileIdInformation: >> + { >> + struct statfs volstats; >> + FILE_ID_INFORMATION *info = ptr; >> + if ((fstat(fd, &st) == -1) || (fstatfs(fd, &volstats) == -1)) >> + { >> + io->u.Status = FILE_GetNtStatus(); >> + } >> + else >> + { >> + memset(info, 0, sizeof(*info)); >> + ((PLARGE_INTEGER)&info->VolumeSerialNumber)->LowPart = (ULONG)volstats.f_fsid.__val[0]; >> + ((PLARGE_INTEGER)&info->VolumeSerialNumber)->HighPart = (ULONG)volstats.f_fsid.__val[1]; >> + ((PLARGE_INTEGER)&info->FileId)->QuadPart = st.st_ino; >> + } >> + } >> + break; >> default: >> FIXME("Unsupported class (%d)\n", class); >> io->u.Status = STATUS_NOT_IMPLEMENTED; >> diff --git a/include/winternl.h b/include/winternl.h >> index 891b6a7..2f2ea86 100644 >> --- a/include/winternl.h >> +++ b/include/winternl.h >> @@ -571,6 +571,15 @@ typedef struct _FILE_INTERNAL_INFORMATION { >> LARGE_INTEGER IndexNumber; >> } FILE_INTERNAL_INFORMATION, *PFILE_INTERNAL_INFORMATION; >> >> +typedef struct _FILE_ID_128 { >> + UCHAR Identifier[16]; >> +} FILE_ID_128, *PFILE_ID_128; > Windows SDK has it in winnt.h, using BYTE instead of UCHAR. > >> + >> +typedef struct _FILE_ID_INFORMATION { >> + ULONGLONG VolumeSerialNumber; >> + FILE_ID_128 FileId; >> +} FILE_ID_INFORMATION, *PFILE_ID_INFORMATION; >> + > This one should go to winbase.h, as FILE_ID_INFO. >> typedef struct _FILE_EA_INFORMATION { >> ULONG EaSize; >> } FILE_EA_INFORMATION, *PFILE_EA_INFORMATION; > >

Dec. 31, 2016

Re: [2/3] ntdll: Implement FileIdInformation class support in NtQueryInformationFile.

by Nikolay Sivov

On 31.12.2016 16:50, Jonathan Doron wrote: > Signed-off-by: Jonathan Doron <jond(a)wizery.com> > --- > dlls/ntdll/file.c | 19 ++++++++++++++++++- > include/winternl.h | 9 +++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c > index 6eddd19..fbadfbd 100644 > --- a/dlls/ntdll/file.c > +++ b/dlls/ntdll/file.c > @@ -2379,7 +2379,7 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io, > 0, /* FileRenameInformationBypassAccessCheck */ > 0, /* FileLinkInformationBypassAccessCheck */ > 0, /* FileVolumeNameInformation */ > - 0, /* FileIdInformation */ > + sizeof(FILE_ID_INFORMATION), /* FileIdInformation */ > 0, /* FileIdExtdDirectoryInformation */ > 0, /* FileReplaceCompletionInformation */ > 0, /* FileHardLinkFullIdInformation */ > @@ -2621,6 +2621,23 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE hFile, PIO_STATUS_BLOCK io, > } > } > break; > + case FileIdInformation: > + { > + struct statfs volstats; > + FILE_ID_INFORMATION *info = ptr; > + if ((fstat(fd, &st) == -1) || (fstatfs(fd, &volstats) == -1)) > + { > + io->u.Status = FILE_GetNtStatus(); > + } > + else > + { > + memset(info, 0, sizeof(*info)); > + ((PLARGE_INTEGER)&info->VolumeSerialNumber)->LowPart = (ULONG)volstats.f_fsid.__val[0]; > + ((PLARGE_INTEGER)&info->VolumeSerialNumber)->HighPart = (ULONG)volstats.f_fsid.__val[1]; > + ((PLARGE_INTEGER)&info->FileId)->QuadPart = st.st_ino; > + } > + } > + break; > default: > FIXME("Unsupported class (%d)\n", class); > io->u.Status = STATUS_NOT_IMPLEMENTED; > diff --git a/include/winternl.h b/include/winternl.h > index 891b6a7..2f2ea86 100644 > --- a/include/winternl.h > +++ b/include/winternl.h > @@ -571,6 +571,15 @@ typedef struct _FILE_INTERNAL_INFORMATION { > LARGE_INTEGER IndexNumber; > } FILE_INTERNAL_INFORMATION, *PFILE_INTERNAL_INFORMATION; > > +typedef struct _FILE_ID_128 { > + UCHAR Identifier[16]; > +} FILE_ID_128, *PFILE_ID_128; Windows SDK has it in winnt.h, using BYTE instead of UCHAR. > + > +typedef struct _FILE_ID_INFORMATION { > + ULONGLONG VolumeSerialNumber; > + FILE_ID_128 FileId; > +} FILE_ID_INFORMATION, *PFILE_ID_INFORMATION; > + This one should go to winbase.h, as FILE_ID_INFO. > typedef struct _FILE_EA_INFORMATION { > ULONG EaSize; > } FILE_EA_INFORMATION, *PFILE_EA_INFORMATION;

Dec. 31, 2016

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

by Andy Lutomirski

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. --Andy

Dec. 31, 2016

Release plans

by Alexandre Julliard

Folks, Since there was (understandably) very little activity this week, I'm going to skip today's release. I'll release rc4 next Friday; after that, there will probably be a couple more release candidates, since I'm hoping that we can make some more progress on bug fixing once people are back from the holidays. That will put the final 2.0 probably in the second half of January. Happy New Year to everyone! -- Alexandre Julliard julliard(a)winehq.org

Dec. 30, 2016

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

by Ricardo Neri

On Tue, 2016-12-27 at 16:48 -0800, Andy Lutomirski wrote: > On Tue, Dec 27, 2016 at 4:39 PM, Ricardo Neri > <ricardo.neri-calderon(a)linux.intel.com> wrote: > > On Fri, 2016-12-23 at 18:11 -0800, Andy Lutomirski wrote: > >> On Fri, Dec 23, 2016 at 5:37 PM, Ricardo Neri > >> <ricardo.neri-calderon(a)linux.intel.com> wrote: > >> > The feature User-Mode Instruction Prevention present in recent Intel > >> > processor prevents a group of instructions from being executed with > >> > CPL > 0. Otherwise, a general protection fault is issued. > >> > > >> > Rather than relaying this fault to the user space (in the form of a SIGSEGV > >> > signal), the instructions protected by UMIP can be emulated to provide > >> > dummy results. This allows to conserve the current kernel behavior and not > >> > reveal the system resources that UMIP intends to protect (the global > >> > descriptor and interrupt descriptor tables, the segment selectors of the > >> > local descriptor table and the task state and the machine status word). > >> > > >> > This emulation is needed because certain applications (e.g., WineHQ) rely > >> > on this subset of instructions to function. > >> > > >> > The instructions protected by UMIP can be split in two groups. Those who > >> > return a kernel memory address (sgdt and sidt) and those who return a > >> > value (sldt, str and smsw). > >> > > >> > For the instructions that return a kernel memory address, the result is > >> > emulated as the location of a dummy variable in the kernel memory space. > >> > This is needed as applications such as WineHQ rely on the result being > >> > located in the kernel memory space function. The limit for the GDT and the > >> > IDT are set to zero. > >> > >> Nak. This is a trivial KASLR bypass. Just give them hardcoded > >> values. For x86_64, I would suggest 0xfffffffffffe0000 and > >> 0xffffffffffff0000. > > > > I see. I assume you are suggesting these values for x86_64 because they > > lie in an unused hole. That makes sense to me. > > > > For the case of x86_32, I have trouble finding a suitable place as there > > are not many available holes. It could be put before VMALLOC_START or > > after VMALLOC_END but this would reveal the position of the vmalloc > > area. Although, to my knowledge, randomized memory is not available for > > x86_32. Without randomization, does it hurt to make sidt/sgdt return the > > address of a kernel static variable? > > I would just use the same addresses, truncated. There's no reason > that the address needs to be truly not present -- it just needs to be > inaccessible to user code. Anything near the top of the address space > should work. Right. Then I will reuse the same addresses. > > > > >> > >> > > >> > The instructions sldt and str return a segment selector relative to the > >> > base address of the global descriptor table. Since the actual address of > >> > such table is not revealed, it makes sense to emulate the result as zero. > >> > >> Hmm, now I wonder if anything uses SLDT to see if there is an LDT. If > >> so, we could emulate it better, but I doubt this matters. > > > > So you are saying that the emulated sldt should return a different value > > based on the presence/absence of a LDT? This could reveal this very > > fact. > > User code knows whether the LDT exists because an LDT only exists if > the program called modify_ldt(). But I doubt this matters in > practice. In such a case sldt would return a non-null segment selector. I will keep giving the null segment selector in all cases and make a note in the code. > > >> > +static int __emulate_umip_insn(struct insn *insn, enum umip_insn umip_inst, > >> > + unsigned char *data, int *data_size) > >> > +{ > >> > + unsigned long const *dummy_base_addr; > >> > + unsigned short dummy_limit = 0; > >> > + unsigned short dummy_value = 0; > >> > + > >> > + switch (umip_inst) { > >> > + /* > >> > + * These two instructions return the base address and limit of the > >> > + * global and interrupt descriptor table. The base address can be > >> > + * 32-bit or 64-bit. Limit is always 16-bit. > >> > + */ > >> > + case UMIP_SGDT: > >> > + case UMIP_SIDT: > >> > + if (umip_inst == UMIP_SGDT) > >> > + dummy_base_addr = &umip_dummy_gdt_base; > >> > + else > >> > + dummy_base_addr = &umip_dummy_idt_base; > >> > + if (X86_MODRM_MOD(insn->modrm.value) == 3) { > >> > + WARN_ONCE(1, "SGDT cannot take register as argument!\n"); > >> > >> No warnings please. > > > > I'll. Remove it. > > Thanks. In general, WARN_ONCE, etc are supposed to indicate kernel > bugs, not user bugs. Agreed. Your statement makes it very clear. I didn't have it that clear in my mind. > > >> > + int not_copied, nr_copied, reg_offset, dummy_data_size; > >> > + void __user *uaddr; > >> > + unsigned long *reg_addr; > >> > + enum umip_insn umip_inst; > >> > + > >> > + not_copied = copy_from_user(buf, (void __user *)regs->ip, sizeof(buf)); > >> > >> This is slightly wrong due to PKRU. I doubt we care. > > > > I see. If I am not mistaken, if the memory is protected by a protection > > key this would cause a page fault. I'll make a note of it. > > Exactly. This is correct behavior unless the key happens to be set up > so it can be executed but not read, in which case emulation will fail. If we can't read we can't emulate anyways. I will add another note. > > >> > >> > + nr_copied = sizeof(buf) - not_copied; > >> > + /* > >> > + * The decoder _should_ fail nicely if we pass it a short buffer. > >> > + * But, let's not depend on that implementation detail. If we > >> > + * did not get anything, just error out now. > >> > + */ > >> > + if (!nr_copied) > >> > + return -EFAULT; > >> > >> If the caller cares about EINVAL vs EFAULT, it cares because it is > >> considering changing the signal to a fake page fault. If so, then > >> this should be EINVAL -- failure to read the text should just prevent > >> emulation. > > > > I see. The caller in this case do_general_protection, which will issue a > > SIGSEGV to the user space anyways. I don't think it cares about the > > EINVAL vs EFAULT. It does care about whether the emulation was > > successful. > > Maybe just make it return bool then? But fixing up the return codes > would be fine, too. I just think that, if it returns int, the value > should be meaningful. Right. I have a small query/proposal related to this below. > > >> > + 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. Thanks and BR, Ricardo

Dec. 30, 2016

Re: [PATCH] user32: add stub for GetAutoRotationState

by Fabian Maurer

On Friday, December 30, 2016 2:17:34 AM CET Alistair Leslie-Hughes wrote: > Since PAR_STATE is a pointer, %p would be more appropriate. Thanks, fixed it.

Dec. 30, 2016

Re: [PATCH] user32: add stub for GetAutoRotationState

by Alistair Leslie-Hughes

Hi Fabian, On 30/12/16 12:31, Fabian Maurer wrote: > Fixes https://bugs.winehq.org/show_bug.cgi?id=42106 > > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > dlls/user32/misc.c | 14 ++++++++++++++ > dlls/user32/user32.spec | 1 + > include/winuser.h | 13 +++++++++++++ > 3 files changed, 28 insertions(+) > > diff --git a/dlls/user32/misc.c b/dlls/user32/misc.c > index fa0502e25e..15f5c877fa 100644 > --- a/dlls/user32/misc.c > +++ b/dlls/user32/misc.c > @@ -836,6 +836,20 @@ LONG WINAPI GetDisplayConfigBufferSizes(UINT32 flags, UINT32 *num_path_info, UIN > return ERROR_NOT_SUPPORTED; > } > > +/********************************************************************** > + * GetAutoRotationState [USER32.@] > + */ > +BOOL WINAPI GetAutoRotationState(PAR_STATE pState) > +{ > + FIXME("(0x%x): stub\n", pState); Since PAR_STATE is a pointer, %p would be more appropriate. Best Regards Alistair

Dec. 30, 2016

Re: [PATCH 2/2] regedit: Only allow value renaming via the Edit or Popup menus

by Nikolay Sivov

On 29.12.2016 13:23, Hugh McMaster wrote: > On Thursday, 29 December 2016 1:10 AM, Ken Thomases wrote: >> I personally dislike this proposed change. I use click-to-rename values all the time and can't think of why that would be a problem. > This behaviour isn't available in the Windows version of regedit, so that's my primary motivation for altering the renaming behaviour. > > Personally, I just don't find the click to rename behaviour that useful. I guess that's because I'm so used to how the Windows version > behaves. In Wine, right-clicking on a value activates the pop-up menu. Left-clicking out of that menu on the same entry activates > renaming mode. I find that behaviour unexpected and annoying. But that's just me. I agree with Ken, I find in-place value renaming useful. However, I see why disabling renaming for keys could be considered as improvement - TreeView is used for navigation most of the time, not for renaming. Accidental renaming definitely happened to me, and that what https://bugs.winehq.org/show_bug.cgi?id=37458 is about. > Thanks for the feedback. > > Hugh >

Dec. 29, 2016

Re: [PATCH 2/2] regedit: Only allow value renaming via the Edit or Popup menus

by Nikolay Sivov

On 29.12.2016 13:13, Hugh McMaster wrote: > I thought of using LVN_BEGINLABELEDIT to handle the edit behaviour, but I'm not > sure how to determine whether the command was sent from the UI via WM_COMMAND > or directly from the ListView click via WM_NOTIFY. When you use main or context menu LVM_EDITLABEL is sent explicitly, and when you edit using Listview capability it happens on its own, so it should be possible to distinguish those cases. > > [1] https://bugs.winehq.org/show_bug.cgi?id=37458

Dec. 29, 2016