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

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v4).

by Bruno Jesus

Hi, thanks for the retry. I'm not at home and I'm replying from mobile so I can't help much, from the test results it is clear that MS broke the API starting on Vista, the.tests need to be.enhaced so that they pass on both pré vista and post xp. In the sockets tests I have made that many times due to other API breaks, look for the word behavior around tests line 1760. So you can split and make tests pass on all systems. On Monday, October 17, 2016, Roman Pišl <rpisl(a)seznam.cz> wrote: > IP dual stack (v4+v6) should be disabled by default, but previous code > was setting IPV6_V6ONLY in bind() which prevented user to override it. > This patch moves setting IPV6_V6ONLY to socket creation time. > > Based on https://www.winehq.org/pipermail/wine-patches/2016- > July/151919.html > > Signed-off-by: Matthieu Nottale <matthieu.nottale(a)infinit.io > <javascript:;>> > Signed-off-by: Roman Pišl <rpisl(a)seznam.cz <javascript:;>> > --- > dlls/ws2_32/socket.c | 23 +++++++++------------- > dlls/ws2_32/tests/sock.c | 50 ++++++++++++++++++++++++++++++ > ++++++++++++++++-- > 2 files changed, 57 insertions(+), 16 deletions(-) > > diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c > index e8c5130..d14e9d6 100644 > --- a/dlls/ws2_32/socket.c > +++ b/dlls/ws2_32/socket.c > @@ -3240,20 +3240,6 @@ int WINAPI WS_bind(SOCKET s, const struct > WS_sockaddr* name, int namelen) > } > else > { > -#ifdef IPV6_V6ONLY > - const struct sockaddr_in6 *in6 = (const struct > sockaddr_in6*) &uaddr; > - if (name->sa_family == WS_AF_INET6 && > - !memcmp(&in6->sin6_addr, &in6addr_any, sizeof(struct > in6_addr))) > - { > - int enable = 1; > - if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, > &enable, sizeof(enable)) == -1) > - { > - release_sock_fd( s, fd ); > - SetLastError(WSAEAFNOSUPPORT); > - return SOCKET_ERROR; > - } > - } > -#endif > if (name->sa_family == WS_AF_INET) > { > struct sockaddr_in *in4 = (struct sockaddr_in*) > &uaddr; > @@ -7233,6 +7219,15 @@ SOCKET WINAPI WSASocketW(int af, int type, int > protocol, > TRACE("\tcreated %04lx\n", ret ); > if (ipxptype > 0) > set_ipx_packettype(ret, ipxptype); > + > +#ifdef IPV6_V6ONLY > + if (unixaf == AF_INET6) > + { > + int enable = 1; > + int fd = get_sock_fd(ret, 0, NULL); > + setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &enable, > sizeof(enable)); > + } > +#endif > return ret; > } > > diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c > index a144bd3..a676255 100644 > --- a/dlls/ws2_32/tests/sock.c > +++ b/dlls/ws2_32/tests/sock.c > @@ -1732,7 +1732,7 @@ static void test_so_reuseaddr(void) > DWORD err; > > saddr.sin_family = AF_INET; > - saddr.sin_port = htons(9375); > + saddr.sin_port = htons(SERVERPORT+1); > saddr.sin_addr.s_addr = inet_addr("127.0.0.1"); > > s1=socket(AF_INET, SOCK_STREAM, 0); > @@ -6110,6 +6110,8 @@ static void test_ipv6only(void) > struct sockaddr_in sin4; > struct sockaddr_in6 sin6; > int ret; > + int enabled; > + int len; > > memset(&sin4, 0, sizeof(sin4)); > sin4.sin_family = AF_INET; > @@ -6142,6 +6144,50 @@ static void test_ipv6only(void) > ok(!ret, "Could not bind IPv4 address (LastError: %d; %d expected if > IPv6 binds to IPv4 as well).\n", > WSAGetLastError(), WSAEADDRINUSE); > > + closesocket(v4); > + closesocket(v6); > + v4 = INVALID_SOCKET; > + v6 = INVALID_SOCKET; > + > + /* Test again, this time disabling V6ONLY. */ > + sin4.sin_port = htons(SERVERPORT+2); > + sin6.sin6_port = htons(SERVERPORT+2); > + > + v6 = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP); > + ok(v6 != INVALID_SOCKET, "Could not create IPv6 socket (LastError: > %d; %d expected if IPv6 not available).\n", > + WSAGetLastError(), WSAEAFNOSUPPORT); > + > + enabled = 2; > + len = sizeof(enabled); > + ret = getsockopt(v6, IPPROTO_IPV6, IPV6_V6ONLY, (char*)&enabled, > &len); > + if (ret) { > + skip("Could not check IPV6_V6ONLY (option not supported?) > ((LastError: %d).\n", WSAGetLastError()); > + goto end; > + } > + ok(enabled == 1, "IPV6_V6ONLY is not enabled by default.\n"); > + > + enabled = 0; > + ret = setsockopt(v6, IPPROTO_IPV6, IPV6_V6ONLY, (char*)&enabled, len); > + ok(!ret, "Failed to disable IPV6_V6ONLY (LastError: %d).\n", > WSAGetLastError()); > + > + ret = bind(v6, (struct sockaddr*)&sin6, sizeof(sin6)); > + ok(!ret, "Could not bind IPv6 address (LastError: %d).\n", > WSAGetLastError()); > + > + enabled = 2; > + len = sizeof(enabled); > + getsockopt(v6, IPPROTO_IPV6, IPV6_V6ONLY, (char*)&enabled, &len); > + ok(enabled == 0, "IPV6_V6ONLY is enabled after bind.\n"); > + > + v4 = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); > + ok(v4 != INVALID_SOCKET, "Could not create IPv4 socket (LastError: > %d).\n", > + WSAGetLastError()); > + > + WSASetLastError(0xdeadbeef); > + bind(v4, (struct sockaddr*)&sin4, sizeof(sin4)); > + > + ok(WSAGetLastError() == WSAEADDRINUSE, "Failed to disable IPV6_V6ONLY > (LastError: %d; %d expected if IPv6 binds to IPv4 as well).\n", > + WSAGetLastError(), WSAEADDRINUSE); > + > end: > if (v4 != INVALID_SOCKET) > closesocket(v4); > @@ -8024,7 +8070,7 @@ static void test_TransmitFile(void) > /* Setup a properly connected socket for transfers */ > memset(&bindAddress, 0, sizeof(bindAddress)); > bindAddress.sin_family = AF_INET; > - bindAddress.sin_port = htons(9375); > + bindAddress.sin_port = htons(SERVERPORT+1); > bindAddress.sin_addr.s_addr = inet_addr("127.0.0.1"); > iret = bind(server, (struct sockaddr*)&bindAddress, > sizeof(bindAddress)); > if (iret != 0) > -- > 2.7.3 > > > >

