February 2011 - Wine-Devel - list.winehq.org

Re: Wine FAQ edits

by Vitaliy Margolen

On 02/01/2011 09:47 PM, Albert Lee wrote: > On Sat, Jan 29, 2011 at 4:36 PM, Vitaliy<vitaliy(a)kievinfo.com> wrote: >>> -cd ~/.wine/drive_c/Program\ Files >>> +cd .wine/drive_c/Program\ Files >> >> This will break if current directory is not $HOME. >> > > 'cd' immediately precedes this. That still doesn't make the command valid. It won't work from any directory, that's the point. >>> -/bin/sh -c glxgears >>> +/usr/bin/env glxgears >> >> What advantage does "env" has over full featured shell? This is unnecessary. > > Why do you need a full shell to run a single external program? It > complicates e.g. argument passing if you used this as a template. Why do you don't want a full featured shell? What of that program is a shell script? >>> -@="/bin/sh run_linx_program acroread \"%1\"" >>> +@="/bin/sh -c \"acroread `wine winepath -u '%1'`\"" >> >> Have you actually tested this? This won't work for paths with spaces. If it >> will work at all. >> > > Not with spaces, I'll fix this as well. You still failed to test if it works. What "supposed to work" in shell doesn't mean it works in Wine's process exec. So please test your suggestions first. With Wine. And don't write shell scripts inside registry. That won't work or do what you think it "should". Vitaliy.

Feb. 2, 2011

Re: Wine FAQ edits

by Albert Lee

On Sat, Jan 29, 2011 at 4:36 PM, Vitaliy <vitaliy(a)kievinfo.com> wrote: > On 01/29/2011 12:21 PM, Dan Kegel wrote: >> >> Hi Albert, >> http://wiki.winehq.org/FAQ?action=diff&rev2=384&rev1=383 looks >> like it was a pretty big edit. >> >> Try smaller ones, and try to do just one kind of change per edit. > > That, and the fact that some changes where questionable, and broken: > >> -cd ~/.wine/drive_c/Program\ Files >> +cd .wine/drive_c/Program\ Files > > This will break if current directory is not $HOME. > 'cd' immediately precedes this. >> - You can also use double quotes, but you need two backslashes instead of >> one: >> - >> -{{{ >> -wine start "C:\\Games\\Tron\\tron.exe" >> -}}} > > Backslashes still have special meaning inside double quotes. So they should > be escaped. Yes, I'll fix this. > >> -/bin/sh -c glxgears >> +/usr/bin/env glxgears > > What advantage does "env" has over full featured shell? This is unnecessary. Why do you need a full shell to run a single external program? It complicates e.g. argument passing if you used this as a template. > >> -@="/bin/sh run_linx_program acroread \"%1\"" >> +@="/bin/sh -c \"acroread `wine winepath -u '%1'`\"" > > Have you actually tested this? This won't work for paths with spaces. If it > will work at all. > Not with spaces, I'll fix this as well. -Albert

Feb. 2, 2011

RE: Correction to crash inside RtlCaptureStackBackTrace() + test case

by Janne Hakonen

> > It doesn't need a try-except block, just better wrap-around handling. > Ah, you mean to enhance the if-check? Well..., to me it seems that there is a problem with the 2nd check: "(void *)(frame + 1) >= NtCurrentTeb()->Tib.StackBase" If frame is 0xffffffff or close to it (e.g. 0xfffffffc), adding 1 to its address makes it overflow and the left side becomes 0x3, or 0x0 in 0xfffffffc's case. And that would make the check fail. I would propose that this would be corrected with an additional overflow check. There are two 4 byte values that needs to be read: next frame address and return address. So, it would be checked that address of last byte of return address (frame + 7 bytes) is not smaller than current frame address, right? The if-check would then be as follows: if (((void *)frame < NtCurrentTeb()->Tib.StackLimit) || ((void *)(frame + 1) >= NtCurrentTeb()->Tib.StackBase) || ((ULONG_PTR)frame & 3) || ((ULONG)((char *)frame + 7) < (ULONG)frame)) ... I'm not quite sure if casting pointers to ULONG for comparison is wise move, but can't think anything else for that. Do you think there is any problems with this approach? Other alternative could be to just simply check if the frame pointer equals to 0xfffffffc, that, combined with "(ULONG_PTR)frame & 3" should handle all cases near 0xffffffff. Best Regards, Janne

