September 2013 - Wine-devel

Re: ntdll: Implement job objects. (try 3)
by Alexandre Julliard 07 Sep '13

07 Sep '13

Andrew Cook <ariscop(a)gmail.com> writes: > This will be my last submission for this piece of code, i want to work > on other things and it's been almost 2 weeks with zero comments on my > previous submission. It's clearly not ready, and you already know that since you've added TODOs all over the place. -- Alexandre Julliard julliard(a)winehq.org

1 0

RFC: Completely revised SIO_ADDRESS_LIST_CHANGE patches
by Erich E. Hoover 06 Sep '13

06 Sep '13

As some of you know I've been working on fixing SIO_ADDRESS_LIST_CHANGE in order to fix some problems with Silverlight on a variety of streaming sites. Due to some feedback I've received I've gone to the effort to completely revise these patches to work inside of the server instead of using iphlpapi. Any and all feedback is appreciated, special thanks go to Sebastian Lackner for looking over these patches ahead of time. Design considerations: 0001) This patch was modeled after similar behavior for unimplemented ioctl() calls in ntdll 0002) The implementation uses a per-socket queue so that when a socket is destroyed prior to a notification event then that socket's async operations are all appropriately woken up. Only one interface change notification object exists at a time, so the object is destroyed whenever there are no sockets expecting events and is then re-created as needed. 0003) This patch is a simple test case with the rest of the socket tests #ifdef'd out for your convenience Looking forward to hearing back! Best, Erich

1 0

Re: aclui: Remove some unneeded header inclusions. (IWYU)
by Alexandre Julliard 06 Sep '13

06 Sep '13

Amine Khaldi <amine.khaldi(a)reactos.org> writes: > @@ -18,15 +18,7 @@ > * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA > */ > > -#include "config.h" > - > -#include <stdarg.h> > - > #include "initguid.h" > -#include "windef.h" > -#include "winbase.h" > -#include "winuser.h" > -#include "winnt.h" > #include "aclui.h" That's not useful. The headers will be included anyway. -- Alexandre Julliard julliard(a)winehq.org

2 2

Re: [2/6] ntdll: Add support to NtWriteFile for special offset -1.
by Alexandre Julliard 06 Sep '13

06 Sep '13

Dmitry Timoshkov <dmitry(a)baikal.ru> writes: > --- > dlls/ntdll/file.c | 3 +++ > dlls/ntdll/tests/file.c | 15 --------------- > 2 files changed, 3 insertions(+), 15 deletions(-) > > diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c > index 9420df5..9d5b02a 100644 > --- a/dlls/ntdll/file.c > +++ b/dlls/ntdll/file.c > @@ -967,6 +967,9 @@ NTSTATUS WINAPI NtWriteFile(HANDLE hFile, HANDLE hEvent, > goto done; > } > > + if (offset && offset->QuadPart == (LONGLONG)-1 /* FILE_WRITE_TO_END_OF_FILE */) > + offset->QuadPart = lseek( unix_handle, 0, SEEK_END ); > + This looks very wrong, offset belongs to the caller. -- Alexandre Julliard julliard(a)winehq.org

2 1

Re: [PATCH 2/3] ws2_32: Implement get socket option SO_PROTOCOL_INFO (try 3)
by Alexandre Julliard 06 Sep '13

06 Sep '13