Oct. 17, 2016

Re: [PATCH] ws2_32: Allow user to enable IP dual stack (v4).

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=26191 Your paranoid android. === wvistau64 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: -559038737; 10048 expected if IPv6 binds to IPv4 as well). === w2008s64 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: -559038737; 10048 expected if IPv6 binds to IPv4 as well). === w7u (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === w7pro64 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). sock.c:7810: Test failed: bytesReturned isn't supposed to be 1 sock.c:7834: Test failed: GetOverlappedResult failed, error 0 === w8 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === w864 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === w1064 (32 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === wvistau64 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: -559038737; 10048 expected if IPv6 binds to IPv4 as well). === w2008s64 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: -559038737; 10048 expected if IPv6 binds to IPv4 as well). === w7pro64 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). sock.c:7810: Test failed: bytesReturned isn't supposed to be 1 sock.c:7856: Test failed: Waiting for timeout failed with 0 + errno 997 === w864 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well). === w1064 (64 bit sock) === sock.c:6188: Test failed: Failed to disable IPV6_V6ONLY (LastError: 0; 10048 expected if IPv6 binds to IPv4 as well).

Oct. 17, 2016

Re: [PATCH v2] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 17.10.2016 15:29, Aric Stewart wrote: > v2: Suggestions from Sebastian Lackner > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus.h | 3 ++ > dlls/winebus.sys/bus_udev.c | 73 ++++++++++++++++++++++++++ > dlls/winebus.sys/main.c | 123 +++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 198 insertions(+), 1 deletion(-) > > > > v2-0001-winebus.sys-Handle-device-reports-for-hidraw-device.txt > > > diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h > index 7ce54bf..bcb1163 100644 > --- a/dlls/winebus.sys/bus.h > +++ b/dlls/winebus.sys/bus.h > @@ -25,6 +25,8 @@ typedef struct > int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev); > NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length); > NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length); > + NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device, BYTE **report_buffer, DWORD *buffer_length); > + void (*end_report_processing)(DEVICE_OBJECT *device, BYTE *report_buffer); > } platform_vtbl; > > void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN; > void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN; > +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) DECLSPEC_HIDDEN; > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c > index 243aabb..37c8412 100644 > --- a/dlls/winebus.sys/bus_udev.c > +++ b/dlls/winebus.sys/bus_udev.c > @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > > #ifdef HAVE_UDEV > > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > + > static struct udev *udev_context = NULL; > static DRIVER_OBJECT *udev_driver_obj = NULL; > > @@ -73,6 +75,11 @@ struct platform_private > { > struct udev_device *udev_device; > int device_fd; > + > + BYTE *report_buffer; > + int buffer_length; > + HANDLE report_thread; > + int control_pipe[2]; > }; > > static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device) > @@ -222,11 +229,77 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf > return STATUS_SUCCESS; > } > > +static DWORD CALLBACK device_report_thread(void *args) > +{ > + DEVICE_OBJECT *device = (DEVICE_OBJECT*)args; > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + struct pollfd plfds[2]; > + > + plfds[0].fd = private->device_fd; > + plfds[0].events = POLLIN; > + plfds[0].revents = 0; > + plfds[1].fd = private->control_pipe[0]; > + plfds[1].events = POLLIN; > + plfds[1].revents = 0; > + > + while (1) > + { > + int size; > + if (poll(plfds, 2, -1) <= 0) continue; > + if (plfds[1].revents || !private->report_buffer || private->buffer_length == 0) > + break; > + size = read(plfds[0].fd, private->report_buffer, private->buffer_length); This could still cause problems when another thread copies the last_report while it is updated. I don't know how you are planning to implement it for other platforms, but wouldn't it be better to use a separate buffer in this thread, and let process_hid_report() do the copying? > + if (size == -1) > + TRACE_(hid_report)("Read failed. Likely an unplugged device\n"); > + else if (size == 0) > + TRACE_(hid_report)("Failed to read report\n"); > + else > + process_hid_report(device, hidraw_busidW, private->report_buffer, size); > + } > + return 0; > +} > + > +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device, BYTE **buffer, DWORD *length) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + *length = private->buffer_length = 1024; > + *buffer = private->report_buffer = HeapAlloc(GetProcessHeap(), 0, *length); > + > + pipe(private->control_pipe); > + private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL); > + if (!private->report_thread) > + { > + ERR("Unable to create device report thread\n"); > + close(private->control_pipe[0]); > + close(private->control_pipe[1]); > + return STATUS_UNSUCCESSFUL; > + } > + else > + return STATUS_SUCCESS; > +} > + > +static void end_report_processing(DEVICE_OBJECT *device, BYTE *buffer) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + if (private->report_thread) > + { > + write(private->control_pipe[1], "q", 1); > + WaitForSingleObject(private->report_thread, INFINITE); > + close(private->control_pipe[0]); > + close(private->control_pipe[1]); > + } > + private->buffer_length = 0; > + private->report_buffer = NULL; > + HeapFree(GetProcessHeap(), 0, buffer); > +} > + > static const platform_vtbl hidraw_vtbl = > { > compare_platform_device, > hidraw_get_reportdescriptor, > hidraw_get_string, > + begin_report_processing, > + end_report_processing, > }; > > static void try_add_device(struct udev_device *dev) > diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c > index 86adbf5..49dcc97 100644 > --- a/dlls/winebus.sys/main.c > +++ b/dlls/winebus.sys/main.c > @@ -40,6 +40,7 @@ > #include "bus.h" > > WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > > struct pnp_device > { > @@ -58,6 +59,14 @@ struct device_extension > const WCHAR *busid; /* Expected to be a static constant */ > > const platform_vtbl *vtbl; > + > + BOOL processing_started; > + BYTE *last_report; > + DWORD last_report_size; > + DWORD buffer_size; > + LIST_ENTRY irp_queue; > + CRITICAL_SECTION report_cs; > + > BYTE platform_private[1]; > }; > > @@ -214,6 +223,9 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > ext->busid = busidW; > ext->vtbl = vtbl; > > + InitializeListHead(&ext->irp_queue); > + InitializeCriticalSection(&ext->report_cs); You might want to give this CS a name, like done in other parts of wine. For example: --- snip --- InitializeCriticalSection(&src->lock); src->lock.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": XA2SourceImpl.lock"); --- snip --- > + > /* add to list of pnp devices */ > pnp_dev->device = device; > list_add_tail(&pnp_devset, &pnp_dev->entry); > @@ -271,6 +283,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > { > struct device_extension *ext = (struct device_extension *)device->DeviceExtension; > struct pnp_device *pnp_device = ext->pnp_device; > + LIST_ENTRY *entry; > + IRP *irp; > > TRACE("(%p)\n", device); > > @@ -279,6 +293,23 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > LeaveCriticalSection(&device_list_cs); > > IoInvalidateDeviceRelations(device, RemovalRelations); > + > + if (ext->processing_started) > + ext->vtbl->end_report_processing(device, ext->last_report); > + > + /* Cancel pending IRPs */ > + EnterCriticalSection(&ext->report_cs); > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irp->IoStatus.u.Status = STATUS_CANCELLED; > + irp->IoStatus.Information = 0; > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + LeaveCriticalSection(&ext->report_cs); > + You will have to delete the CS here. Also make sure to reset Spare[0] if it was previously set to a debug string. For example: --- snip --- src->lock.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&src->lock); --- snip --- > HeapFree(GetProcessHeap(), 0, ext->serial); > IoDeleteDevice(device); > > @@ -432,6 +463,58 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); > break; > } > + case IOCTL_HID_GET_INPUT_REPORT: > + { > + HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); > + TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n"); > + EnterCriticalSection(&ext->report_cs); > + if (!ext->processing_started) > + { > + irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size); > + if (status == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + { > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + } > + > + if (packet->reportBufferLen < ext->last_report_size) > + { > + irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + > + ZeroMemory(packet->reportBuffer, packet->reportBufferLen); Do we need this line? Usually callers do not expect valid data after the length provided by irp->IoStatus.Information. Is it different in this case? > + memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size); > + packet->reportBufferLen = ext->last_report_size; > + irp->IoStatus.Information = ext->last_report_size; > + irp->IoStatus.u.Status = status = STATUS_SUCCESS; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + case IOCTL_HID_READ_REPORT: > + { > + TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); > + EnterCriticalSection(&ext->report_cs); > + if (!ext->processing_started) > + { > + if (ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size) == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + { > + irp->IoStatus.u.Status = status; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > + } > + InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); > + status = STATUS_PENDING; > + LeaveCriticalSection(&ext->report_cs); > + break; > + } > default: > { > ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode; > @@ -441,11 +524,49 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > } > } > > - IoCompleteRequest(irp, IO_NO_INCREMENT); > + if (status != STATUS_PENDING) > + IoCompleteRequest(irp, IO_NO_INCREMENT); > > return status; > } > > +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) > +{ > + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; > + IRP *irp; > + LIST_ENTRY *entry; > + > + EnterCriticalSection(&ext->report_cs); > + if (ext->last_report && report != ext->last_report && ext->buffer_size <= length) Shouldn't it be "length <= ext->buffer_size" ? > + { > + memcpy(ext->last_report, report, length); > + ext->last_report_size = length; > + } > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + IO_STACK_LOCATION *irpsp; > + TRACE_(hid_report)("Processing Request\n"); > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irpsp = IoGetCurrentIrpStackLocation(irp); > + > + if (irpsp->Parameters.DeviceIoControl.OutputBufferLength < length) > + { > + irp->IoStatus.Information = 0; > + irp->IoStatus.u.Status = STATUS_BUFFER_TOO_SMALL; > + } > + else > + { > + memcpy(irp->UserBuffer, report, length); > + irp->IoStatus.Information = length; > + irp->IoStatus.u.Status = STATUS_SUCCESS; > + } > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + LeaveCriticalSection(&ext->report_cs); > +} > + > NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path ) > { > static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0}; > > >