Feb. 1, 2011

Re: crypt32: Fix quoting-behaviour

by Juan Lang

Hi Christian, I'm afraid this patch is incorrect. In particular, - chars = CertRDNValueToStrA( - rdn->rgRDNAttr[j].dwValueType, - &rdn->rgRDNAttr[j].Value, psz ? psz + ret : NULL, - psz ? csz - ret : 0); + if ((dwStrType & 0x000000ff) == CERT_X500_NAME_STR) + chars = quote_string_a( + rdn->rgRDNAttr[j].dwValueType, + &rdn->rgRDNAttr[j].Value, psz ? psz + ret : NULL, + psz ? csz - ret : 0); + else + chars = CertRDNValueToStrA( + rdn->rgRDNAttr[j].dwValueType, + &rdn->rgRDNAttr[j].Value, psz ? psz + ret : NULL, + psz ? csz - ret : 0); This change (and the corresponding one to CertNameToStrW) is incorrect. I tested a couple of strings with CERT_SIMPLE_NAME_STR, and they are also quoted. I can try to take these over if you like, or you can continue to work on them. Let me know what you prefer. --Juan

Feb. 1, 2011

Re: RFC: Adding Mac support to secur32/schannel.c

by Henri Verbeet

On 1 February 2011 17:19, Juan Lang <juan.lang(a)gmail.com> wrote: >> what IMO complicates writing them is that only the client part of >> schannel is currently implemented. > > That might be true for writing tests against Wine's implementation, > but there's nothing to stop them from being skipped if a server > implementation isn't available. In general, I write tests against > Windows first. How is this case different? Furthermore, adding a > server implementation to schannel isn't likely to be that complex, as > GnuTLS does support server-side connections as well, so if that's > what's holding back tests, it shouldn't be. > I wrote some tests when doing the initial implementation, but those were standalone, running against a real https server running locally. I suppose I could check if I still have them somewhere and send them to you, but they would be fairly useless for preventing regressions as long as they're not part of the Wine tests, and are actually run. >> Not really. IMO it's just a case of neglect. > > Agreed that it is neglected. Without tests, I'm nervous to take on > ownership of it. There have been a few half-hearted attempts to work > in this area, but I haven't seen much contribution from people willing > to maintain the code over the long term. > Yeah, but that's hardly specific to schannel. > I may be flogging a dead horse here, but I personally am loath to see > another implementation creep in, side by side with the existing one, > that has no guarantee of working any better. I don't see how this > helps CodeWeavers, either, other than reducing installation > complexity. If there are bugs in the new implementation, and I expect > there will be, you'll still have a large support load. Worse, even if > you succeed in fixing bugs for your Mac customers, the rest of us > don't benefit, as the current implementation still isn't getting any > support. If there are development resources available to work on > schannel, why not put them into something that benefits the project as > a whole? Note that in general I agree with your position, I'm just trying to clarify some of the reasons of why the current situation is the way it is. I think it would be a good thing for CodeWeavers to spend some time on Schannel by adding server support and tests, but it's not my decision to make. If it was something I'd have to do on my own time, I'm simply not going to have the time any time soon though.

Feb. 1, 2011

Re: [dlls/shdocvw] Initialize variable (Coverity)

by Nikolay Sivov

On 2/1/2011 19:21, Paul Vriens wrote: > On 02/01/2011 03:26 PM, Nikolay Sivov wrote: >> On 2/1/2011 17:14, Paul Vriens wrote: >>> Hi, >>> >>> Addresses Coverity #1090. >>> >>> If CoCreateInstance() fails we check the uninitialized urlfile >>> variable. >> Same story with urlobj. In fact it looks better to try CoCreateInstance >> first, before anything else like lstrcpy and just return earlier on >> failure. >> >> > > Isn't urlobj, NULL'ed by CoCreateInstance on failure? > You're right, it should null it. We don't have a test for that by the way (that it's nulled on unregistered class failure). So this could be left as it is.