Bruno Jesus <00cpxxx(a)gmail.com> writes: > +static int ws_protocol_info(SOCKET s, int unicode, WSAPROTOCOL_INFOW *buffer) > +{ > + int size; > + union _info > + { > + WSAPROTOCOL_INFOA *a; > + WSAPROTOCOL_INFOW *w; > + } info; > + info.w = buffer; > + > + size = unicode ? sizeof(WSAPROTOCOL_INFOW) : sizeof(WSAPROTOCOL_INFOA); > + memset(buffer, 0, size); > + info.w->iAddressFamily = WS_AF_UNSPEC; > + > + SERVER_START_REQ( get_socket_info ) > + { > + req->handle = wine_server_obj_handle( SOCKET2HANDLE(s) ); > + wine_server_call( req ); > + info.w->iAddressFamily = convert_af_u2w(reply->family); > + info.w->iSocketType = convert_socktype_u2w(reply->type); > + info.w->iProtocol = convert_proto_u2w(reply->protocol); > + } > + SERVER_END_REQ; You need to check the server return status and set the correct error code. -- Alexandre Julliard julliard(a)winehq.org

2 1

Re: D3D command stream patches for testing
by LOMBARD Maxime 06 Sep '13

06 Sep '13

Hi Stefan, I've tested two games "FarCry3" and "Splinter Cell Blacklist" with : - Wine 1.7.1 + CS patch + CSMT On/Off - Wine 1.7.1 clean - CSMT On = CSMT enabled + StrictDrawOrdering disabled - CSMT Off = CSMT disabled + StrictDrawOrdering enabled I run the game by *WINEDEBUG=fps,err-all,fixme-all primusrun /home/berillions/Desktop/Build/32/wine-1.7.1-{clean/csmt}/wine "$1" 2>&1 | tee /dev/stderr | grep --line-buffered "^trace:fps:" | osd_cat -c white -s 1 -l2;* ## My Laptop ## - Nvidia GTX670MX - Intel I5-3230M - Debian Sid 64-bits - Nvidia drivers 325.15 ## FarCry 3 tests ## Graphic options : - Texture : High - Ambient Lighting : High - Shadow : Ultra - Post Fx : Ultra - Geometry : Ultra - Vegetation : Very High - Terrain : High - Water : Very High - Environment : High Wine 1.7.1 clean : - Main Menu : ~35 fps - In-game (move or static camera) : ~12 fps average Wine 1.7.1 + CS patch + CSMT Off : - Main Menu : ~45fps - In-game : ~14 fps average Wine 1.7.1 + CS patch + CSMT On : - Main Menu : ~45fps - In-game : ~18 fps average ## Splinter Cell Blacklist ## Graphic Options : - Texture detail : Ultra - Shadow Quality : High - Parallax : On - Tesselation : Off - Texture Filtering : Anisotropic 4x - V-Sync : On - Dynamic ambient Occlusion : Field AO - Anti-Aliasing : FXAA - Directx : Dx9 Wine 1.7.1 clean : - In-game : ~15 fps average Wine 1.7.1 + CS patch + CSMT Off : - In-game : ~15 fps average Wine 1.7.1 + CS patch + CSMT On : - In-game : ~3 fps average In resume, CS patch works for FarCry 3, +6fps but cause a very low fps for SC Blacklist, divide by 5 the fps in game. Max

1 0

Re: crypt32 patch review
by Juan Lang 06 Sep '13

06 Sep '13

[+wine-devel] Hi Jacek, I've added the list so the discussion can take place in public. On Tue, Sep 3, 2013 at 8:50 AM, Jacek Caban <jacek(a)codeweavers.com> wrote: > I have a patch for crypt32. I'd appreciate your review before I submit > it to Wine. It has high potential to be insecure... This should fix Wine bug 28004 as well as some websites that don't send full cert chain in > SSL/TLS handshake, but instead just their own certs. For that we need > two changes. First, we need to look for issuers in global stores (as in > those that are not passed to CertGetCertificateChain). When I did that, > I don't really see a security issue here. The global stores are implicitly trusted, so neglecting to check them was probably an oversight on my part. Your tests imply something is missing. My only real question here is whether the test results are in any way impacted by some unknown caching happening in Windows. Has this chain been verified prior to running the test? I'm going to guess that the test bot machines are "clean" prior to running the tests, but if they download their test executables from winehq.org, that won't be the case. > it came out that it's not enough for intermediate issuers. Those have to > be downloaded if we have information about their location stored in > validated cert. That's easy using cryptnet. Could you please review the > attached patch? > + for(i=0; i < info->cAccDescr; i++) { + if(!strcmp(szOID_PKIX_CA_ISSUERS, info->rgAccDescr[i].pszAccessMethod) + && info->rgAccDescr[i].AccessLocation.dwAltNameChoice == CERT_ALT_NAME_URL) { You explicitly make use of the AIA extension to find the location of intermediate certs. This is reasonable, and it gets your test case to pass, but it won't cover end certs that don't make use of the AIA extension. As I said above, I wonder whether caching is impacting the test results. More: The thing that would be also nice to have as follow up is to cache > downloaded URLs. They are already cached in URL cache, but I believe > that they should be also cached in some place like world collection or > something like that. I'm not sure what would be appropriate. Do you have > a suggestion? > Yes. I suspect the most relevant bug is actually 27168. 28004 is probably a dupe of it, except that I'm not certain that PartyPoker uses chromium. In brief, Microsoft's crypt32 appears to cache intermediate certs in a PCCERT_CONTEXT's hCertStore. What I should be doing in crypt32, but am not, is to have certs ref count the stores they're in, such that the last cert to be closed causes its store to be freed, if the store is closed prior to the cert being freed. This, combined with certificate chains being cached in Microsoft's implementation, would allow intermediate certs to be found without explicitly checking the AIA. You can see how the certificate chain engine ought to support caching on MSDN: http://msdn.microsoft.com/en-us/library/windows/desktop/aa377184(v=vs.85).a… I never implemented chain caching, as I expected it'd be a performance optimization and could be addressed at a later time, and never got back to it. In addition to bug 27168, you can see how Chromium is making use of crypt32's certificate caching in net::X509Certificate::CreateOSCertChainForCert, both its comments and implementation: http://src.chromium.org/viewvc/chrome/trunk/src/net/cert/x509_certificate.h http://src.chromium.org/viewvc/chrome/trunk/src/net/cert/x509_certificate_w… Back to your patch: I'm not entirely opposed to the explicit AIA approach, but I think it's worth at least thinking about other methods. And, I'd prefer to see the code block more explicitly separated from the surrounding code, with either a comment or a clear function name to indicate what's going on. As it is, one has to read quite a bit of CRYPT_FindIssuer to realize that it's really looking for an AIA extension to find intermediate issuers. Last thing on the patch, some style nits. (You already remarked that it needs splitting, and I agree.) + if(issuer) { Elsewhere in crypt32 I indent prior to open parentheses, so I worry this will make it a little harder to maintain a consistent style. Please observe the existing style, here and elsewhere. - issuer = CertFindCertificateInStore(store, - subject->dwCertEncodingType, 0, CERT_FIND_SUBJECT_NAME, - &subject->pCertInfo->Issuer, prevIssuer); + issuer = CRYPT_FindIssuer(engine, subject, store, CERT_FIND_SUBJECT_NAME, + &subject->pCertInfo->Issuer, flags, prevIssuer); It might not be easy to guess my indenting style for continuation lines, but I use a single additional space to indicate continuation through crypt32. I'd appreciate observing the same style. - hAdditionalStore, &chain); + hAdditionalStore, dwFlags, &chain); Likewise, the indentation change here doesn't seem necessary. Thanks very much for working on this. Hope my comments are helpful, let me know if I can be of further assistance. --Juan

2 1

Re: ntdll/tests: Add tests for job objects. (try 2)
by Frédéric Delanoy 06 Sep '13

06 Sep '13

On Fri, Sep 6, 2013 at 5:41 AM, Andrew Cook <ariscop(a)gmail.com> wrote: > --- > dlls/ntdll/tests/Makefile.in | 1 + > dlls/ntdll/tests/job.c | 151 > +++++++++++++++++++++++++++++++++++++++++++ > include/winnt.h | 5 ++ > 3 files changed, 157 insertions(+) > create mode 100644 dlls/ntdll/tests/job.c + if(pNtIsProcessInJob) + todo_wine todo_wine ok(pNtIsProcessInJob(pi[0].hProcess, JobObject) == STATUS_PROCESS_NOT_IN_JOB, + "NtIsProcessInJob: expected STATUS_PROCESS_NOT_IN_JOB, got %x\n", ret); Is that double todo_wine intended? It seems weird... -- Frédéric Delanoy

2 1

Re: [PATCH 1/3] riched20: Use codepage in ME_ToUnicode. (try 2)
by Nikolay Sivov 06 Sep '13

06 Sep '13

On 9/4/2013 07:17, Jactry Zeng wrote: > - ME_EndToUnicode(unicode, wszText); > + ME_EndToUnicode(unicode ? 1200 : CP_ACP, wszText); It's still ugly to use magic numbers like that, in a similar situation I had some time ago I use ~0 to mark WCHAR data encoding that does need special handling. Even with a value like ~0 it needs to be documented in code comments.

2 2

Re: [PATCH 2/3] ws2_32: Implement get socket option SO_PROTOCOL_INFO (try 3)
by Marvin 06 Sep '13

06 Sep '13

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://newtestbot.winehq.org/JobDetails.pl?Key=2035 Your paranoid android. === wxppro (32 bit sock) === sock.c:519: Test failed: oob_server (b8): unexpectedly at the OOB mark: 0 sock: unhandled exception c0000005 at 00000000

1 0