Oct. 17, 2016

Re: [PATCH] hid/tests: Add HID device enumeration test

by Sebastian Lackner

On 14.10.2016 11:36, Aric Stewart wrote: > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > configure | 1 + > configure.ac | 1 + > dlls/hid/tests/Makefile.in | 5 +++ > dlls/hid/tests/device.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 87 insertions(+) > create mode 100644 dlls/hid/tests/Makefile.in > create mode 100644 dlls/hid/tests/device.c > > > > 0001-hid-tests-Add-HID-device-enumeration-test.txt > > > diff --git a/configure b/configure > index 99793b0..7bb6fef 100755 > --- a/configure > +++ b/configure > @@ -17936,6 +17936,7 @@ wine_fn_config_dll gpkcsp enable_gpkcsp > wine_fn_config_dll hal enable_hal > wine_fn_config_dll hhctrl.ocx enable_hhctrl_ocx clean,implib htmlhelp > wine_fn_config_dll hid enable_hid implib > +wine_fn_config_test dlls/hid/tests hid_test > wine_fn_config_dll hidclass.sys enable_hidclass_sys implib hidclass > wine_fn_config_dll hlink enable_hlink clean,implib > wine_fn_config_test dlls/hlink/tests hlink_test > diff --git a/configure.ac b/configure.ac > index ba8fd0f..9cab382 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -3025,6 +3025,7 @@ WINE_CONFIG_DLL(gpkcsp) > WINE_CONFIG_DLL(hal) > WINE_CONFIG_DLL(hhctrl.ocx,,[clean,implib],[htmlhelp]) > WINE_CONFIG_DLL(hid,,[implib]) > +WINE_CONFIG_TEST(dlls/hid/tests) > WINE_CONFIG_DLL(hidclass.sys,,[implib],[hidclass]) > WINE_CONFIG_DLL(hlink,,[clean,implib]) > WINE_CONFIG_TEST(dlls/hlink/tests) > diff --git a/dlls/hid/tests/Makefile.in b/dlls/hid/tests/Makefile.in > new file mode 100644 > index 0000000..d4a69f0 > --- /dev/null > +++ b/dlls/hid/tests/Makefile.in > @@ -0,0 +1,5 @@ > +TESTDLL = hid.dll > +IMPORTS = hid setupapi > + > +C_SRCS = \ > + device.c \ The "\" at the end seems wrong. Please note that you can also use tools/make_makefiles to update/fix this file automatically for you. > diff --git a/dlls/hid/tests/device.c b/dlls/hid/tests/device.c > new file mode 100644 > index 0000000..f3260ec > --- /dev/null > +++ b/dlls/hid/tests/device.c > @@ -0,0 +1,80 @@ > +/* > + * Copyright (c) 2016 Aric Stewart > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA > + */ > + > +#include "ntstatus.h" > +#define WIN32_NO_STATUS > +#include "windows.h" > +#include "setupapi.h" > + > +#ifndef WINE_NTSTATUS_DECLARED > +#define WINE_NTSTATUS_DECLARED > +typedef LONG NTSTATUS; > +#endif Such hacks should be avoided in tests. I believe you are trying to workaround a bug in the Wine hidsdi.h header file. Wine declares NTSTATUS after the includes: --- snip --- #include <hidusage.h> #include <ddk/hidpi.h> #ifndef WINE_NTSTATUS_DECLARED #define WINE_NTSTATUS_DECLARED typedef LONG NTSTATUS; #endif --- snip --- On Windows its declared before: --- snip --- typedef _Return_type_success_(return >= 0) LONG NTSTATUS; #include "hidusage.h" #include "hidpi.h" --- snip --- > + > +#include "ddk/hidsdi.h" > + > +#include "wine/test.h" > + > +static void enumerate_devices(void) > +{ > + GUID hid_guid; > + WCHAR device_name[128]; > + HDEVINFO info_set; > + DWORD index = 0; > + SP_DEVICE_INTERFACE_DATA interface_data; > + DWORD detail_size = MAX_PATH * sizeof(WCHAR); > + SP_DEVICE_INTERFACE_DETAIL_DATA_W *data; > + > + HidD_GetHidGuid(&hid_guid); > + > + ZeroMemory(&interface_data, sizeof(interface_data)); > + interface_data.cbSize = sizeof(interface_data); > + > + data = HeapAlloc(GetProcessHeap(), 0, sizeof(*data) + detail_size); > + data->cbSize = sizeof(*data); > + > + info_set = SetupDiGetClassDevsW(&hid_guid, NULL, NULL, DIGCF_DEVICEINTERFACE); Shouldn't this be released afterwards? > + while (SetupDiEnumDeviceInterfaces(info_set, NULL, &hid_guid, index, &interface_data)) > + { > + index ++; > + > + if (SetupDiGetDeviceInterfaceDetailW(info_set, &interface_data, data, detail_size, NULL, NULL)) Shouldn't the DeviceInterfaceDetailDataSize parameter also include the sizeof(*data) ? > + { > + PHIDP_PREPARSED_DATA ppd; > + HIDP_CAPS Caps; > + HANDLE file = CreateFileW(data->DevicePath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0); > + if (file == INVALID_HANDLE_VALUE) > + { > + trace("Failed to access device %s, likely not plugged in or access is denied.\n", wine_dbgstr_w(data->DevicePath)); > + continue; > + } > + ok(HidD_GetPreparsedData(file, &ppd), "Failed to preparsed data\n"); Is there a "get" missing in the debug string? > + ok(HidP_GetCaps(ppd, &Caps) == HIDP_STATUS_SUCCESS, "Failed to get Caps\n"); > + ok(HidD_GetProductString(file, device_name, sizeof(device_name)), "Failed to get product string\n"); > + trace("Found device %s (%s, %02x, %02x)\n", wine_dbgstr_w(device_name), wine_dbgstr_w(data->DevicePath), Caps.UsagePage, Caps.Usage); > + ok(HidD_FreePreparsedData(ppd), "Failed to free preparsed data\n"); Its a matter of taste, but usually its better to assign those values to a variable. This also allows to print the error code / GetLastError() in case of a failure. > + CloseHandle(file); > + } > + } > + HeapFree(GetProcessHeap(), 0, data); > +} > + > +START_TEST(device) > +{ > + enumerate_devices(); > +} > > >