Feb. 1, 2011

Re: [dlls/shdocvw] Initialize variable (Coverity)

by Paul Vriens

On 02/01/2011 03:26 PM, Nikolay Sivov wrote: > On 2/1/2011 17:14, Paul Vriens wrote: >> Hi, >> >> Addresses Coverity #1090. >> >> If CoCreateInstance() fails we check the uninitialized urlfile variable. > Same story with urlobj. In fact it looks better to try CoCreateInstance > first, before anything else like lstrcpy and just return earlier on > failure. > > Isn't urlobj, NULL'ed by CoCreateInstance on failure? -- Cheers, Paul.

Feb. 1, 2011

Re: RFC: Adding Mac support to secur32/schannel.c

by Juan Lang

> Well, I think that regardless of what schannel ends up using, wininet > and winhttp should be implemented on top schannel in the long term, > instead of using OpenSSL directly. I don't think GnuTLS is really the Well, that's certainly true, as there are features of at least wininet that can't be implemented as long as OpenSSL is using a file descriptor directly. I don't have enough confidence in schannel's current implementation to start that, though. > problem though, or that the existing schannel code is particularly > badly implemented. It seems to me that it's more a case of the > schannel / secur32 API being somewhat unclear, even to the > applications actually using it. Tests would certainly help there, but Perhaps. Tests would help convince me. > what IMO complicates writing them is that only the client part of > schannel is currently implemented. That might be true for writing tests against Wine's implementation, but there's nothing to stop them from being skipped if a server implementation isn't available. In general, I write tests against Windows first. How is this case different? Furthermore, adding a server implementation to schannel isn't likely to be that complex, as GnuTLS does support server-side connections as well, so if that's what's holding back tests, it shouldn't be. > Not really. IMO it's just a case of neglect. Agreed that it is neglected. Without tests, I'm nervous to take on ownership of it. There have been a few half-hearted attempts to work in this area, but I haven't seen much contribution from people willing to maintain the code over the long term. I may be flogging a dead horse here, but I personally am loath to see another implementation creep in, side by side with the existing one, that has no guarantee of working any better. I don't see how this helps CodeWeavers, either, other than reducing installation complexity. If there are bugs in the new implementation, and I expect there will be, you'll still have a large support load. Worse, even if you succeed in fixing bugs for your Mac customers, the rest of us don't benefit, as the current implementation still isn't getting any support. If there are development resources available to work on schannel, why not put them into something that benefits the project as a whole? --Juan

Feb. 1, 2011

Re: [dlls/shdocvw] Initialize variable (Coverity)

by Nikolay Sivov

On 2/1/2011 17:14, Paul Vriens wrote: > Hi, > > Addresses Coverity #1090. > > If CoCreateInstance() fails we check the uninitialized urlfile variable. Same story with urlobj. In fact it looks better to try CoCreateInstance first, before anything else like lstrcpy and just return earlier on failure.

Feb. 1, 2011

Re: gdiplus: C++ fixes for headers

by Iain Arnell

On Tue, Feb 1, 2011 at 10:28 AM, Paul Chitescu <paulc(a)voip.null.ro> wrote: > On Tuesday 01 February 2011 09:49:46 am Iain Arnell wrote: >> To fix inclusion of gdiplus.h under C++, this patch: >> * adds missing class declaration for GpPen >> * adds missing class declaration for CharacterRange >> * fixes PenAlignment/GpPenAlignment declaration >> * avoids redeclaration of PixelFormat [snip the patch] > > Wouldn't be better to wrap the offending C with: > > #ifdef __cplusplus > extern "C" { > #endif > > ... > > #ifdef __cplusplus > } > #endif > It's not a matter of offending C code; just that the C++ in these headers has bitrotted over the years as the code has changed. -- Iain.

Feb. 1, 2011