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

Re: programs: Add winemsibuilder.

by Joris Huizer

While skimming through the new patches, I found: > +WCHAR *encode_stream( const WCHAR *in ) > +{ > + DWORD c, next, count; > + WCHAR *out, *p; > + > + count = strlenW( in ); > + if (count > MAX_STREAM_NAME) > + return NULL; > + > + count += 2; > + out = HeapAlloc( GetProcessHeap(), 0, count > * sizeof(WCHAR) ); > + p = out; You aren't checking for out-of-memory condition here. As you seem to cover all the other possible errors, I thought to let you know HTH, Joris

Feb. 1, 2011

Re: [PATCH 1/5] shell32: Added SHCNRF_InterruptLevel flag support to SHChangeNotifyRegister

by Alexandre Julliard

Piotr Caban <piotr(a)codeweavers.com> writes: > void InitChangeNotifications(void) > { > + watched_directories = 0; > + interrupts_server_terminate = CreateEventW(NULL, FALSE, FALSE, NULL); > + interrupts_server = CreateThread(NULL, 0, interrupts_server_thread, NULL, 0, NULL); > } This needs to be done on demand. You can't create a thread in every process on the off chance that it would use interrupt notifications. Probably you can use the thread pool for this. -- Alexandre Julliard julliard(a)winehq.org

Feb. 1, 2011

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

by Henri Verbeet

On 1 February 2011 02:08, Juan Lang <juan.lang(a)gmail.com> wrote: > Sure, I can buy that. I'll note that OpenSSL is also available for > the Mac, and already loaded by wininet and winhttp. It could be > appropriate to move from GnuTLS to OpenSSL for schannel, so we'd only > have a single implementation for both Linux and Mac in schannel. > 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 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 what IMO complicates writing them is that only the client part of schannel is currently implemented. >> Well, it doesn't help make schannel less buggy, but it doesn't aim to. However, it does help Macs without GnuTLS (the default) go from a completely non-functional schannel to a merely buggy schannel. > > I suppose that argument is also why we got the buggy (sorry, Henri) > GnuTLS schannel in the first place. Not really. IMO it's just a case of neglect. For what it's worth, at some point the plan at CodeWeavers was that Hans would do some work on schannel, but I assume msi bugs took priority there.

Feb. 1, 2011

Re: gdiplus: C++ fixes for headers

by Paul Chitescu

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 > > Simple testcase: > > cat >test.cpp <<EOF > #include <windows.h> > #include <gdiplus.h> > EOF > wineg++ -c test.cpp > --- > include/gdiplusenums.h | 2 +- > include/gdiplusgpstubs.h | 2 ++ > include/gdiplusimaging.h | 4 ++-- > include/gdiplustypes.h | 25 +++++++++++++++++++++++++ > 4 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/include/gdiplusenums.h b/include/gdiplusenums.h > index 82f20eb..dc03b29 100644 > --- a/include/gdiplusenums.h > +++ b/include/gdiplusenums.h > @@ -734,7 +734,7 @@ typedef enum StringTrimming StringTrimming; > typedef enum FontStyle FontStyle; > typedef enum StringFormatFlags StringFormatFlags; > typedef enum HotkeyPrefix HotkeyPrefix; > -typedef enum PenAlignment GpPenAlignment; > +typedef enum PenAlignment PenAlignment; > typedef enum PaletteFlags PaletteFlags; > typedef enum ImageCodecFlags ImageCodecFlags; > typedef enum CombineMode CombineMode; > diff --git a/include/gdiplusgpstubs.h b/include/gdiplusgpstubs.h > index 3edc1f4..5a65cd8 100644 > --- a/include/gdiplusgpstubs.h > +++ b/include/gdiplusgpstubs.h > @@ -22,6 +22,7 @@ > #ifdef __cplusplus > > class GpGraphics {}; > +class GpPen {}; > class GpBrush {}; > class GpHatch : public GpBrush {}; > class GpSolidFill : public GpBrush {}; > @@ -92,6 +93,7 @@ typedef WrapMode GpWrapMode; > typedef Color GpColor; > typedef FlushIntention GpFlushIntention; > typedef CoordinateSpace GpCoordinateSpace; > +typedef PenAlignment GpPenAlignment; > typedef PenType GpPenType; > > #endif > diff --git a/include/gdiplusimaging.h b/include/gdiplusimaging.h > index 42a00cb..3e9f6ee 100644 > --- a/include/gdiplusimaging.h > +++ b/include/gdiplusimaging.h > @@ -110,7 +110,7 @@ public: > UINT Width; > UINT Height; > INT Stride; > - PixelFormat PixelFormat; > + INT PixelFormat; /* MSDN: "PixelFormat PixelFormat;" */ > VOID* Scan0; > UINT_PTR Reserved; > }; > @@ -177,7 +177,7 @@ typedef struct BitmapData > UINT Width; > UINT Height; > INT Stride; > - PixelFormat PixelFormat; > + INT PixelFormat; /* MSDN: "PixelFormat PixelFormat;" */ > VOID* Scan0; > UINT_PTR Reserved; /* undocumented: stores the lock mode */ > } BitmapData; > diff --git a/include/gdiplustypes.h b/include/gdiplustypes.h > index 4f97432..b84c74e 100644 > --- a/include/gdiplustypes.h > +++ b/include/gdiplustypes.h > @@ -199,6 +199,31 @@ public: > INT Height; > }; > > +class CharacterRange > +{ > +public: > + CharacterRange() > + { > + First = Length = 0; > + } > + > + CharacterRange(IN INT first, IN INT length) > + { > + First = first; > + Length = length; > + } > + > + CharacterRange& operator=(IN const CharacterRange& rhs) > + { > + First = rhs.First; > + Length = rhs.Length; > + return *this; > + } > +public: > + INT First; > + INT Length; > +}; > + > #else /* end of c++ typedefs */ > > typedef struct Point Wouldn't be better to wrap the offending C with: #ifdef __cplusplus extern "C" { #endif ... #ifdef __cplusplus } #endif

Feb. 1, 2011

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

by Damjan Jovanovic

On Tue, Feb 1, 2011 at 3:08 AM, Juan Lang <juan.lang(a)gmail.com> wrote: > Hi Ken, thanks for the reply. > > > As Henri said, it's that it's a set of external dependencies (not just > one; GnuTLS has its own dependencies) and that they are security-related. > To the greatest extent practical, security-related libraries should come > from one's distro or OS vendor. > > Sure, I can buy that. I'll note that OpenSSL is also available for > the Mac, and already loaded by wininet and winhttp. It could be > appropriate to move from GnuTLS to OpenSSL for schannel, so we'd only > have a single implementation for both Linux and Mac in schannel. > > OpenSSL seems like a bad idea. It has poor binary compatibility and problematic FIPS 140 certification, and Fedora is dropping it in favour of NSS: http://fedoraproject.org/wiki/FedoraCryptoConsolidation http://fedoraproject.org/wiki/CryptoConsolidationEval OpenSSL isn't part of the LSB (while NSS is), so if we ever want to make a Wine LSB package, it might be a good idea to get OpenSSL out of Wine entirely. See also the August 2008 wine-devel thread about this: http://www.winehq.org/pipermail/wine-devel/2008-August/068575.html Damjan Jovanovic

Feb. 1, 2011

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

by Juan Lang

Hi Ken, thanks for the reply. > As Henri said, it's that it's a set of external dependencies (not just one; GnuTLS has its own dependencies) and that they are security-related. To the greatest extent practical, security-related libraries should come from one's distro or OS vendor. Sure, I can buy that. I'll note that OpenSSL is also available for the Mac, and already loaded by wininet and winhttp. It could be appropriate to move from GnuTLS to OpenSSL for schannel, so we'd only have a single implementation for both Linux and Mac in schannel. > Well, it doesn't help make schannel less buggy, but it doesn't aim to. However, it does help Macs without GnuTLS (the default) go from a completely non-functional schannel to a merely buggy schannel. I suppose that argument is also why we got the buggy (sorry, Henri) GnuTLS schannel in the first place. But adding GnuTLS to a Mac also achieves the same thing, without adding additional code. I agree it isn't perfect, but I'm not sure the additional code path is worth it, at least until we understand where the bugs are. See below. > Well, if schannel were under active development or were soon to be, then I'd hold off on adding a second implementation. However, that doesn't seem to be the case. Adding another implementation probably doesn't help. As I see it, we have a lack of expertise in the area, and no one who's willing to maintain the code there. I don't see how this helps with either problem. > As to making debugging harder, I think the impact is pretty minor. The internal schan_imp_* interface is pretty close to GnuTLS. There's not much of the schannel logic -- the stuff that's potentially buggy -- that ends up behind that interface. It mostly consists of making the GnuTLS call, translating its results to Win32 error codes, and logging errors with gnutls_perror(). Frankly, in several respects, I think that moving some of the GnuTLS-using code into smaller, more narrowly focused functions improved the code quite aside from enabling a second implementation. Well, if the interface to GnuTLS could be improved, I'd be happy to see a patch series that did just that. That would benefit everyone. > Admittedly, as schannel is developed, the internal interface may need to grow. And having a second implementation does increase the inertia for that. But that's different from debugging. And, again, schannel is currently pretty static, so this is a hypothetical concern for an unknown time in the future. I don't see that it is is hypothetical. There are many secur32/schannel bugs already, and the fact that secur32 isn't changing doesn't help them move forward. Having the code change only complicates things, as I see it. > > To my mind it seems that, if having a Mac-specific implementation is ever going to be accepted, accepting it now is better than waiting indefinitely until the GnuTLS-based implementation is perfected and then revamping the by-then-even-more-GnuTLS-centric design to accommodate a second implementation. That does assume that the GnuTLS implementation is the one that remains. I'm not entirely convinced this will end up being appropriate. It's not clear to me that shimming GnuTLS into SSPI will ever address the strange SecBuffer requirements apps seem to have. It's not that I have clear evidence that this approach is doomed to failure, but my experience looking at schannel-related bugs doesn't give me good feelings about it. So, what I asked for before, and what I still see as lacking, is a set of schannel tests. I'd love to see some that identified deficiencies in our current implementation. The ideal outcome, for me, isn't that we fix all the open schannel bugs without writing additional tests. Doing so doesn't help anyone else address future problems, or help ensure that moving away from GnuTLS (on the Mac or elsewhere) doesn't introduce new problems. I'd much rather see a strong enough set of tests that we have some idea of what the problems are. This would help with the expertise and maintenance problems. Expertise, both because whoever writes tests gains some, and because whoever reads them can as well. Maintenance, because tests help prevent regressions, or help prevent a new implementation from being written incorrectly in the first place. Since Henri was kind enough to accept some responsibility for the schannel implementation being buggy, I'll confess that I've wanted to write a good set of schannel tests for years now. I even began, but never got very far. Sadly, life sometimes interferes. I hope that helps. --Juan

Feb. 1, 2011

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

by Ken Thomases

Hi Juan, On Jan 28, 2011, at 10:36 AM, Juan Lang wrote: >> I'm planning to add an alternative implementation of schannel (SSL/TLS) support for the Mac. The current implementation is based on GnuTLS. That library is not typically found on Mac OS X. Although packagers can build it and ship it and its dependencies with Wine for Mac OS X, I think it's better (especially for security-related functionality) to use the system-provided library. > > What's the issue with building GnuTLS? Is it that GnuTLS doesn't > support the Mac Keychain? Is it that it's an external dependency? As Henri said, it's that it's a set of external dependencies (not just one; GnuTLS has its own dependencies) and that they are security-related. To the greatest extent practical, security-related libraries should come from one's distro or OS vendor. > If the latter, we already pull in quite a bit that isn't found on the > Mac, so the incremental change isn't large. Well, I'd prefer to reduce those other external dependencies, too. But that's a fight for another day. ;) > One minor nit: s/adaptor/adapter/ Well, my dictionary shows both as correct, although "adapter" is apparently preferred in American English. Thanks. >> I plan to introduce a new internal interface (the schan_imp_* stuff in my patch) and incrementally refactor the code to hide uses of GnuTLS behind that interface. Then, I'm thinking of breaking the GnuTLS implementation out into a separate module, schannel_gnutls.c. Then, I'd add a second implementation module, schannel_mac.c, based on the Mac Secure Transport API (as shown in the patch). Each of the two modules would be made "empty" by preprocessor conditionals, as appropriate. >> >> I'd appreciate a review of this general plan. > > I do see two problems with the general plan. One isn't specific to > your plan: schannel as it is is buggy. We don't know where the bugs > are, and they've languished for a long time. Your proposed plan > doesn't help. Well, it doesn't help make schannel less buggy, but it doesn't aim to. However, it does help Macs without GnuTLS (the default) go from a completely non-functional schannel to a merely buggy schannel. > The second is the additional bug triaging complexity. > > I'd much prefer to see a decent set of test cases for schannel. I > suppose one could argue that that's orthogonal to this plan, but see > my second complaint: without test cases, an additional implementation > just makes debugging schannel bugs harder. Well, if schannel were under active development or were soon to be, then I'd hold off on adding a second implementation. However, that doesn't seem to be the case. As to making debugging harder, I think the impact is pretty minor. The internal schan_imp_* interface is pretty close to GnuTLS. There's not much of the schannel logic -- the stuff that's potentially buggy -- that ends up behind that interface. It mostly consists of making the GnuTLS call, translating its results to Win32 error codes, and logging errors with gnutls_perror(). Frankly, in several respects, I think that moving some of the GnuTLS-using code into smaller, more narrowly focused functions improved the code quite aside from enabling a second implementation. Admittedly, as schannel is developed, the internal interface may need to grow. And having a second implementation does increase the inertia for that. But that's different from debugging. And, again, schannel is currently pretty static, so this is a hypothetical concern for an unknown time in the future. To my mind it seems that, if having a Mac-specific implementation is ever going to be accepted, accepting it now is better than waiting indefinitely until the GnuTLS-based implementation is perfected and then revamping the by-then-even-more-GnuTLS-centric design to accommodate a second implementation. Thanks for your feedback. Cheers, Ken

Feb. 1, 2011