Oct. 17, 2016

Re: [PATCH] winebus.sys: Handle device reports for hidraw devices

by Sebastian Lackner

On 17.10.2016 06:41, Aric Stewart wrote: > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus.h | 3 ++ > dlls/winebus.sys/bus_udev.c | 63 +++++++++++++++++++++++++++++ > dlls/winebus.sys/main.c | 97 ++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 162 insertions(+), 1 deletion(-) > > > > 0001-winebus.sys-Handle-device-reports-for-hidraw-devices.txt > > > diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h > index 7ce54bf..bcb1163 100644 > --- a/dlls/winebus.sys/bus.h > +++ b/dlls/winebus.sys/bus.h > @@ -25,6 +25,8 @@ typedef struct > int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev); > NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length); > NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length); > + NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device, BYTE **report_buffer, DWORD *buffer_length); > + void (*end_report_processing)(DEVICE_OBJECT *device, BYTE *report_buffer); > } platform_vtbl; > > void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN; > void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; > NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN; > +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) DECLSPEC_HIDDEN; > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c > index 243aabb..5307224 100644 > --- a/dlls/winebus.sys/bus_udev.c > +++ b/dlls/winebus.sys/bus_udev.c > @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > > #ifdef HAVE_UDEV > > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > + > static struct udev *udev_context = NULL; > static DRIVER_OBJECT *udev_driver_obj = NULL; > > @@ -73,6 +75,10 @@ struct platform_private > { > struct udev_device *udev_device; > int device_fd; > + > + BYTE *report_buffer; > + int buffer_length; > + HANDLE report_thread; > }; > > static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device) > @@ -222,11 +228,68 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf > return STATUS_SUCCESS; > } > > +static DWORD CALLBACK device_report_thread(void *args) > +{ > + DEVICE_OBJECT *device = (DEVICE_OBJECT*)args; > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + struct pollfd plfds[1]; > + > + plfds[0].fd = private->device_fd; > + plfds[0].events = POLLIN; > + > + while (1) > + { > + int size; > + if (poll(plfds, 1, -1) <= 0) continue; > + if (!private->report_buffer || private->buffer_length == 0) > + break; > + size = read(plfds[0].fd, private->report_buffer, private->buffer_length); > + if (size == -1) > + TRACE_(hid_report)("Read failed. Likely an unplugged device\n"); > + else if (size == 0) > + TRACE_(hid_report)("Failed to read report\n"); > + else > + process_hid_report(device, hidraw_busidW, private->report_buffer, size); > + } > + return 0; > +} > + > +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device, BYTE **buffer, DWORD *length) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + *length = private->buffer_length = 1024; > + *buffer = private->report_buffer = HeapAlloc(GetProcessHeap(), 0, *length); > + > + private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL); > + if (!private->report_thread) > + { > + ERR("Unable to create device report thread\n"); > + return STATUS_UNSUCCESSFUL; > + } > + else > + return STATUS_SUCCESS; > +} > + > +static void end_report_processing(DEVICE_OBJECT *device, BYTE *buffer) > +{ > + struct platform_private *private = impl_from_DEVICE_OBJECT(device); > + if (private->report_thread) > + { > + TerminateThread(private->report_thread, 0); TerminateThread is not really the best way to clean up this worker thread. Depending on when exactly it is called, it can corrupt internal data and crash the whole process. In this case, it would definitely be better to have some synchronization mechanism (for example event pipe) to allow a clean shutdown. > + WaitForSingleObject(private->report_thread, INFINITE); > + } > + private->buffer_length = 0; > + private->report_buffer = NULL; > + HeapFree(GetProcessHeap(), 0, buffer); > +} > + > static const platform_vtbl hidraw_vtbl = > { > compare_platform_device, > hidraw_get_reportdescriptor, > hidraw_get_string, > + begin_report_processing, > + end_report_processing, > }; > > static void try_add_device(struct udev_device *dev) > diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c > index 86adbf5..f12cdf7 100644 > --- a/dlls/winebus.sys/main.c > +++ b/dlls/winebus.sys/main.c > @@ -40,6 +40,7 @@ > #include "bus.h" > > WINE_DEFAULT_DEBUG_CHANNEL(plugplay); > +WINE_DECLARE_DEBUG_CHANNEL(hid_report); > > struct pnp_device > { > @@ -58,6 +59,13 @@ struct device_extension > const WCHAR *busid; /* Expected to be a static constant */ > > const platform_vtbl *vtbl; > + > + BOOL processing_started; > + BYTE *last_report; > + DWORD last_report_size; > + DWORD buffer_size; > + LIST_ENTRY irp_queue; > + > BYTE platform_private[1]; > }; > > @@ -214,6 +222,8 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, > ext->busid = busidW; > ext->vtbl = vtbl; > > + InitializeListHead(&ext->irp_queue); > + > /* add to list of pnp devices */ > pnp_dev->device = device; > list_add_tail(&pnp_devset, &pnp_dev->entry); > @@ -271,6 +281,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > { > struct device_extension *ext = (struct device_extension *)device->DeviceExtension; > struct pnp_device *pnp_device = ext->pnp_device; > + LIST_ENTRY *entry; > + IRP *irp; > > TRACE("(%p)\n", device); > > @@ -279,6 +291,21 @@ void bus_remove_hid_device(DEVICE_OBJECT *device) > LeaveCriticalSection(&device_list_cs); > > IoInvalidateDeviceRelations(device, RemovalRelations); > + > + /* Cancel pending IRPs */ > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + irp->IoStatus.u.Status = STATUS_CANCELLED; > + irp->IoStatus.Information = 0; > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > + > + if (ext->processing_started) > + ext->vtbl->end_report_processing(device, ext->last_report); > + > HeapFree(GetProcessHeap(), 0, ext->serial); > IoDeleteDevice(device); > > @@ -432,6 +459,49 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR); > break; > } > + case IOCTL_HID_GET_INPUT_REPORT: > + { > + HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer); > + TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n"); > + if (!ext->processing_started) > + { > + irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size); > + if (status == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + break; > + } We might need synchronization here to ensure that only one worker thread is started (if there are multiple IOCTL_HID_GET_INPUT_REPORT requests at the same time). > + > + if (packet->reportBufferLen < ext->last_report_size) > + { > + irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL; > + break; > + } > + > + ZeroMemory(packet->reportBuffer, packet->reportBufferLen); > + memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size); The worker thread always uses the same buffer. How do you ensure that the "last_report" you are copying here is a complete record, and not corrupted (for example partially old data)? > + packet->reportBufferLen = ext->last_report_size; > + irp->IoStatus.Information = ext->last_report_size; > + irp->IoStatus.u.Status = status = STATUS_SUCCESS; > + break; > + } > + case IOCTL_HID_READ_REPORT: > + { > + TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n"); > + if (!ext->processing_started) > + { > + if (ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size) == STATUS_SUCCESS) > + ext->processing_started = TRUE; > + else > + { > + irp->IoStatus.u.Status = status; > + break; > + } > + } > + InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry); The irp_queue will require locking when it is accessed from multiple threads. > + status = STATUS_PENDING; Thats fine for now, but I'm planning to send some additional patches soon to fix STATUS_PENDING handling. Drivers on Windows have to call IoMarkIrpPending() when they return STATUS_PENDING. Also see: https://msdn.microsoft.com/en-us/library/windows/hardware/ff549422(v=vs.85)… """Otherwise, the I/O manager attempts to complete the IRP as soon as the dispatch routine returns control.""" and """If a driver queues incoming IRPs, it should call IoMarkIrpPending before it queues each IRP. Otherwise, an IRP could be dequeued, completed by another driver routine, and freed by the system before the call to IoMarkIrpPending occurs, thereby causing a crash. """ > + break; > + } > default: > { > ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode; > @@ -441,11 +511,36 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) > } > } > > - IoCompleteRequest(irp, IO_NO_INCREMENT); > + if (status != STATUS_PENDING) > + IoCompleteRequest(irp, IO_NO_INCREMENT); > > return status; > } > > +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) > +{ > + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; > + IRP *irp; > + LIST_ENTRY *entry; > + > + if (ext->last_report && report != ext->last_report && ext->buffer_size <= length) > + { > + memcpy(ext->last_report, report, length); > + ext->last_report_size = length; > + } > + entry = RemoveHeadList(&ext->irp_queue); > + while(entry != &ext->irp_queue) > + { > + TRACE_(hid_report)("Processing Request\n"); > + irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); > + memcpy(irp->UserBuffer, report, length); Shouldn't there also be some kind of length validation? > + irp->IoStatus.Information = length; > + irp->IoStatus.u.Status = STATUS_SUCCESS; > + IoCompleteRequest(irp, IO_NO_INCREMENT); > + entry = RemoveHeadList(&ext->irp_queue); > + } > +} > + > NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path ) > { > static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0}; > > >

Oct. 17, 2016

Re: Imort system certificates in crypt32@Wine

by Jacek Caban

Hi Donat, On 10/16/16 9:34 PM, Donat Enikeev wrote: > Hi Jacek, > >> I agree that hardcoded paths are not nice, but if we don't have >> better generic solution, we have to live with that > > So, could expressing certificates into registry once and beforehand > during wine-prefix creation - be that one "better generic solution"? I > think I could spend some time on it, if that considered a right way to go This solution would change nothing in that regards - you still need hardcoded list of paths to do the import. You would just use it slightly differently. Also, I did not say to do that for wine prefix creation. User may intentionally remove a certificate in host system after wineserver is created and we should reflect that in Wine (not to mention moving wine prefix to another machine). That's why I suggested using REG_OPTION_VIOLATILE - so that registry keys would be gone when wineserver terminates (aka. Windows shutdown) and we'd recreate them the next time root store is created. And yes, I would consider such solution better. >> How about instead of current CRYPT_RootOpenStore call, we'd create >> registry entries expressing system certificates using >> REG_OPTION_VIOLATILE once for Wine session? > > That will work as well, but would require another custom wine-specific > piece of code for the certificates import using that flag for registry > keys OR passing special flag to the existing Store management functions. I'm not sure why you need more that we already have. We already have special handling for root store in CRYPT_SysRegOpenStoreW. You could just change it to call new import_system_certs function and continue like for other stores instead of calling CRYPT_RootOpenStore and returning. > In general, if sticking to the current way of implementation is the > most reasonable option in the context, just proper handling of flags > and priorities for collections is required (following Remarks at > https://msdn.microsoft.com/en-us/library/windows/desktop/aa376022(v=vs.85).… > ), so the collection will forward added certificates to the > appropriate store: registry one with RW access, not the memory based > created by CRYPT_RootOpenStore with RO access I would need deeper look at the code to comment that. However, note that with that we'd use registry stores that are already used for non-root cases. That said, I wouldn't be surprised if you find that existing code would needs fixes. Thanks, Jacek

Oct. 16, 2016

Re: Imort system certificates in crypt32@Wine

by Donat Enikeev

Hi Jacek, > I agree that hardcoded paths are not nice, but if we don't have > better generic solution, we have to live with that So, could expressing certificates into registry once and beforehand during wine-prefix creation - be that one "better generic solution"? I think I could spend some time on it, if that considered a right way to go > How about instead of current CRYPT_RootOpenStore call, we'd create > registry entries expressing system certificates using > REG_OPTION_VIOLATILE once for Wine session? That will work as well, but would require another custom wine-specific piece of code for the certificates import using that flag for registry keys OR passing special flag to the existing Store management functions. In general, if sticking to the current way of implementation is the most reasonable option in the context, just proper handling of flags and priorities for collections is required (following Remarks at https://msdn.microsoft.com/en-us/library/windows/desktop/aa376022(v=vs.85).… ), so the collection will forward added certificates to the appropriate store: registry one with RW access, not the memory based created by CRYPT_RootOpenStore with RO access Considering these, what do you think? Best, Donnie On Вс, окт 16, 2016 at 10:07 , Jacek Caban <jacek(a)codeweavers.com> wrote: > Hi Donat, > > > On 10/16/16 7:50 PM, Donat Enikeev wrote: >> Hi Guys, >> Watched video recently from WineConf 2015, and resulting directions >> around opening community looks really promising, so I am performing >> 3rd attempt to be useful here :) >> The bug https://bugs.winehq.org/show_bug.cgi?id=30187 : Cisco IP >> Communicator failing to setup due to 'certmgr.exe' tool fails to >> install certificate to the system-wide trusted certificate store. >> Wine uses registry-based certificates stores by default, but treats >> HKLM\Root store as a special case. Whenever app opens such store, >> Crypt32 goes through hard-coded paths in rootstore.c: >> static const char * const CRYPT_knownLocations[] = { >> "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", ... }; >> And adds all found certificates to the special store. In the context >> of bug, this special-case store doesn't support adding certificates, >> and thus Cisco IP fails to install. >> So a backward compatible patch (attached) that just fixes this bug >> looks straightforward: make a stores collection, add there HKLM\Root >> certificates registry store at first and then that system store with >> certificates from the environment, and return the collection. It >> will allow applications do whatever they used to with certificates, >> keeping all the linux certificates available for verification of any >> kind. >> The problem with this approach is that current wine crypt32 doesn't >> actually save certificates that were added to a collection of stores >> (the test for this attached), while Win does. Although it could be >> fixed with a different patch in one function, >> but I would like to hear your thoughts first around following >> questions: >> 1. Does this backward-compatible patch-set make sense at all and >> worth proceeding? Probably you have some ongoing activities >> 2. Do you still think that wine should import system certificates >> during HKLM\Root request at all, not just shipping with those from >> typical windows installation? >> 3. Have you considered different approach of utilizing system >> certificates in Wine? For example, import all system certificates to >> the *registry* during wine-prefix initialization process, and work >> with them from there in a way windows does (even native crypt32 will >> benefit from this approach). That will allow to unify and simplify >> crypt32 and remove all that arguable hard-coded paths in the code, >> and bring more familiar environment to the windows application and, >> at the same time, isolation. > > First of all, I think we want to keep current solution of using host > system's cert store and configuration as much as possible. I agree > that hardcoded paths are not nice, but if we don't have better > generic solution, we have to live with that (although it's possible > to improve it in some cases, like it's done for Mac). > > That said, I think that expressing system certificates in Wine > registry would be the right solution. How about instead of current > CRYPT_RootOpenStore call, we'd create registry entries expressing > system certificates using REG_OPTION_VIOLATILE once for Wine session? > Then we could use regular registry store for root store. > > Thanks, > Jacek * > *

Oct. 16, 2016

Re: Imort system certificates in crypt32@Wine

by Jacek Caban

Hi Donat, On 10/16/16 7:50 PM, Donat Enikeev wrote: > Hi Guys, > Watched video recently from WineConf 2015, and resulting directions > around opening community looks really promising, so I am performing > 3rd attempt to be useful here :) > The bug https://bugs.winehq.org/show_bug.cgi?id=30187 : Cisco IP > Communicator failing to setup due to 'certmgr.exe' tool fails to > install certificate to the system-wide trusted certificate store. > Wine uses registry-based certificates stores by default, but treats > HKLM\Root store as a special case. Whenever app opens such store, > Crypt32 goes through hard-coded paths in rootstore.c: > static const char * const CRYPT_knownLocations[] = > { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", ... }; > And adds all found certificates to the special store. In the context > of bug, this special-case store doesn't support adding certificates, > and thus Cisco IP fails to install. > So a backward compatible patch (attached) that just fixes this bug > looks straightforward: make a stores collection, add there HKLM\Root > certificates registry store at first and then that system store with > certificates from the environment, and return the collection. It will > allow applications do whatever they used to with certificates, keeping > all the linux certificates available for verification of any kind. > The problem with this approach is that current wine crypt32 doesn't > actually save certificates that were added to a collection of stores > (the test for this attached), while Win does. Although it could be > fixed with a different patch in one function, > but I would like to hear your thoughts first around following questions: > 1. Does this backward-compatible patch-set make sense at all and worth > proceeding? Probably you have some ongoing activities > 2. Do you still think that wine should import system certificates > during HKLM\Root request at all, not just shipping with those from > typical windows installation? > 3. Have you considered different approach of utilizing system > certificates in Wine? For example, import all system certificates to > the *registry* during wine-prefix initialization process, and work > with them from there in a way windows does (even native crypt32 will > benefit from this approach). That will allow to unify and simplify > crypt32 and remove all that arguable hard-coded paths in the code, and > bring more familiar environment to the windows application and, at the > same time, isolation. First of all, I think we want to keep current solution of using host system's cert store and configuration as much as possible. I agree that hardcoded paths are not nice, but if we don't have better generic solution, we have to live with that (although it's possible to improve it in some cases, like it's done for Mac). That said, I think that expressing system certificates in Wine registry would be the right solution. How about instead of current CRYPT_RootOpenStore call, we'd create registry entries expressing system certificates using REG_OPTION_VIOLATILE once for Wine session? Then we could use regular registry store for root store. Thanks, Jacek * *

Oct. 16, 2016

Imort system certificates in crypt32@Wine

by Donat Enikeev

[Resending in plain/text, apologies for html] Hi Guys, Watched video recently from WineConf 2015, and results around opening community looks really promising, so I am performing 3rd attempt to be useful here :) The bug https://bugs.winehq.org/show_bug.cgi?id=30187 : Cisco IP Communicator failing to setup due to 'certmgr.exe' tool fails to install certificate to the system-wide trusted certificate store. Wine uses registry-based certificates stores by default, but treats HKLM\Root store as a special case: whenever app opens such store, Crypt32 goes through hard-coded paths in rootstore.c: static const char * const CRYPT_knownLocations[] = { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", ... }; And adds all found certificates to the special store. In the context of bug, this special-case store doesn't support adding certificates, and thus Cisco IP fails to install. So a backward compatible patch (attached) that just fixes this bug looks straightforward: make a stores collection, add there HKLM\Root certificates registry store first and then, add that system store with certificates from the environment. And return the resulting collection as a store to work with. It will allow applications do whatever they used to with certificates, keeping all the linux certificates available for verification of any kind. The problem with this approach is that current wine crypt32 doesn't actually save certificates that were added to a collection of stores (the test for this attached), while Win does. Although it could be fixed with a different patch in one function, but I would like to hear your thoughts first around following questions: 1. Does this backward-compatible patch-set make sense at all and worth proceeding? Probably you have some ongoing activities 2. Do you still think that wine should import system certificates during HKLM\Root request at all, not just shipping with those from typical windows installation? 3. Have you considered different approach of utilizing system certificates in Wine? For example, import all system certificates to the *registry* during wine-prefix initialization process, and work with them from there in a way windows does (even native crypt32 will benefit from this approach). That will allow to unify and simplify crypt32 and remove all that arguable hard-coded paths in the code, and bring more familiar environment to the windows application and, at the same time, isolation. Please share your thoughts, Donnie

Oct. 16, 2016

Re: mscoree: Add CorIsLatestSvc function stub.

by Sebastian Lackner

On 16.10.2016 17:06, Vincent Povirk wrote: >>> Have you found a real app that calls this? As far as I know, it's only >>> called by native .NET components that shouldn't be used with builtin >>> mscoree. >> >> At least two apps calls this function after .NET 4.5.1 (offline) have >> been installed. > > If .NET is installed then builtin mscoree should not be used. > > How exactly have you installed .NET 4.5.1? It sounds like you are missing some overrides. In case you did a manual installation, please note that mscoree has to be set to "native", not "native,builtin". In the second case builtin can still get loaded when apps look for mscoree at an unusual location, or if the app has the wrong architecture. Regards, Sebastian

Oct. 16, 2016