Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)
On Sun, Sep 29, 2013 at 11:10 PM, Daniel Jeliński djelins...@gmail.comwrote:
2013/9/30 Nikolay Sivov bungleh...@gmail.com
On 9/30/2013 00:51, Daniel Jeliński wrote:
+struct progress_list {
+const DWORD progress_retval_init; /* value to return from progress
routine */
+const BOOL cancel_init;/* value to set Cancel flag to */
+const DWORD progress_retval_end; /* value to return from progress
routine */
+const BOOL cancel_end; /* value to set Cancel flag to */
+const DWORD progress_count;/* number of times progress is
invoked */
+const BOOL copy_retval;/* expected CopyFileEx result */
+const DWORD lastError; /* expected CopyFileEx error
code */
+} ;
I don't see a point making them 'const'.
I'm matching the formatting of existing code:
http://source.winehq.org/source/dlls/kernel32/tests/file.c#L65
Also, what's the point of not making them const?
It's a little strange, stylewise. More consistent with C++ style would be
to make the entire struct constant. But in a test, we often eschew with
such things if they distract, and here I think they do.
+static DWORD WINAPI progress(LARGE_INTEGER TotalFileSize,
+LARGE_INTEGER TotalBytesTransferred,
+LARGE_INTEGER StreamSize,
+LARGE_INTEGER StreamBytesTransferred,
+DWORD dwStreamNumber,
+DWORD dwCallbackReason,
+HANDLE hSourceFile,
+HANDLE hDestinationFile,
+LPVOID lpData)
+{
+progressInvoked++;
Please pass all globals as context data with lpData, and please use
'void*' instead of LPVOID.
Good point about lpData. Still, does that make the patch invalid? Why
didn't you mention that on the first review?
About LPVOID - I'm matching the headers:
http://source.winehq.org/source/include/winbase.h#L910
for the third patch:
http://source.winehq.org/source/include/winbase.h#L1018
LPVOID is just an uglier #define of void*. It's easier to read as void*, so
please use that instead.
--Juan
Re: crypt32: Support HCCE_LOCAL_MACHINE.
Hi Ben, thanks for having a whack at this.
Some tests would be nice.
-static HCERTCHAINENGINE CRYPT_defaultChainEngine;
+/* There are two default chain engines which correspond to
HCCE_CURRENT_USER and
+ * HCCE_LOCAL_MACHINE.
+*/
+static HCERTCHAINENGINE CRYPT_defaultChainEngine[2] = { NULL, NULL };
C automatically initializes statics to 0, so the initialization here is
unnecessary. I'm also a little uncertain about the use of an array of two
objects, I'm not sure that two distinct objects wouldn't be easier to
follow, but I'm not religious on this point.
+ if (hChainEngine HCCE_LOCAL_MACHINE
+ InterlockedDecrement(engine-ref) == 0)
I think a function that returns whether an HCERTCHAINENGINE is one of the
special ones would make this easier to read, e.g.:
static int is_special_chain_engine(HCERTCHAINENGINE h)
{
return h == HCCE_CURRENT_USER || h == HCCE_LOCAL_MACHINE;
}
then:
+ if (is_special_chain_engine(hChainEngine)
+ InterlockedDecrement(engine-ref) == 0)
+static HCERTCHAINENGINE CRYPT_GetDefaultChainEngine(HCERTCHAINENGINE h)
{
-if (!CRYPT_defaultChainEngine)
+if (!CRYPT_defaultChainEngine[(int)h])
The constant casting is a little awkward. At least introduce a local
pointer to the one you intend to modify, so we're not constantly having to
re-read that cast.
+if (hChainEngine = HCCE_LOCAL_MACHINE)
+engine = (CertificateChainEngine*)CRYPT_GetDefaultChainEngine(
+ hChainEngine);
if (TRACE_ON(chain))
dump_chain_para(pChainPara);
/* FIXME: what about HCCE_LOCAL_MACHINE? */
See my earlier suggestion on is_special_chain_engine. Also, what about that
comment three lines down? Can't it be removed?
-void default_chain_engine_free(void)
+void default_chain_engine_free(HCERTCHAINENGINE h)
{
-CertFreeCertificateChainEngine(CRYPT_defaultChainEngine);
+CertFreeCertificateChainEngine(CRYPT_defaultChainEngine[(int)h]);
The function default_chain_engine_free is a thin wrapper around
CryptFreeCertificateChainEngine; its intended use is to clear memory at
shutdown. Rather than shift the responsibility of knowing which engines to
free to the caller, just have it free the two engines itself.
Thanks,
--Juan
Re: RFC: HKCR merge implementation
Hi George, On Tue, Sep 10, 2013 at 3:55 PM, George Stephanos gaf.stepha...@gmail.comwrote: I'm proposing my HKEY_CLASSES_ROOT implementation patches for review. Feel free to comment. So far, I've written code for all functions except for the RegEnum family. General description: HKCR handles are special. All wine handles have the two lowest bits zero'd. HKCR handles are masked. 10b specifically. They have their own table separate from the generic handle table. An HKCR handle has an HKCU handle and an HKLM handle below it. Access mask and a string representing the path are also needed since the handle has to attempt to reopen previously failed openings at certain stages. First patch: specially handles HKCR handles WITHOUT affecting the general behavior. The end result is still the exact same. Patch provides a foundation for the rest. Second patch: added path management Third patch: added HKCU Here's a quick description of each function: create_hkcr_struct: allocates the memory needed for the struct, adds it to the table and gives back a pointer to it get_hkcr_path: given an HKCR handle and a subkey string, prepares a subkey string representing the same subkey rooted at HKLM/HKCU. create_hkcr: RegCreateKeyEx open_hkcr: RegOpenKeyEx resolve_hkcr: checks the HKCR handle, tries to reopen previously failed internal handles, gives back HKCU first if available then HKLM close_hkcr: deallocates path and struct, removes struct from table. http://pastie.org/8314658 a couple bitwise nits in your first patch: +#define HKCR_MASK 2 +#define IS_HKCR(hk) ((UINT_PTR)hk 0 ((UINT_PTR)hk 3) == HKCR_MASK) Typically a mask would define all the bits that could be set, and a flag would be the particular bit you want to test. Something like: #define SPECIAL_HKEY_MASK 3 #define HKCR_FLAG 2 Also, in the following expression: ((UINT_PTR)hk 0 ((UINT_PTR)hk 3) The second can never be true when the first is false, so you can just drop the first. These suggestions yield: #define IS_HKCR(hk) (((UINT_PTR)(hk) SPECIAL_HKEY_MASK) == HKCR_FLAG) --Juan
Re: Patch for bug 34388
On Fri, Sep 6, 2013 at 9:17 PM, Charles Davis cdavi...@gmail.com wrote: On Sep 6, 2013, at 5:01 PM, Juan Lang wrote: On Fri, Sep 6, 2013 at 3:54 PM, Charles Davis cdavi...@gmail.com wrote: Maybe then the real fix is to make Wine accept either a constructor SET or the custom tag (ASN_CONTEXT | ASN_CONSTRUCTOR) it currently accepts, for either attribute set. I should come up with a test case first, though, to see if that's what Windows does. I'll get back to you on that. Yeah, that seems plausible, as either some sort of BER/DER thing or just two alternate encodings for the same value. I'm not certain, but tests will definitely help. So much for that theory. I tried twice to replace the CONSTRUCTOR | CONTEXT tag with the generic CONSTRUCTOR | SET tag (jobs 2057 and 2058 on newtestbot). Both times, Crypto bailed out. I don't think Windows will accept a CONSTRUCTOR | SET tag there under any circumstances. I think we're seriously screwing up somewhere reading the code signature. Trouble is, I don't know where, or if this is even a problem in Wine at all--it might be peculiar to my system. (The other Wine users who reported being unable to run the Star Citizen Launcher because of this were missing a root CA.) If you've got the signature that was failing, please keep it handy, and we can make it into a test case. If not, keep an eye on it to see if it recurs. What I've done in the past is to modify crypt32 to write a failing cert, sig, whatever to /tmp, and made a test case out of it. I don't know why, but strangely, the problem with the Launcher that prompted my patch seems to have gone away (at least, on my system) with the update they just released. Maybe it's related to the problems a few people were having with this on Windows. Maybe the signature really was malformed. That wouldn't be unheard of. I've forgotten which game it was, but at least once some game server was sending a malformed signature that happened to work on some broken Windows systems. IIRC, they ended up fixing it once a Windows service pack started rejecting it. --Juan
Re: Patch for bug 34388
Hiya Chip, I don't have an answer off the top of my head. But let's see if we can unpack that ASN.1 a bit. First, with the type, notice that crypt32_private.h defines a few useful types that Microsoft omitted for some reason from their public header, including: #define ASN_SETOF (ASN_UNIVERSAL | ASN_PRIMITIVE | 0x11) Okay, so that's what we're dealing with. Next, to help sort it out: write each of the blobs to a binary, and interpret it with openssl asn1parse -dump -i -inform DER. Here's the output from PKCSSignerWithUnknownItem31: 0:d=0 hl=4 l= 146 cons: SEQUENCE 4:d=1 hl=2 l= 1 prim: INTEGER :00 7:d=1 hl=2 l= 25 cons: SEQUENCE 9:d=2 hl=2 l= 20 cons: SEQUENCE 11:d=3 hl=2 l= 18 cons:SET 13:d=4 hl=2 l= 16 cons: SEQUENCE 15:d=5 hl=2 l= 3 prim: OBJECT:commonName 20:d=5 hl=2 l= 9 prim: PRINTABLESTRING :Juan Lang 31:d=2 hl=2 l= 1 prim: INTEGER :01 34:d=1 hl=2 l= 6 cons: SEQUENCE 36:d=2 hl=2 l= 2 prim: OBJECT:1.2.3 40:d=2 hl=2 l= 0 prim: NULL 42:d=1 hl=2 l= 96 cons: SET 44:d=2 hl=2 l= 0 prim: EOC (a bunch of these omitted) 140:d=1 hl=2 l= 6 cons: SEQUENCE 142:d=2 hl=2 l= 2 prim: OBJECT:1.5.6 146:d=2 hl=2 l= 0 prim: NULL 148:d=1 hl=2 l= 0 prim: OCTET STRING And here's the output from PKCSSignerWithAuthAttrAndUnknownItem31: 0:d=0 hl=4 l= 196 cons: SEQUENCE 4:d=1 hl=2 l= 1 prim: INTEGER :00 7:d=1 hl=2 l= 25 cons: SEQUENCE 9:d=2 hl=2 l= 20 cons: SEQUENCE 11:d=3 hl=2 l= 18 cons:SET 13:d=4 hl=2 l= 16 cons: SEQUENCE 15:d=5 hl=2 l= 3 prim: OBJECT:commonName 20:d=5 hl=2 l= 9 prim: PRINTABLESTRING :Juan Lang 31:d=2 hl=2 l= 1 prim: INTEGER :01 34:d=1 hl=2 l= 6 cons: SEQUENCE 36:d=2 hl=2 l= 2 prim: OBJECT:1.2.3 40:d=2 hl=2 l= 0 prim: NULL 42:d=1 hl=2 l= 32 cons: cont [ 0 ] 44:d=2 hl=2 l= 30 cons: SEQUENCE 46:d=3 hl=2 l= 3 prim:OBJECT:commonName 51:d=3 hl=2 l= 23 cons:SET 53:d=4 hl=2 l= 21 cons: SEQUENCE 55:d=5 hl=2 l= 19 cons: SET 57:d=6 hl=2 l= 17 cons: SEQUENCE 59:d=7 hl=2 l= 3 prim:OBJECT:commonName 64:d=7 hl=2 l= 10 prim:PRINTABLESTRING :Juan Lang 76:d=1 hl=2 l= 96 cons: SET 78:d=2 hl=2 l= 0 prim: EOC (again, a bunch of these omitted) 174:d=1 hl=2 l= 6 cons: SEQUENCE 176:d=2 hl=2 l= 2 prim: OBJECT:1.5.6 180:d=2 hl=2 l= 0 prim: NULL 182:d=1 hl=2 l= 16 prim: OCTET STRING - 00 01 02 03 04 05 06 07-08 09 0a 0b 0c 0d 0e 0f You can see that in the latter case, you've got a context-specific tag of 0 wrapping a sequence containing a non-empty set. That context-specific [0] looks an awful lot like signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL, no? And SignedAttributes is defined as SignedAttributes ::= SET SIZE (1..MAX) OF Attribute. Anyway, in the latter case, the signed attributes precede the set, and in the former case they're missing. In addition to that, it's not entirely clear from your test results that it's expected to succeed in Windows in either case: +ret = pCryptDecodeObjectEx(dwEncoding, PKCS7_SIGNER_INFO, + PKCSSignerWithAuthAttrAndUnknownItem31, + sizeof(PKCSSignerWithAuthAttrAndUnknownItem31), + CRYPT_DECODE_ALLOC_FLAG, NULL, buf, size); +if (ret) You don't have an ok(ret) here. I can't blame you too much for that, though: it looks like I omitted a bunch of ok(ret)s in my tests too. Sigh. Sorry about that. Could you add that and see what that turns up? --Juan On Thu, Sep 5, 2013 at 11:57 PM, Charles Davis cdavi...@gmail.com wrote: Hi Juan, I need some advice. Attached is a patch to fix bug 34388, where an app that tries to verify its code signature fails because Wine, while parsing the certificates embedded within the signature, encounters an item in the CMS signer info (tag 0x31, i.e. ASN_UNIVERSAL | ASN_CONSTRUCTOR | 0x11), right before the HashEncryptionAlgorithm sequence item, that it doesn't yet know how to decode. This patch (which just skips the item in question) allows that app to successfully verify its code signature and run, but... My testing on Windows (cf. job 2037 on newtestbot) shows some interesting behavior. Windows will indeed accept this item, but only if the AuthAttrs item is also present and immediately precedes it in the sequence. (The other test bails out with CRYPT_E_ASN_BADTAG.) I don't quite know what to make of this. The odd thing is that the certificate in question doesn't have this optional AuthAttrs item, and yet (in most cases, at least) most people who run this particular app on Windows do not have this problem. I can't seem to find anything about this from reading the relevant RFCs
Re: Patch for bug 34388
On Fri, Sep 6, 2013 at 3:54 PM, Charles Davis cdavi...@gmail.com wrote: Maybe then the real fix is to make Wine accept either a constructor SET or the custom tag (ASN_CONTEXT | ASN_CONSTRUCTOR) it currently accepts, for either attribute set. I should come up with a test case first, though, to see if that's what Windows does. I'll get back to you on that. Yeah, that seems plausible, as either some sort of BER/DER thing or just two alternate encodings for the same value. I'm not certain, but tests will definitely help. --Juan
Re: crypt32 patch review
[+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 ja...@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).aspx
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_win.cc
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,
Re: ws2_32/tests: Fix a comment
Hi Andre, -/* this is a io heavy test, do it at the end so the kernel doesn't start dropping packets */ +/* this is a heavy io test, do it at the end so the kernel doesn't start dropping packets */ To my eyes, this isn't an improvement. A slight improvement might be this is a io-heavy test, or this is an io-heavy test, but I'm not sure it's enough of an improvement to bother. --Juan
Re: Possible help or direction to build Win to Linux pass through device - resent
Hi Vincent, Pavel, On Tue, Jul 2, 2013 at 8:44 AM, Vincent Povirk madewokh...@gmail.comwrote: Since you're not prepared to spend a lot of time improving Wine's driver support, it sounds like modifying core parts of Wine specifically to support your application is the best approach. An alternative is to modify your application for use with Wine. What others have suggested in similar situations is to build a Linux application that speaks to the device driver directly, and provides a socket interface, then use sockets from the Windows program to talk to the device. This is usually straightforward enough, and doesn't require learning lots of Wine internals to get working. --Juan
Re: Need help with a rsaenh bug
On Fri, Jun 28, 2013 at 9:16 AM, Qian Hong fract...@gmail.com wrote: Dear Juan, Thanks for reviewing! On Fri, Jun 28, 2013 at 11:31 PM, Juan Lang juan.l...@gmail.com wrote: It's more in line with most C code to use !memcmp(...) instead of memcmp(...)==0. I find it easier to scan, anyway, as I've gotten used to ! comparisons to check equality in memcmp, strcmp, and variants. I'm glad to change, but the surrounding code use memcmp(...)==0 already, should I change that as well? Ah. Thanks for following the existing style then :-/ No, don't bother changing the existing code. I leave it up to you whether to follow my suggestion or ignore it, either is fine in this case. --Juan
Re: [PATCH] Tests that prove ntdll has no notion of HKCR.
Hi George,
static void test_classesroot(void)
{
+static const WCHAR reg_user[] = {
'\\','R','E','G','I','S','T','R','Y','\\','U','S','E','R' };
+static const WCHAR reg_machine[] = {
'\\','R','E','G','I','S','T','R','Y','\\','M','A','C','H','I','N','E' };
Almost, but these have to be NULL terminated, i.e.
+static const WCHAR reg_user[] = {
'\\','R','E','G','I','S','T','R','Y','\\','U','S','E','R',0 };
+static const WCHAR reg_machine[] = {
'\\','R','E','G','I','S','T','R','Y','\\','M','A','C','H','I','N','E',0 };
--Juan
Re: [PATCH] Tests that prove ntdll has no notion of HKCR.
Hi George,
(consider subscribing to wine-devel so your emails don't get stuck in
moderation.)
On Sat, May 11, 2013 at 7:43 AM, George Stephanos
gaf.stepha...@gmail.comwrote:
As instructed, I added a few lines to the already written tests that
confirm my claim.
Part of the research of the registry merging project was to determine
where the implementation is going to be written: advapi32, ntdll or
the server itself. The server choice was dismissed since HKCR isn't
stored and rather fetched live. These tests prove that advapi32 calls
that reference HKCR are either pointed there to \REGISTRY\MACHINE or
\REGISTRY\USER and hence, that the merge is to be done in advapi32.
http://newtestbot.winehq.org/JobDetails.pl?Key=976
---
dlls/advapi32/tests/registry.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/dlls/advapi32/tests/registry.c
b/dlls/advapi32/tests/registry.c
index b2483a7..8437e4d 100644
--- a/dlls/advapi32/tests/registry.c
+++ b/dlls/advapi32/tests/registry.c
@@ -43,6 +43,7 @@ static DWORD (WINAPI *pRegDeleteTreeA)(HKEY,LPCSTR);
static DWORD (WINAPI *pRegDeleteKeyExA)(HKEY,LPCSTR,REGSAM,DWORD);
static BOOL (WINAPI *pIsWow64Process)(HANDLE,PBOOL);
static NTSTATUS (WINAPI * pNtDeleteKey)(HANDLE);
+static NTSTATUS (WINAPI * pNtQueryKey)(HANDLE,int,PVOID,ULONG,PULONG);
static NTSTATUS (WINAPI * pRtlFormatCurrentUserKeyPath)(UNICODE_STRING*);
static NTSTATUS (WINAPI * pRtlFreeUnicodeString)(PUNICODE_STRING);
@@ -136,6 +137,7 @@ static void InitFunctionPtrs(void)
pRtlFormatCurrentUserKeyPath = (void *)GetProcAddress( hntdll,
RtlFormatCurrentUserKeyPath );
pRtlFreeUnicodeString = (void *)GetProcAddress(hntdll,
RtlFreeUnicodeString);
pNtDeleteKey = (void *)GetProcAddress( hntdll, NtDeleteKey );
+pNtQueryKey = (void *)GetProcAddress( hntdll, NtQueryKey );
}
/* delete key and all its subkeys */
@@ -2104,6 +2106,7 @@ static void test_classesroot(void)
DWORD type = REG_SZ;
static CHAR buffer[8];
LONG res;
+void *buf = malloc(300*sizeof(wchar_t));
You could just allocate a buffer on the stack. 300 is probably overkill,
100 looks like it would do it. You want to use WCHAR rather than wchar_t.
/* create a key in the user's classes */
if (!RegOpenKeyA( HKEY_CURRENT_USER,
Software\\Classes\\WineTestCls, hkey ))
@@ -2132,6 +2135,9 @@ static void test_classesroot(void)
RegCloseKey( hkey );
return;
}
+pNtQueryKey( hkcr, 3 /*KeyNameInformation*/, buf,
500*sizeof(wchar_t), (ULONG*)res );
Not that this will actually overflow, but you specify 500, but only
allocated 300. A straightforward way to accomplish agreement is to use
sizeof(buf) / sizeof(buf[0]) as the size, if it's not dynamically allocated.
+ok( wcsncmp((wchar_t*)buf+2, L\\REGISTRY\\USER, 14) == 0,
+key not from \\REGISTRY\\USER\n);
Using L constructs isn't portable, you'll need to declare these the
tedious way you'll find in other Wine tests:
static const WCHAR reg_user[] = { '\\','R',''E,'G','I','S','T','R',Y',
etc. Then you can use the sizeof(reg_user) / sizeof(reg_user[0]) trick to
specify the length.
I think Alexandre will object to using msvcrt functions (wcsncmp in this
case), but I don't have a straightforward alternative yet.
Welcome to Wine development :)
--Juan
Re: [PATCH] Tests that prove ntdll has no notion of HKCR.
(consider subscribing to wine-devel so your emails don't get stuck in moderation.) Hmm but I am already! Ok, that's strange. Maybe I just got it late. I think Alexandre will object to using msvcrt functions (wcsncmp in this case), but I don't have a straightforward alternative yet. strcmp is already used. What's the difference? Ah, well, strcmp comes from the C library your compiler uses. wcsncmp can only come from msvcrt. When compiling for Wine, this can result in mixing C runtime libraries, and hilarity can result. Say, do these tests work on Wine? If not, you'll want to mark them with todo_wine, i.e. todo_wine ok(...). --Juan
Re: GSoC 2013 - Registry Merging Project
Hi Guo, On Tue, May 14, 2013 at 1:32 PM, Guo Jian orz...@gmail.com wrote: I just found that the REG_OPTION_VOLATILE of create_key in hkcr may have some tricks. Not surprisingly found a strange situation when testing on windows. See my test here please : http://newtestbot.winehq.org/JobDetails.pl?Key=932 This happens as following : First create hkcu/software/classes/key1, then create the hkcr/key1/subkey with REG_OPTION_VOLATILE. RegEnumKey on hkcr will give two 'key1'. I tried to use RegQueryInfoKeyA to count them, also there are more than there should be. Why there are two duplicated keys? May it be a bug of windows? If it is, should we design our algorithm to avoid or treat it as a normal feature? That's interesting. Windows might allow both a volatile and non-volatile key of the same name. Since the volatile one is kept in-memory, any values that exist in it will be discarded eventually, so any inconsistency that arises isn't the kernel's problem. That'd be my guess, at any rate: even though such a behavior has the potential to confuse application developers, since it's not the kernel's problem, it's probably an overlooked quirk. As far as what to do with it: this is one of those nuanced, i.e. hard to decide, things with our tests. In general, we assume that Windows's behavior is the correct behavior, unless we have strong reason to assume otherwise. We've encountered many times that even what appear to be bugs end up being relied upon by application developers. In this case, though, I think you might have stumbled across something that's relatively rare, and unless there's an application that depends on this behavior, it might be worth ignoring for now. Hope that helps. --Juan
Re: Ask for mentor - Improve Bcrypt
Hi Kaiyi Zhang, (or is Zhang Kaiyi?) I think there's a little misconception in your proposal. Bcrypt the algorithm is not the same as the BCrypt functions in crypt32. I believe that Microsoft redesigned their CryptoAPI and more or less renamed their functions BCrypt*. I don't believe they have any relationship to the Bcrypt algorithm by Niels Provos and David Mazieres. I don't know how many applications depend on the Bcrypt* functions in newer crypt32 implementations, vs. the the Crypt* functions that are present in all versions of crypt32. If you want feedback on Wine crypto stuff, I'm probably the best to help you with that. Sorry if I've been a little absent, I'm pretty busy between my family and my day job of late. I have another idea for you that I think is tractable, and perhaps up your alley. The DH and DSS algorithms are still unimplemented in Wine. We already have tests for them, so I think it should be possible to contribute implementations. See the tests in dlls/dssenh/tests. I hope that helps. --Juan On Thu, May 9, 2013 at 8:35 PM, Kaiyi Zhang zky.own.s...@gmail.com wrote: Hello everyone: I am Kaiyi Zhang,A Computer Science and Engineering student from china, my major is information security. My nickname in #winehackers is KaiyiZhang. I heard the GSOC and I'm very interested in this, wine is tempting to me.I have submitted my wine application before the deadline of GSOC. However, There are some exams which takes a lot of time, So lack of communication with the wine developers. I know it is late to apply for GSoC, however, I'm still interesting to the cryptography work in Wine even if I'm not accept as an official GSoC2013 student. Cryptography work is difficult, and Wine is difficult, so cryptography in Wine is double difficult for a beginner like me, could I ask for a mentor for the cryptography work even if my proposal is not accepted? To my dear kindly future mentor: I don't know how to thank you, but I'll work hard on contributing to the Wine project as more as I can. Sincerely appreciate. My idea is bcrypt improvement, I think it's very suitable for me, I learned the information security. Cryptography is one of my courses. I once development a Elliptic curve cryptography Demonstration system which makes the ECC Graphical. And i found there is blank in bcrypt, I want to do some improvement in this. I know there it's difficult for me to develop. but i can code some test code firstly. I read the dlls/rsaendh/tests/rsaenh.c: test_hashes(),test_des(),test_rsa() etc. and test the code on ubuntu successfully. Here is my plan: Week 1 - Week 2 - Get the Winedebug and document more adept - Get more information about bcrypt - Bcrypt Wiki: http://en.wikipedia.org/wiki/Bcrypt - A Future-Adaptable Password Scheme: http://static.usenix.org/events/usenix99/provos/provos_html/node1.html - File of Bcrypt: http://bcrypt.sourceforge.net/ - Read the Document of MSDN about Bcrypt and code some examples - MSDN about Bcrypt: http://msdn.microsoft.com/en-us/library/windows/desktop/aa833130(v=vs.85).aspx Week 3 - Week 5 -Read some wine tests code and run some tests like: - the basic encryption dll/crypt32/ - Understand the bcrypt algorithm, there is a lot of code for learning like: - A Future-Adaptable Password Scheme: http://static.usenix.org/events/usenix99/provos/provos_html/node1.html - https://github.com/rg3/bcrypt - https://code.google.com/p/py-bcrypt/source/browse/bcrypt/bcrypt.c Week 6 - Week 11 - Code the BCrypt. According to the MSDN The Steps of Bcrypt - Open the Algorithm Provider - Get or Set Algorithm Properties - Create or Import a key - Perform Cryptiographic Operations - Close the Algorithm Provider There is a cross platform Bcrypt at http://bcrypt.sourceforge.net. if it is possible we can use it and its license compatible with the wine lgpl license. - add stub functions for Tests functions() and code the Tests functions(). The dlls/rsaenh/tests/rsaenh.c is a good reference. encrypt the pdData and compare with the right value. stub BCryptCreateHash() BCryptHashData() BCryptDuplicateHash() BCryptDuplicateHash BCryptDestroyHash() for test_hashes(). stub BCryptGenerateSymmetricKey() BCryptKeyDerivation() BCryptDestroyKey() for test_key_derivation(). stub BCryptGenRandom() for test_gen_random(). Week 12 - End - There are also some more cryptions need to tests, I can do more improvement these Thank you.
Re: [PATCH] msvcrt: fix character/byte confusion in buffer overflow branch
In general, I think you want to send this to wine-patches, not here.
On Mon, May 6, 2013 at 12:26 PM, Max Kellermann m...@duempel.org wrote:
The first memcpy() call in puts_clbk_str_w() confuses character count
and byte count. It uses the number of characters (out-len) as number
of bytes. This leaves half of the buffer undefined.
Interestingly, the second memcpy() call in the same function is
correct.
This bug potentially makes applications expose internal (secret) data.
Usually, the destination buffer is on the stack, and the stack often
contains secrets. Therefore, one could argue that this bug
constitutes a security vulnerability.
It'd be hard to make that argument convincingly. That's neither here nor
there, I suppose, but...
---
dlls/msvcrt/printf.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h
index cfba4b7..8b749bc 100644
--- a/dlls/msvcrt/printf.h
+++ b/dlls/msvcrt/printf.h
@@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len,
const APICHAR *str)
return len;
if(out-len len) {
-memcpy(out-buf, str, out-len);
+memcpy(out-buf, str, out-len*sizeof(APICHAR));
out-buf += out-len;
If the memcpy was incorrect, the += is also incorrect. I'm not sure which
is the case, but either way, your patch can't be correct as is.
--Juan
Re: [PATCH] msvcrt: fix character/byte confusion in buffer overflow branch
On Tue, May 7, 2013 at 9:10 AM, Piotr Caban piotr.ca...@gmail.com wrote:
On 05/07/13 17:46, Juan Lang wrote:
In general, I think you want to send this to wine-patches, not here.
This patch was also sent to wine-patches.
On Mon, May 6, 2013 at 12:26 PM, Max Kellermann m...@duempel.org
mailto:m...@duempel.org wrote:
---
dlls/msvcrt/printf.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h
index cfba4b7..8b749bc 100644
--- a/dlls/msvcrt/printf.h
+++ b/dlls/msvcrt/printf.h
@@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int
len, const APICHAR *str)
return len;
if(out-len len) {
-memcpy(out-buf, str, out-len);
+memcpy(out-buf, str, out-len*sizeof(APICHAR));
out-buf += out-len;
If the memcpy was incorrect, the += is also incorrect. I'm not sure
which is the case, but either way, your patch can't be correct as is.
out-buf is of APICHAR* type, so it's updated correctly. The patch looks
good for me.
Thanks, Piotr. Max, my apologies for the sloppy review.
--Juan
Re: GSoC 2013 - Registry Merging Project
Hi Guo Jian, just so you know, there's another application for the same project. This doesn't mean that yours can't be accepted, but it does impact your chances. Thank you for your interest. Good luck, --Juan On Fri, May 3, 2013 at 1:05 AM, orzhvs orz...@gmail.com wrote: Hi. I'm applying for the Google Summer of Code 2013, this is my proposal for the registry merging project. I'll work on improving Wine's registry in its stability and simulation to solve frequently seen non user-friendly problems, which usually caused by that application or system settings are not stored and fetched correctly. Project My Understand: I'm most interested in the project Registry - implement merging between HKEY_CLASSES_ROOT and HKEY_CURRENT_USER\Software\Classes. Its core task is to implement RegOpenUserClassesRoot, which returns a key token of merging view of HKCU HKLM classes rather than present returning HKCR Classes. The difficulity is that as a view of two independent registry trees, how to treat conflicts and make changes to two seperated entrys when using HKCR as root, and how to achieve the function with no or least effect on other part of registry operation such as RegOpenKeyExX. I have some seed ideas that maybe not mature but I like solving problems initially from a burst port : Scheme 1. Registry operations based on hKey ( handle ) under HKEY_CLASSES_ROOT will be degeneration to full path in text, like HKEY_CLASSES_ROOT\Subdir1, and try the operation after HKEY_CLASSES_ROOT replaced with HKEY_CURRENT_USER\Software\Classes and HKEY_LOCAL_MACHINE\Software\Classes in order, only the first success operation will be applied. This may satisfy the demand, both for query and modify. For details like the correct order, I'll make it out once get working on it. In this way, the HKCR key subtree does not exist in fact but only exists as a name. Scheme 2. After the base registry is initialized, build the HKCR's subkey trees with a new pulled in object type (say it 'symbol link') that directly point to the correct keys from HKLM or HKCU. Thus operation in HKCR tree are redirected to corresponding branch internally. When operating the HKCR itself, like creating new subkeys direct under HKCR, the operation is redirected to HKCU classes. This scheme seems more fluent but needs some tricks in the server part. What I'll Do : First of all I'll do a lot of tests on pure winows registry to definite my questions and guesses, write one or more test reports on different version of windows, some of which is already being done now. Followed by I will design the algorithms and data structures and make a complete development planation. Then enjoying coding and debuging Wine. I love the procedure, during the longest part of this project, keep writing daily report. Got the plan finished, I will applying improvement. Finally strict tests will be done to show if I achieved the final goal and a conclude paper will be carefully submitted. I'd like not only settle down the main project, but also several ideas to make Wine better, which are found in my daily use of Wine, such as add support for the shell context menu in Wine explorer for many convenient shotcuts of some software like the famous 7zip. I'll do this once time allows. Why Me : I myself is a faithful user of Wine. I know the thought of Wine and know not only basic usage but also a little implement of it. I understand Windows registry both Windows API pretty well from nearly 8 years coding. Have done much test and hooks in windows including RegXXX in advapi32.dll or lower level NtOpenKey in ntdll.dll. Further more, I used to do some kernel debug tests and hacks on registry, like hiding, protecting a subkey from regedit, resolving values from hive file rather than using API. These things are done under interests. I've been learning Windows API and OS theories from books and network like msdn for long. Ofcourse there will be no problem in registry knowledge. It's usual for me to write codes more than 1000 lines a day, for self use or for the competation ACM. I believe that I'm a careful man in codes and other things, I like discovering, guessing and proving. To prove I've developed a not too bad coding style, please refer to my submisions on codeforces http://codeforces.com/submissions/adnim (to view code click numbers under column #). And this may be a little evidence denoting that at least I've used git before, https://code.google.com/p/eryapass/ I know what should I do and what should not. I'm sure aware of the legal requirements. All of my tests will be done only in the black-box ways. No reverse work on Windows files is allowed and needed in Wine project, though I'm good at that. All codes will be written creatively and nothing will be copied from existing ones like WRK, RcactOS or something else. I love open source. I benefited
Re: [2/4] include: added transact.idl (resend)
Type it yourself. Refer to MSDN, public descriptions, and publicly
available headers, but don't copy/paste from any of them.
--Juan
On Wed, May 1, 2013 at 3:56 AM, Daniel Jeliński djelins...@gmail.comwrote:
It probably is, I downloaded it somewhere. Had to remove some stuff to get
it to compile. I think saw some mail saying that headers aren't copyrighted.
Anyway. What's the correct approach here? I mean, I clearly shouldn't add
anything, and removing stuff also doesn't look like the right idea. Well I
could remove comments, but that's hardly innovative. What's the usual way
to do it?
2013/5/1 Alexandre Julliard julli...@winehq.org
Daniel Jeliński djelins...@gmail.com writes:
+import unknwn.idl;
+
+interface ITransaction;
+interface ITransactionDispenser;
+interface ITransactionOptions;
+interface ITransactionOutcomeEvents;
+interface ITransactionCompletionEvents;
+
+/*==
+ * Transaction related types
+
*==*/
+
+[local,pointer_default(unique)]
+interface BasicTransactionTypes
+{
+
+typedef struct BOID {
+byte rgb[16];
+} BOID;
+
+cpp_quote(#define BOID_NULL (*((BOID*)(IID_NULL
+
+/* change the following two line together */
+cpp_quote(#ifndef MAX_TRAN_DESC_DEFINED)/* conflicts
with uimsg.h. This is temporary work around */
+cpp_quote(#define MAX_TRAN_DESC_DEFINED)
+typedef enum TX_MISC_CONSTANTS
+{
+MAX_TRAN_DESC = 40
+} TX_MISC_CONSTANTS;
+cpp_quote(#endif)
+
+/* Unit Of Work. */
+
+typedef BOID XACTUOW;
+
+/* Data type for isolation level values. */
+
+typedef LONG ISOLEVEL;
+
+/* Constants that specifiy isolation level of a transaction. */
+
+typedef enum ISOLATIONLEVEL {
+ISOLATIONLEVEL_UNSPECIFIED = 0x,
+ISOLATIONLEVEL_CHAOS= 0x0010,
+ISOLATIONLEVEL_READUNCOMMITTED = 0x0100,
+ISOLATIONLEVEL_BROWSE = 0x0100, /* Synonym for
_READUNCOMITTED */
+ISOLATIONLEVEL_CURSORSTABILITY = 0x1000,
+ISOLATIONLEVEL_READCOMMITTED= 0x1000, /* Synonym for
_CURSORSTABILITY */
+ISOLATIONLEVEL_REPEATABLEREAD = 0x0001,
+ISOLATIONLEVEL_SERIALIZABLE = 0x0010,
+ISOLATIONLEVEL_ISOLATED = 0x0010, /* Synonym for
_SERIALIZABLE */
+} ISOLATIONLEVEL;
+
+/* Transaction information structure, used in ITransaction */
+
+typedef struct XACTTRANSINFO {
+XACTUOW uow;/* The current unit of
work */
+ISOLEVELisoLevel; /* The isolation level
for the current UOW */
+ULONG isoFlags; /* Values from ISOFLAG
enumeration */
+DWORD grfTCSupported; /* Flags indicating
capabilities */
+DWORD grfRMSupported; /* ... of this
transaction wrt */
+DWORD grfTCSupportedRetaining;/* ...
parameters to Commit */
+DWORD grfRMSupportedRetaining;/* ... */
+} XACTTRANSINFO;
+
+typedef struct XACTSTATS {
+ULONG cOpen; /* The number of currently
extant transactions. */
+ULONG cCommitting;/* The number of transactions
which are proceding towards committing. */
+ULONG cCommitted; /* The number of transactions
that are have been committed. */
+ULONG cAborting; /* The number of transactions
which are in the process of aborting. */
+ULONG cAborted; /* The number of transactions
that are have been aborted. */
+ULONG cInDoubt; /* The number of transactions
which are presently in doubt. */
+ULONG cHeuristicDecision; /* The number of transactions
that have completed by heuristic decision. */
+FILETIMEtimeTransactionsUp; /* The amount of time that
this transaction service has been up. */
+} XACTSTATS;
This is clearly a straight copy of the SDK header. You can't do that.
--
Alexandre Julliard
julli...@winehq.org
Re: GSoC proposal
Hi George, On Wed, May 1, 2013 at 8:14 AM, George Stephanos gaf.stepha...@gmail.comwrote: Hello wine-devel! This is my proposal as a student for Google Summer of Code 2013. I'm George Stephanos from the Arab Academy for Science and Technology situated in Egypt. gsteph on #winehackers I'm currently about to finish my second semester as a BSc in CS. Previous Experience: I haven't developed for Wine before though I've always been a big fan of the project. I'm fairly good with C/C++ and Python and I think I have enough Win32 API experience. I've previously developed small WinSock, DirectX and classic GUI applications (with and without MFC). I'm a freelancer. I've worked (alone) on around 25-30 projects rating 4+/5 in each. I've enrolled in Google Code-In 2010 and 2011 (which is basically GSoC for high school students) writing lots of ARM assembly code for VideoLAN (#x264dev on freenode). In 2011, I would have ranked 12th if not for an issue that occurred between VideoLAN and Google. Proposal: I've read your ideas page and the only registry task [1] has particularly caught my attention. I'm aware of what I need to do. I've read [2], [3] and [4]. I haven't yet browsed the Wine tree at all and I'll probably be missing implementation strategies but that'll hopefully be worked out with my mentor. This would be nice to see, but I'll warn you, it might be a challenging one. The coding part of it probably won't end up all that large in the end, but you'll have to figure out a lot of the Wine architecture before you get anywhere with it. The registry is implemented in the Wine server (see server/registry.c), and APIs are implemented in ntdll (see dlls/ntdll/reg.c), and advapi32 (see dlls/advapi32/registry.c). Making changes to the server usually requires a lot more convincing of Alexandre than, say, a shiny new control panel applet. I usually advise people to get up to speed by first, building the wine tree, then attempting to add new tests. You might not have enough time to do that prior to the application deadline, and in this case, Andre already added some tests for you :) More things to read up about: these might be implemented using registry symbolic links. Anyway, that's enough background to get you started :) Good luck, --Juan
Re: secur32: Take schannel backend capabilities into account when configuring enabled protocols.
Hi Jacek, On Sat, Mar 30, 2013 at 8:36 AM, Jacek Caban ja...@codeweavers.com wrote: Most of the argument could be used against enabling TLS 1.1 and TLS 1.2, because it's not present on older Macs (nor enabled by default on Windows), so we'll have different behaviour. That's sadly something we have to live with. Anyway, I'm all for trying to avoid using SSL2, but I'd like to be a bit less extreme. How about this patch: http://source.winehq.org/patches/data/95298 With this patch, SSL2 will be completely disabled in default Wine config, but it will be still possible to enable by registries, if someone really needs it. Thanks, this approach looks good to me. --Juan
Re: secur32: Take schannel backend capabilities into account when configuring enabled protocols.
Hi Jacek, thanks for the detailed reply. On Fri, Mar 29, 2013 at 3:02 AM, Jacek Caban ja...@codeweavers.com wrote: Each protocol has two kinds of enable/disable flags: enabled and disabled by default. Those have different default values for each protocol and there are registry setting allowing to change each of them. Only enabled protocols are used at all. This patch limits enabled protocols to those that we can really support. If an application asks schannel to use default set of protocols (which I'd expect them to do unless they have a good reason), schannel will use all enabled protocols that are not disabled by default. An alternative to default set of protocols is listing each allowed separately. This means that if protocol is enabled and disabled by default it won't be used unless application explicitly asks for it. SSL2 is such a protocol by default. Do you think we should do this differently? Yes, my suggestion here is to explicitly disable SSL2 support altogether. GnuTLS doesn't support it, and having behavior that differs between Linux and Mac can be kind of maddening. I can imagine something like, embedded browser doesn't work for game X, with lots of works for me reports and the occasional fails here too, only to discover that it works on Mac but not Linux. The additional cost of a difference in behavior doesn't seem worth it, especially when the protocol itself really should have died long ago. --Juan
Re: secur32: Take schannel backend capabilities into account when configuring enabled protocols.
On Thu, Mar 28, 2013 at 12:31 PM, Ken Thomases k...@codeweavers.com wrote:
On Mar 28, 2013, at 6:05 AM, Jacek Caban wrote:
--- a/dlls/secur32/schannel_macosx.c
+++ b/dlls/secur32/schannel_macosx.c
@@ -630,6 +630,11 @@ static OSStatus schan_push_adapter(SSLConnectionRef
transport, const void *buff,
return ret;
}
+DWORD schan_imp_enabled_protocols(void)
+{
+/* NOTE: No support for TLS 1.1 and TLS 1.2 */
+return SP_PROT_SSL2_CLIENT | SP_PROT_SSL3_CLIENT |
SP_PROT_TLS1_0_CLIENT;
Do we really want to continue supporting SSL2? It's got a number of
vulnerabilities, and is disabled pretty much everywhere by now:
http://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_2.0
--Juan
Re: ntdll: make NtDelayExecution a bit more efficient
On Fri, Mar 8, 2013 at 6:11 AM, Graham Knap graham.k...@gmail.com wrote: Michael Stefaniuc wrote: i.e. commit 8099c2b9. JW says ... to more closely resemble Windows behavior. The key is to yield in a Sleep... JW is Jeremy White so us old timers chuckle now ;) I know the name, but nothing more. Why is this funny? He's the CEO of Codeweavers, and he's the first one to admit he doesn't write great code, or at least that Alexandre rejects his code a lot :) If the ancient wisdom isn't backed by tests there's a fair chance that it might not be applicable today. Or that it was a wrong theory Interesting. Unfortunately, I have no idea how to go about writing tests for this. Can anyone offer any suggestions? It's, well, hard to test. MSDN suggests it was true in XP, but subsequently changed: http://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx I no longer recall which app it was Jeremy was working on fixing, but there was at least one app that depended on this behavior. See Jeremy's posts on RFC: yield on Waits and Threading issues. --Juan
Re: iphlpapi: Let C look like C.
Hi Michael, this isn't actually a problem with your patch, just something I spotted: On Wed, Feb 6, 2013 at 3:15 PM, Michael Stefaniuc mstef...@redhat.comwrote: On 02/06/2013 11:16 PM, Austin English wrote: On Feb 6, 2013 11:13 PM, Michael Stefaniuc mstef...@redhat.de mailto:mstef...@redhat.de wrote: --- dlls/iphlpapi/ipstats.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c index f8191ba..966d24e 100644 --- a/dlls/iphlpapi/ipstats.c +++ b/dlls/iphlpapi/ipstats.c @@ -693,7 +693,7 @@ DWORD WINAPI GetIcmpStatisticsEx(PMIB_ICMP_EX stats, DWORD family) } ret = GetIcmpStatistics(ipv4stats); -if SUCCEEDED(ret) +if (SUCCEEDED(ret)) This code is incorrect. All paths that set ret yield a non-negative value for ret, so SUCCEEDED(ret) is always 1. Using SUCCEEDED/FAILED on something other than an HRESULT is asking for trouble. The correct thing to do is if (!ret). --Juan
Re: [PATCH] cryptui: use add_oid_to_usage correctly (Coverity)
On Sat, Feb 2, 2013 at 5:05 AM, Marcus Meissner mar...@jet.franken.dewrote: On Fri, Feb 01, 2013 at 03:48:27PM -0800, Juan Lang wrote: On Fri, Feb 1, 2013 at 3:45 PM, Juan Lang juan.l...@gmail.com wrote: Hi Marcus, -add_oid_to_usage(usage, ptr); +usage = add_oid_to_usage(usage, ptr); This looks fine, but would you mind making the same change on line 337? Actually, perhaps I hit sent too early. If this memory allocation fails, which is the only situation under which add_oid_to_usage doesn't just return its first parameter, it'll immediately crash on the next invocation with a NULL pointer dereference. I'm not sure it's worth all the trouble in an out of memory situation. Perhaps just remove the return value and let the caller crash. Actually the loop around checks that as a condition and would lead to return NULL: for (ptr = usageStr, comma = strchr(ptr, ','); usage ptr *ptr; For the second one the loop around does not catch it. I think the add_oid_to_usage() should not even do it this way and not touch usage at all, but instead return a memory allocation error and let the caller handle it:/ Or just let it crash. Yeah, I think just letting it crash is better than trying to sort it all out. Thanks, and sorry for the confusing and buggy code :/ --Juan
Re: [PATCH] cryptui: use add_oid_to_usage correctly (Coverity)
Hi Marcus, -add_oid_to_usage(usage, ptr); +usage = add_oid_to_usage(usage, ptr); This looks fine, but would you mind making the same change on line 337? Thanks, --Juan
Re: [PATCH] cryptui: use add_oid_to_usage correctly (Coverity)
On Fri, Feb 1, 2013 at 3:45 PM, Juan Lang juan.l...@gmail.com wrote: Hi Marcus, -add_oid_to_usage(usage, ptr); +usage = add_oid_to_usage(usage, ptr); This looks fine, but would you mind making the same change on line 337? Actually, perhaps I hit sent too early. If this memory allocation fails, which is the only situation under which add_oid_to_usage doesn't just return its first parameter, it'll immediately crash on the next invocation with a NULL pointer dereference. I'm not sure it's worth all the trouble in an out of memory situation. Perhaps just remove the return value and let the caller crash. Thanks again, --Juan
Re: [PATCH 8/8] wininet: Get rid of WORKREQ* types
Hi Jacek, just wanted to say these series of patches make me happy :) lpfnHungarianNotationDieDieDie ;) --Juan
Re: Wine Wiki needs your help!
Hi Kyle, On Tue, Jan 15, 2013 at 8:10 AM, Kyle Auble randomidma...@yahoo.com wrote: The one thing that would probably help a lot is if there was a regularly updated tarball of the wiki content either at WineHQ or Lattica's FTP again. I haven't messed with cron itself much, but my archive.cron script should pack up the files correctly. The main complication is that the user dir probably should be shared on a need-to-know basis because it contains weakly-hashed password info. Could the password hashes be excluded from the regular tarball? E.g. using --exclude in the tar command? --Juan
Re: [PATCH] crypt32: free the encoded msg (Coverity)
In case this was awaiting an ACK from me, this looks good. Thanks. --Juan
Re: [website] Are we doing something wrong with our secure connections?
Hi André, On Wed, Jan 2, 2013 at 8:08 AM, André Hentschel n...@dawncrow.de wrote: Hi, FWIW i have just seen: https://www.ssllabs.com/ssltest/analyze.html?d=testbot.winehq.orghideResults=on https://www.ssllabs.com/ssltest/analyze.html?d=test.winehq.orghideResults=on https://www.ssllabs.com/ssltest/analyze.html?d=winehq.orghideResults=onignoreMismatch=on which tells me we have some problems with secure website connection, the question is, do we need more security here? The answer is, no. More reasoning: in general, I don't think we're relying on any confidentiality in the patches we submit to testbot: anyone can connect and see the patches, as well as their test results. So no, I don't think problems with TLS on testbot are a concern, now or ever. And in particular: the qualsys scan tells us that the cert we're using is vulnerable to the BEAST attack, and that the server is vulnerable to the CRIME attack. The BEAST and CRIME attacks can allow an attacker to learn the plaintext of a stable piece of ciphertext sent in many connections, e.g. an authentication cookie, without having learned the server's private key. In testbot's case, we do use cookie-based authentication after initial login, but at worst that'd allow an attacker to submit jobs as one of us developers, or cancel one of our test jobs, change our password, etc. I don't think this is much of a concern. --Juan
Re: wininet: Don't perform revocation checks when verifying a certificate.
On Wed, Dec 12, 2012 at 12:32 AM, Hans Leidekker h...@codeweavers.comwrote: On Tue, 2012-12-11 at 12:59 -0800, Juan Lang wrote: Getting the client to trust the server cert can be as easy as ignoring untrusted root errors, if you don't think this impacts the revocation results. Returning revocation is straightforward enough, assuming you have a server under your control. So self-sign the CRL too. I guess that might work if ignoring untrusted root errors extends to verification of the CRL. Actually, I was thinking a 2-certificate chain, with the root signing the CRL. I don't think a cert that revokes itself has a lot of meaning. --Juan
Re: wininet: Don't perform revocation checks when verifying a certificate.
On Tue, Dec 11, 2012 at 6:10 AM, Hans Leidekker h...@codeweavers.comwrote: On Tue, 2012-12-11 at 14:52 +0100, Jacek Caban wrote: On 12/11/12 09:45, Hans Leidekker wrote: https://testbot.winehq.org/JobDetails.pl?Key=23300 is a test which shows that revocation checks fail for the certificate on outlook.com when passed straight to CertVerifyRevocation. The reason is that a CRL link specified in the certificate does not resolve. https://testbot.winehq.org/JobDetails.pl?Key=23301 is a test which makes a secure connection to outlook.com from wininet and shows that this succeeds. My conclusion is that native wininet doesn't perform revocation checks. Your tests prove that we should relax our verification on CERT_TRUST_IS_OFFLINE_REVOCATION or something similar. To prove that revocation checks are not made, a test with truly revoked cert would be needed. True, though to perform the revocation check the CRL has to be retrieved and my tests with wireshark didn't show any signs of that. Would adding to the tests as part of this patch be a bad thing? --Juan
Re: wininet: Don't perform revocation checks when verifying a certificate.
On Tue, Dec 11, 2012 at 12:37 PM, Hans Leidekker h...@codeweavers.comwrote: On Tue, 2012-12-11 at 11:52 -0800, Juan Lang wrote: On Tue, Dec 11, 2012 at 6:10 AM, Hans Leidekker h...@codeweavers.com wrote: On Tue, 2012-12-11 at 14:52 +0100, Jacek Caban wrote: On 12/11/12 09:45, Hans Leidekker wrote: https://testbot.winehq.org/JobDetails.pl?Key=23300 is a test which shows that revocation checks fail for the certificate on outlook.comwhen passed straight to CertVerifyRevocation. The reason is that a CRL link specified in the certificate does not resolve. https://testbot.winehq.org/JobDetails.pl?Key=23301 is a test which makes a secure connection to outlook.com from wininet and shows that this succeeds. My conclusion is that native wininet doesn't perform revocation checks. Your tests prove that we should relax our verification on CERT_TRUST_IS_OFFLINE_REVOCATION or something similar. To prove that revocation checks are not made, a test with truly revoked cert would be needed. True, though to perform the revocation check the CRL has to be retrieved and my tests with wireshark didn't show any signs of that. Would adding to the tests as part of this patch be a bad thing? I thought about that but I am hesitant to use a random site that's not under our control. The alternative is to setup our own site with a certificate that only fails the revocation check, which I think means that we need to have it signed by a trusted root and then revoked. I'm not sure we have the means to do that currently. Do you have any suggestions? I agree with you that this is a little finicky to test, but I think it's tractable. The desired outcome is a server trusted by the client under test, presenting a valid server cert, under two conditions: where the server cert's revocation can't be tested, and where the server's cert is actually revoked. First the easy bit: testing what happens when the revocation can't be checked involves a valid cert with a CRL distribution point that doesn't respond. Easy, as long as you can create the server cert that the client trusts. Getting the client to trust the server cert can be as easy as ignoring untrusted root errors, if you don't think this impacts the revocation results. Returning revocation is straightforward enough, assuming you have a server under your control. Now, the server: is it possible to have another foo.winehq.org provisioned that returns an arbitrary cert? If not, it's possible to run a server locally, but this is a lot more code overhead, so probably not worth it for this go around. --Juan
Re: Bug#566351: libgcrypt11: should not change user id as a side effect
On Wed, Nov 7, 2012 at 3:22 AM, Dan Kegel d...@kegel.com wrote: After gstreamer, gcrypt is also dropping support for alternative thread libraries. Good thing secur32/schannel_gnutls.c doesn't use it. (Right?) Right. If someone were motivated, we could begin to transition winhttp and wininet to schannel instead of OpenSSL, and we could eliminate this dependency. It's a lot to ask, but it seems, subjectively, like schannel has gotten enough love to be workable for most sites by now. --Juan
Re: iphlapi: Fix some leaks (coverity)
Hi Frédéric, thanks for the patch. Allow me to elaborate: On Tue, Nov 6, 2012 at 6:05 AM, Alexandre Julliard julli...@winehq.orgwrote: Frédéric Delanoy frederic.dela...@gmail.com writes: @@ -1520,8 +1521,8 @@ DWORD WINAPI GetIpAddrTable(PMIB_IPADDRTABLE pIpAddrTable, PULONG pdwSize, BOOL sizeof(MIB_IPADDRROW), IpAddrTableSorter); ret = NO_ERROR; } - HeapFree(GetProcessHeap(), 0, table); } +HeapFree(GetProcessHeap(), 0, table); } That's not the correct fix. The correct place to fix is getIpAddrTable in ifenum.c, so that should it fail, it frees the memory it allocated. This was obviously an oversight on my part. See the for loop here: http://source.winehq.org/source/dlls/iphlpapi/ifenum.c#L735 This loop terminates if something fails, so after the for loop is an ideal place to add a HeapFree. In this way, the callers (GetIpAddrTable is but one) never have to cope with freeing memory when getIpAddrTable fails. --Juan
Re: [PATCH] iphlpapi: Set DhcpEnabled to TRUE for all interfaces.
Hi Qian,
There is a winpcap based network authentication client which check for the
DhcpEnabled value, this patch make the app happy and then the app works with
André's pcap wrapper [1].
Please let me know if this is acceptable, or we have to correctly implement
DhcpEnabled status?
I think this is fine, but you'd want to remove/fix some outdated
comments I wrote at the same time:
* NOTES
* Since GetAdaptersInfo never returns adapters that have DHCP enabled,
* this function does nothing.
*
* FIXME
* Stub, returns ERROR_NOT_SUPPORTED.
*/
DWORD WINAPI IpReleaseAddress(PIP_ADAPTER_INDEX_MAP AdapterInfo)
{
TRACE(AdapterInfo %p\n, AdapterInfo);
/* not a stub, never going to support this (and I never mark an adapter as
DHCP enabled, see GetAdaptersInfo, so this should never get called) */
The same comments are in IpRenewAddress.
Historical reason: when I wrote iphlpapi's first implementation, I had
recently been working on a Windows app that released and renewed IP
addresses on DHCP-enabled interfaces, because Windows at the time was
pretty poor at doing so in response to changes to wireless connection
state. I figured I wouldn't be the only one to have had to work around
this issue, so I didn't want to have to implement IpReleaseAddress and
IpRenewAddress in Wine.
That was almost 10 years ago now, so these reasons no longer apply.
I'm pretty sure the app I wrote isn't in use any longer, and even if
it is, it isn't in use on Wine (I also wrote a Linux version.)
Thanks,
--Juan
Re: shell32: Add tests for ShellExecute()'s handling of file URLs.
Hi Francois, pretty sure you didn't mean to leave this hunk in: @@ -2296,9 +2406,13 @@ START_TEST(shlexec) init_test(); +#if 0 test_argify(); test_lpFile_parsed(); test_filename(); +#endif +test_fileurl(); +#if 0 test_find_executable(); test_lnks(); test_exes(); @@ -2308,6 +2422,7 @@ START_TEST(shlexec) test_commandline(); test_directory(); test_trojans(); +#endif --Juan
Re: [PATCH 21/21] jscript: Pack jsval_t to 64-bit structure on i386
Hi Jacek, + * that NaN value representation has 52 (almost) free bytes. You mean bits, yes? While you're at it, + * jsval_t structure is used to represent JavaScript dynamicaly-typed values. dynamically is spelled with two l's. Thanks, --Juan
Re: dssenh: implementing the dssenh cryptographic service provider
Hi Marek, On Wed, Sep 12, 2012 at 2:57 PM, Marek Chmiel kcm...@gmail.com wrote: Hello all, over the summer I had a chance to complete the regression test suite for the DSSENH cryptographic provider; thank you to those that helped along the way. I am currently researching how to implement the actual provider, and any suggestions would be greatly appreciated. A question I have is: RSAENH AND DSSENH both use the same encryption algorithms (I am not 100% sure about that yet), so for example could I use one that was written for RSAENH or shall I write those functions on my own using Libtomcrypt? There are some algorithms in common, e.g. MD5 and SHA1. For these, I think you'd want to copy/paste the existing implementation from rsaenh into dssenh. There are other algorithms, e.g. DSS aka DSA, e.g. DH, that are not. For these, I think you'd want to use an existing implementation, but since none exists in Wine, you'd have to import new code. libtomcrypt's license makes it relatively easy to use. Hope that helps, --Juan
Re: Fix the FIXME message to reflect the real problem.
On Mon, Jun 4, 2012 at 2:19 AM, Piotr Caban piotr.ca...@gmail.com wrote: On 06/02/12 13:35, m...@mtew.isa-geek.net wrote: @@ -2735,7 +2735,7 @@ static BOOL CommitUrlCacheEntryInternal( goto cleanup; } - FIXME(entry already in cache - don't know what to do!\n); + FIXME(collision handler needed - the entry is already in use!\n); This message is not about entry being used. I think the old message is quite accurate. The don't know what to do part is quite strange but I don't see reason to change it. I agree. The reason we get these all the time is that we never fulfill requests from cache, but we do write to the cache, so for all potential cache hits, we write duplicate cache entries. After 4 entries have been written, we begin to see the fixme. If this message is annoying, it's working: we need to start fulfilling requests from cache, instead of just filling up users' hard drives. --Juan
Re: [3/3] iphlpapi: Check for the right TCP statistics structure name (for DragonFly BSD)
Hi André, +#if HAVE_STRUCT_TCPSTAT_TCPS_CONNATTEMPT +struct tcpstat tcp_stat; +#elif HAVE_STRUCT_TCP_STATS_TCPS_CONNATTEMPT +struct tcp_stats tcp_stat; +#endif struct tcpstat tcp_stat; I think you meant to remove the declaration outside of the #if. --Juan
crypt32/tests(1/6): Don't shadow a variable with a variable of a different type
--Juan
From 728b013f2ca52de878dc65633a9c6cbcb1a9002c Mon Sep 17 00:00:00 2001
From: Juan Lang juan.l...@gmail.com
Date: Thu, 17 May 2012 15:55:16 -0700
Subject: [PATCH 2/7] Don't shadow a variable with a variable of a different
type
---
dlls/crypt32/tests/cert.c | 36 ++--
1 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/dlls/crypt32/tests/cert.c b/dlls/crypt32/tests/cert.c
index 9678053..6e46a57 100644
--- a/dlls/crypt32/tests/cert.c
+++ b/dlls/crypt32/tests/cert.c
@@ -2098,7 +2098,6 @@ static void testCreateSelfSignCert(void)
if (context)
{
DWORD size = 0;
-PCRYPT_KEY_PROV_INFO info;
/* The context must have a key provider info property */
ret = CertGetCertificateContextProperty(context,
@@ -2106,24 +2105,25 @@ static void testCreateSelfSignCert(void)
ok(ret size, Expected non-zero key provider info\n);
if (size)
{
-info = HeapAlloc(GetProcessHeap(), 0, size);
-if (info)
+PCRYPT_KEY_PROV_INFO pInfo = HeapAlloc(GetProcessHeap(), 0,
size);
+
+if (pInfo)
{
ret = CertGetCertificateContextProperty(context,
- CERT_KEY_PROV_INFO_PROP_ID, info, size);
+ CERT_KEY_PROV_INFO_PROP_ID, pInfo, size);
ok(ret, CertGetCertificateContextProperty failed: %08x\n,
GetLastError());
if (ret)
{
/* Sanity-check the key provider */
-ok(!lstrcmpW(info-pwszContainerName, cspNameW),
+ok(!lstrcmpW(pInfo-pwszContainerName, cspNameW),
Unexpected key container\n);
-ok(!lstrcmpW(info-pwszProvName, MS_DEF_PROV_W),
+ok(!lstrcmpW(pInfo-pwszProvName, MS_DEF_PROV_W),
Unexpected provider\n);
-ok(info-dwKeySpec == AT_SIGNATURE,
- Expected AT_SIGNATURE, got %d\n, info-dwKeySpec);
+ok(pInfo-dwKeySpec == AT_SIGNATURE,
+ Expected AT_SIGNATURE, got %d\n, pInfo-dwKeySpec);
}
-HeapFree(GetProcessHeap(), 0, info);
+HeapFree(GetProcessHeap(), 0, pInfo);
}
}
@@ -2152,7 +2152,6 @@ static void testCreateSelfSignCert(void)
if (context)
{
DWORD size = 0;
-PCRYPT_KEY_PROV_INFO info;
/* The context must have a key provider info property */
ret = CertGetCertificateContextProperty(context,
@@ -2160,24 +2159,25 @@ static void testCreateSelfSignCert(void)
ok(ret size, Expected non-zero key provider info\n);
if (size)
{
-info = HeapAlloc(GetProcessHeap(), 0, size);
-if (info)
+PCRYPT_KEY_PROV_INFO pInfo = HeapAlloc(GetProcessHeap(), 0, size);
+
+if (pInfo)
{
ret = CertGetCertificateContextProperty(context,
-CERT_KEY_PROV_INFO_PROP_ID, info, size);
+CERT_KEY_PROV_INFO_PROP_ID, pInfo, size);
ok(ret, CertGetCertificateContextProperty failed: %08x\n,
GetLastError());
if (ret)
{
/* Sanity-check the key provider */
-ok(!lstrcmpW(info-pwszContainerName, cspNameW),
+ok(!lstrcmpW(pInfo-pwszContainerName, cspNameW),
Unexpected key container\n);
-ok(!lstrcmpW(info-pwszProvName, MS_DEF_PROV_W),
+ok(!lstrcmpW(pInfo-pwszProvName, MS_DEF_PROV_W),
Unexpected provider\n);
-ok(info-dwKeySpec == AT_KEYEXCHANGE,
-Expected AT_KEYEXCHANGE, got %d\n, info-dwKeySpec);
+ok(pInfo-dwKeySpec == AT_KEYEXCHANGE,
+Expected AT_KEYEXCHANGE, got %d\n, pInfo-dwKeySpec);
}
-HeapFree(GetProcessHeap(), 0, info);
+HeapFree(GetProcessHeap(), 0, pInfo);
}
}
--
1.7.7.3
Re: func or pFunc in tests ?
Hi Alexandre, On Sun, May 27, 2012 at 6:22 AM, GOUJON Alexandre ale.gou...@gmail.com wrote: Some tests call directly the function by its name (i.e. GetWindowsDirectory, CreateFileA, CloseHandle...) while others declare a pointer to the function (prefixing its name by 'p' and followed by a capital letter) retrieved via GetProcAddress. I know that tests have been written by lots of different people at different times and the general rule is follow the style of the test file to be consistent but here, both styles exist in the same file (kernel32/tests/volume.c). Is there any rule concerning using func over pFunc ? Is it related to the module it comes from ? Basically, if the function is available on all versions of Windows that the test bot runs on, use func. If it's not available on some versions, and you need to test whether it's available to avoid a non-running test, use pFunc instead. Regarding the history, a lot of the usage of pFunc in the tests is for Windows 95/98/ME, which we no longer test on, so their continued use is legacy. That's not true in all cases, e.g. often a function wasn't available in Windows NT 4.0 either. --Juan
Re: kernel32: Correct WideCharToMultiByte and MultiByteToWideChar error codes and conditions (try 4)
Hi Alex,
first, thanks for taking the time to respond to feedback. Showing
responsiveness helps a great deal.
Next, on your patch: I'm trying to help you get this committed, so
this is meant to be constructive feedback.
+static void test_WideCharToMultiByte_error(UINT page, DWORD flags, LPCWSTR src,
+ INT srclen, LPSTR dst, INT dstlen,
+ LPCSTR defchar, BOOL* used,
+ const char* test_description,
+ DWORD expected_error)
+{
+SetLastError(ERROR_SUCCESS);
+WideCharToMultiByte(page, flags, src, srclen, dst, dstlen, defchar, used);
+ok(GetLastError() == expected_error,
+ codepage %i, %s, expected error %i got error %i\n, page,
+ test_description, expected_error, GetLastError());
+}
+
+static void test_MultiByteToWideChar_error(UINT page, DWORD flags, LPCSTR src,
+ INT srclen, LPWSTR dst, INT dstlen,
+ const char* test_description,
+ DWORD expected_error)
+{
+SetLastError(ERROR_SUCCESS);
+MultiByteToWideChar(page, flags, src, srclen, dst, dstlen);
+ok(GetLastError() == expected_error,
+ codepage %i, %s, expected error %i got error %i\n, page,
+ test_description, expected_error, GetLastError());
+}
These helpers don't really gain you anything here, there's really not
enough code to be worth using helpers. Please just copy/paste the
changes everyplace you're currently using them. Also, a common
convention we use is to set last error to 0xdeadbeef prior to a test
we expect to set last error on failure.
Thanks,
--Juan
urlcache: what all are you working on?
Hi Morten and Piotr, I notice both of you are working on wininet's url cache. That's great: it's largely bitrot, and has been for years. My question is, what are you planning to fix? It seems like you're both running into some of the same issues right now, and it would be nice if you could coordinate a little. Ok, I lie: it'd be nice to get code from both of you ;) If I were to guess, it looks like you're both working on eliminating corruption and deadlocks, both of which can occur right now. Fantastic. Another thing that'd be nice to see is reducing the amount of disk space used. Morten, some of the patches you sent me do some of that, by removing files once they're no longer in use. (The patches I have do that too.) Piotr, were you planning to address this too? A question that I have to ask is, would be better to eliminate the url cache altogether? Right now its only utility is to pass test cases, and conveniently fill up your disk. No requests are ever fulfilled from the cache. Either of you have any plans to tackle that? Thanks, --Juan
Re: Programmatically determining arguments to a WinAPI function
Hi Roger, On Mon, Apr 2, 2012 at 10:41 AM, Roger Cruz roger_r_c...@yahoo.com wrote: I'm looking for a way to determine programmatically what the arguments to a Win32 API implemented in Wine are. I'm trying to implement an API redirection stub that I can use to trace calls into all of the Wine-implemented DLLs. My stub needs to know what each argument passed into the Win32 API, their types and sizes, and whether they reside on the stack or register when calling the routine and finally their return value. I think you want to look at winapi (http://source.winehq.org/source/tools/winapi/). In researching Wine, I have come to find out two tools, winedump and winebuild that may be helpful in doing this but I have yet to find out how to use them correctly to get the information I seek. Maybe they just don't have that capability or maybe I just haven't played with them enough to find all of its uses. For example, I found that each DLL has a *.spec file which looks to have some of the info I need: @ stdcall GetSystemTimeAsFileTime(ptr) Looking at the man pages, this tells me that GetSystemTimeAsFileTime takes in an argument as a pointer but I don't see any return value information here. Also, does this file ever represent arguments passed by value on the stack, like a structure? If so, does it capture how big the structure is? What about function prototypes with variable arguments (similar to printf(...)).. are those capture in the file? Can I also assume (based on the calling convention listed as stdcall) which arguments will be on registers and which will be on the stack? Yes. stdcall, cdecl, and fastcall are all documented as to their calling convention. In particular, stdcall and cdecl both accept all arguments on the stack and return through EAX. (They differ in who pops arguments from the stack.) This is basic Win32 stuff, any Windows reference in the last 25+ years ought to tell you it. fastcall is more recent, but they're all up on Wikipedia: http://en.wikipedia.org/wiki/X86_calling_conventions --Juan
Re: Would like to get some feedback on GSOC idea
Hi Marek, On Tue, Mar 27, 2012 at 9:13 AM, M C kcm...@gmail.com wrote: Hi everyone, My name is Marek Chmiel, I am a student a NEIU. I am studying computer science and network security related topics. This semester I had spent a fair amount of time writing crypt related functions with java, and became very interested in Cryptography. After looking over the suggested ideas on SOC official wine wiki I was pleasantly surprised to see that there was already a crypt related idea suggested. I have a great understanding of c++ and java, and though I know c I have not worked with it as much as the other languages. Implementing a DSS provider sounds like an awesome task to tackle, if someone has never written code for Linux, can they take on such a task? Are there any significant issues implementing a DSS that you guys know about? Any crypt GSOC ideas would you suggest for someone who has wrote limited amount of code for Linux? If you know C, you ought to be able to code in Linux. I don't see that being a significant hurdle. I don't know of any significant issues implementing a DSS provider. The usual gotchas with coding for Wine apply here: tests, tests, tests. Solid test cases are a prerequisite to adding new code to Wine, and if a SoC project produces nothing but a solid test suite, it's still a win. I expect this is a pretty straightforward project for someone motivated, so I'm happy to see someone interested in picking it up. What I would also like to do, besides implementing a DSS provider, is to fix most if not all of the crypt related bugs that remain in Wine. This might seem like more than most people can chew, but viewing the Bug Tracking database it appears that there aren't that many crypt related bugs and might even be caused by a lack of a DSS provider (Hopefully this wouldn't create a entire new series of bugs). There aren't that many bugs, it's true. I don't think many of them relate specifically to DSS, and some may be quite challenging to fix. But the more you can fix, the better :) As you dig more into the project, I'm sure you'll have more specific questions. By all means ask once you get there. (Also, I'll try to be a mentor, should your project be accepted. I may be on vacation part of the summer, hopefully not enough to impact your work too much.) --Juan
Re: Major mmdevapi and winmm audio bugs
Hi Andrew, one small contribution: Winmm's timer functions use poll() with a timeout value, subtracting the time elapsed curing the callback. This works quite well in dsound. The Win32 API also provides the SetTimer API. But that depends on a message queue, which is a hassle I don't know if we want to bother with in the drivers (plus, it may be no more reliable than the TimerQueues). Using SetTimer is not the way you want to go. Its timing is unpredictable and its interface strange. I'd stay away from it. --Juan
Re: SourceForge forbids download
Hi Aidin, On Mon, Feb 6, 2012 at 11:01 AM, Aidin Gharibnavaz ai...@aidinhut.com wrote: SourceForge blocked download for some countries by default, such as Iran. (More detail can be found here) So, if it's not illegal to you to let Iranians download your software, please go to the SourceForge's ProjectAdmin, and set the ExportControl option, to let us enjoy the great work of yours. While it may or not be illegal, and I am not a lawyer, I believe it isn't legal to allow a download from the US without having applied for a review first: http://www.bis.doc.gov/encryption/encfaqs6_17_02.html Wine includes general encryption algorithms, not just authentication, and doesn't have restrictions on key length. While many of the cryptographic primitives were obtained from freely-available source code, I don't know whether things like certificate verification fall under the review requirements. Hence, I don't believe that we can enable downloads from Iran on SourceForge. It's entirely possible to get the Wine source via other means, however. Sorry for the hassle. --Juan
Re: [2/3] msi: Use the return value of IXMLDOMNode_get_text (clang).
Hi Hans, hr = IXMLDOMNode_get_text( node, s ); IXMLDOMNode_Release( node ); -if (!strcmpW( s, product_code )) r = ERROR_SUCCESS; -SysFreeString(s); +if (hr == S_OK !strcmpW( s, product_code )) r = ERROR_SUCCESS; +SysFreeString( s ); Is it really safe to free s if IXMLDOMNode_get_text failed? --Juan
Re: crypt32: Only accept trailing NULLs in a certificate common name.
Hi Erich, On Tue, Jan 31, 2012 at 9:34 AM, Erich E. Hoover ehoo...@mines.edu wrote: On Tue, Jan 31, 2012 at 10:23 AM, Erich E. Hoover ehoo...@mines.edu wrote: On Tue, Jan 31, 2012 at 10:04 AM, Juan Lang juan.l...@gmail.com wrote: Sorry I didn't spot this earlier. Without this, someone who registers a certificate common name with an embedded NULL, like codeweavers.com\0.badguy, could fool crypt32 into accepting it for a domain it isn't registered to, codeweavers.com in my example. It looks like you've just changed it to allow more than one NULL at the end... It seems to me that the matching code already handles the case of an embedded NULL, since it goes through the allowed_len characters and manually checks each byte (rather than using a routine like strcmp() which stops at NULLs). Well, sort of. The byte-by-byte comparison takes place component by component. The boundary between each component is defined by the presence of a '.'. That's why, in my example, I have an embedded NULL immediately prior to a '.'. After the end of each component is found, it's passed to match_domain_component. In the current git version, each component strips a NULL, hence an embedded NULL is accepted. In the version I sent, only trailing NULL(s) are removed. You're right that I allow multiple trailing NULLs rather than just one, but that difference seems immaterial. The key one is to prevent NULLs immediately prior to dots. --Juan
Re: crypt32: Only accept trailing NULLs in a certificate common name.
Wow, I clearly didn't read that you moved the code over to match_common_name. Apparently I'm not quite conscious today, my apologies! Clearly I wasn't quite conscious when I reviewed your patch in the first place :) No worries, for security fixes lots of checking is always worthwhile! --Juan
Re: [PATCH 1/1] crypt32: Fix domain component length check.
Hi Erich, On Fri, Jan 27, 2012 at 10:33 AM, Erich E. Hoover ehoo...@mines.edu wrote: On Fri, Jan 27, 2012 at 11:16 AM, Austin English austinengl...@gmail.com wrote: ... Forgot the patch. Thanks! I don't know how I missed that, apparently I'm blind today. surely this needs a test. Ping me if you need help creating one. Thanks, --Juan
Re: In-process wineserver
Hi Daniel, On Mon, Jan 23, 2012 at 3:15 AM, Alexandre Julliard julli...@winehq.org wrote: Daniel Santos danielfsan...@att.net writes: I've updated my in-process wineserver hack, cleaned it up a bit more and fixed a few problems. So, at least in Star Wars Battlefront II, the sound and HID problems are fixed (the select server call must be made via the pipe). I presume there are other server calls with similar properties. Also, the wineserver now exits properly and cleans up it's instance when the main process terminates. Changed the enabling enviroment variable to WINESPEEDHACK. So what would it take for this to be integrated into wine as an optional feature? You'll probably have to kill me first ;-) a more serious reply: you may not be aware the Transgaming did just such a thing years ago, and lobbied pretty heavily for it to be included. AJ declined at the time. The reasons for not including it remain the same: because of the possibility of a buggy app taking down the wineserver, we couldn't ever attempt to support such a beast. Any bug reported against it would be suspect. Worse, people might report bugs neglecting to mention that they'd enabled it. So no, we really won't ever include such a feature as part of an official release around here. We already have enough trouble with people reporting bugs in unsupported configurations. You're welcome to maintain your own fork, or maintain your patch for people to try in their own Wine builds if they like. --Juan
Re: richedit: v1.0 richedit uses CR and LF for enter
Hi Jason, + ME_InsertTextFromCursor(editor, 0, endlv10[0], 2, style); you want to use endlv10 instead, i.e.: + ME_InsertTextFromCursor(editor, 0, endlv10, 2, style); --Juan
Re: richedit: v1.0 richedit uses CR and LF for enter
This was deliberate - I did that originally and I got a compile warning as one is const WCHAR * and one is const WCHAR[2] (Not sure if that error was from a MSVC windows or Linux compile, but I was trying to avoid it, and a typecast was a bit pointless as the above is accurate as well, isnt it?) You could declare it non-const. That's often what we end up doing in the tests, that or casting away the const. --Juan
Re: richedit: v1.0 richedit uses CR and LF for enter
Whats the difference between a typecast and var[0]? I wasn't advocating for a typecast, merely admitting that we use them from time to time. What's the compile warning you're actually seeing on gcc? I don't think we care what warnings are produced in MSVC. --Juan
Re: Debugging Wine with Lightroom 3.5
Hi Roland, On Sun, Dec 18, 2011 at 6:46 AM, Roland Baudin rolan...@free.fr wrote: Yes, I suspected such a mechanism. Now if I understand well I have to find which one is the builtin mechanism and which one is the fallback mechanism. Is there some documentation around about the differences between Vista and XP? That list would be large, too large to be conveniently listed in one place. What one could do, which is unfortunately often equally tedious, is to compare +relay logs from running the app in Wine under both modes. If looking through multi-GB log files is your idea of fun, you have the makings of a Wine developer ;) --Juan
Re: crypt32: Avoid reading unitialized variables (Coverity)
On second thought, coverity would still complain even with the
TRACEs adapted, so I'll mark these defects as Ignored in Coverity
Does the attached patch address one of the Coverity complaints?
--Juan
From 13115c8ab68550b38f992eba04c9a05e88696f73 Mon Sep 17 00:00:00 2001
From: Juan Lang juan.l...@gmail.com
Date: Thu, 15 Dec 2011 19:20:50 -0800
Subject: [PATCH] Don't trace an in/out parameter when it's an out parameter
---
dlls/crypt32/encode.c |8
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/dlls/crypt32/encode.c b/dlls/crypt32/encode.c
index 05a3558..7590012 100644
--- a/dlls/crypt32/encode.c
+++ b/dlls/crypt32/encode.c
@@ -185,7 +185,7 @@ BOOL WINAPI CRYPT_AsnEncodeSequence(DWORD dwCertEncodingType,
DWORD i, dataLen = 0;
TRACE(%p, %d, %08x, %p, %p, %d\n, items, cItem, dwFlags, pEncodePara,
- pbEncoded, *pcbEncoded);
+ pbEncoded, pbEncoded ? *pcbEncoded : 0);
for (i = 0, ret = TRUE; ret i cItem; i++)
{
ret = items[i].encodeFunc(dwCertEncodingType, NULL,
@@ -580,7 +580,7 @@ static BOOL CRYPT_AsnEncodeCRLEntry(const CRL_ENTRY *entry,
DWORD cItem = 2;
BOOL ret;
-TRACE(%p, %p, %p\n, entry, pbEncoded, pcbEncoded);
+TRACE(%p, %p, %d\n, entry, pbEncoded, pbEncoded ? *pcbEncoded : 0);
if (entry-cExtension)
{
@@ -736,7 +736,7 @@ static BOOL CRYPT_AsnEncodeExtension(CERT_EXTENSION *ext, BYTE *pbEncoded,
};
DWORD cItem = 1;
-TRACE(%p, %p, %d\n, ext, pbEncoded, *pcbEncoded);
+TRACE(%p, %p, %d\n, ext, pbEncoded, pbEncoded ? *pcbEncoded : 0);
if (ext-fCritical)
{
@@ -3194,7 +3194,7 @@ BOOL WINAPI CRYPT_AsnEncodeOctets(DWORD dwCertEncodingType,
DWORD bytesNeeded, lenBytes;
TRACE((%d, %p), %08x, %p, %p, %d\n, blob-cbData, blob-pbData,
- dwFlags, pEncodePara, pbEncoded, *pcbEncoded);
+ dwFlags, pEncodePara, pbEncoded, pbEncoded ? *pcbEncoded : 0);
CRYPT_EncodeLen(blob-cbData, NULL, lenBytes);
bytesNeeded = 1 + lenBytes + blob-cbData;
--
1.7.1
Re: crypt32: Avoid reading unitialized variables (Coverity)
Should the TRACE (e.g. on crypt32:2435) be adapted so it conditionally prints the value instead? Or be removed altogether? I'd prefer a conditional print if it's not too ugly. There haven't been too many decoding bugs in a while, but the trace can sometimes help identify them. Thanks, --Juan
Re: Re: added remaining functions, data structures, definitions required by http api to include/http
can you be more specific please?
Yes:
+#ifndef __HTTP_H__
+#define __HTTP_H__
Don't do that, it's already #ifdef protected.
+/*#if _WIN32_WINNT = 0x0501*/
Dead code, just leave it out.
+typedef struct _HTTP_REQUEST_V2
+{
+/* TODO : anonymous structure is not supported by C standard */
+/* struct HTTP_REQUEST_V1; */
For one thing, that's what the DUMMYSTRUCTNAME macro is for. For
another, by omitting it, you've made the structure binary incompatible
with the PSDK one for C code. Clearly that's wrong.
I stopped reviewing here, you have many examples of the above error.
I'm also concerned whether this was the product of copy/paste, which
isn't allowed. Was it?
--Juan
Re: Re: Re: added remaining functions, data structures, definitions required by http api to include/
Hi Arash, I'm also concerned whether this was the product of copy/paste, which isn't allowed. Was it? about that copy/paste stuff, well I guess we eventually produce an almost identical header file with one in the SDK I mean everything in the header file is already exposed to the user and IS part of API am I not right? I might agree with you, but lawyers might not. As a result, we don't allow copy/paste code in Wine, even in headers. just one more question if you don't mind! should I use winsock for socket programming or native linux's? and for some library I may need (regex for example) is there any restriction or convection or something like that or I can use any third party lib? Native linux sockets are typically used, unless there's good reason for them not to be. 3rd party libraries are harder to work with. You typically have to choose something which you expect will be installed on every common platform, built appropriate configure checks to determine if the library is present at compile time, make your code fail gracefully if not, and dynamically load the library at runtime. Static linking is also viable, if you expect the library is commonly available, and if the license is compatible: LGPL and BSD are, GPL is not, just to give a few examples. It'd help more if we knew precisely what you're working on, and what you need e.g. regex for. --Juan
Re: [PATCH 1/1] ws2_32: Selectively bind UDP sockets to interfaces while still allowing broadcast packets (try 2, resend).
Hi Erich,
+ * Bind the given socket exclusively to a specific interface.
I know the style around here is to comment minimally, but I didn't
find it clear what the behavior of this function is. For one thing,
+static int interface_bind( int fd, struct sockaddr *addr )
(snip)
+int ret = FALSE;
(snip)
+ret = TRUE;
You're using #defines used with BOOL, so a BOOL return value would be
a little clearer IMO. int, in the context of socket functions, often
uses 0 = success, -1 = failure.
Still, even with that change,
+if (in_sock-sin_addr.s_addr == htonl(WS_INADDR_ANY))
+{
+/* Not binding to specific interface, use default route */
+goto cleanup;
+}
+optlen = sizeof(sock_type);
+if (getsockopt(fd, SOL_SOCKET, SO_TYPE, sock_type, optlen) == -1
+|| sock_type != SOCK_DGRAM)
+{
+/* Not a UDP socket, interface-specific bind is irrelevant. */
+goto cleanup;
These two blocks will use the default return value, FALSE, and it
wasn't clear to me from the function name or comment that it should
return FALSE in this case. Not that doing so is incorrect: just that
it isn't obvious that that's the way it should behave. In this case,
I feel a comment indicating that the function returns TRUE iff the
function bound to a specific device, and that FALSE doesn't
necessarily mean failure, is merited.
Also,
+/* Obtain the size of the IPv4 adapter list and allocate
structure memory */
+if (GetAdaptersInfo(NULL, adap_size) != ERROR_BUFFER_OVERFLOW)
Why do all this outside the #ifdef SO_BINDTODEVICE guard? I'd rather
you just had a different implementation of the function when
SO_BINDTODEVICE isn't present that issues the warning and does nothing
else.
I'll let Alexandre comment on whether the approach itself is appropriate.
Thanks,
--Juan
Re: added remaining functions, data structures, definitions required by http api to include/http.h
That's a lot of stuff. What needs it? It's also obviously incorrect in places. --Juan
RFH: Solaris users with IPv6
Hi all, in the tidy up I've been doing to iphlpapi, I notice that on Solaris it's possible to enumerate IPv6 addresses using SIOCGIFCONF. I've got a patch written that does that, but I have no way to test it myself. If anyone here has some version of Solaris with a machine with IPv6 addresses, even autoconfigured ones, and feels like testing a patch, please let me know. I'll send the patch and a small test program. Thanks, --Juan
RFC: depending on getifaddrs in iphlpapi
Hi folks, right now iphlpapi uses autoconf to detect if getifaddrs is available, and only uses it to enumerate IPv6 addresses. I'm thinking of replacing the current IPv4 enumeration code, which uses ioctl/SIOCGIFCONF, with getifaddrs. The reason I'm leaning toward replacing, rather than adding side-by-side with the current code, is that getifaddrs is commonly available: it's available on all recent versions of Linux, as well as on FreeBSD and MacOS. There might be some platform somewhere that's disenfranchised by this, but I'm not sure it's worth the code complexity to maintain the current code. Any strenuous objections? Reasons to use getifaddrs rather than ioctls: 1. SIOCGIFCONF is actually pretty fiddly, and is the current code is broken on FreeBSD, or FreeBSD's support is broken (bug 27653.) 2. SIOCGIFFLAGS is apparently broken on current Linux versions. I haven't found any bug against Wine or Linux about that, I just noticed it while testing the existing code. Thanks, --Juan
Re: RFC: depending on getifaddrs in iphlpapi
I'm thinking of replacing the current IPv4 enumeration code, which uses ioctl/SIOCGIFCONF, with getifaddrs. The reason I'm leaning toward replacing, rather than adding side-by-side with the current code, is that getifaddrs is commonly available: it's available on all recent versions of Linux, as well as on FreeBSD and MacOS. There recent versions of Linux as of which year? Good question. When I was writing this code in 2003, the stable version of Debian didn't support it. I haven't seen a Linux system that doesn't support it since then, but I've only checked periodically. might be some platform somewhere that's disenfranchised by this, but I'm not sure it's worth the code complexity to maintain the current code. What about Solaris/OpenSolaris? That's the only other major system where Wine can be made to run. Ah. Okay, that's an important objection. I'll go with keeping the current code, then. Thanks, --Juan
Re: Antw.: RFC: depending on getifaddrs in iphlpapi
Why? AFAIK getifaddr was added to at least openSolaris Last year and you still can use the old Code as fallback Yes, by keeping the current code I meant alongside a getifaddrs-based implementation. --Juan
Re: [PATCH 4/4] kernel32, ntdll: implement most of GetNamedPipeHandleState
Hi Bernhard, nit: -todo_wine -ok(!ret GetLastError() == ERROR_INVALID_PARAMETER, +todo_wine ok(!ret GetLastError() == ERROR_INVALID_PARAMETER, This change is a no-op. --Juan
Re: todo_wine, broken() and a SW anti-pattern
Hi Joerg, hr = IAudioClient_GetService(ac, IID_IAudioStreamVolume, (void**)out); ok(hr == AUDCLNT_E_NOT_INITIALIZED, ... call returns %08x\n, hr); todo_wine ok(broken(out != NULL), GetService %08x out pointer %p\n, hr, out); 1. broken() documents that I consider native's observable behaviour broken because I judge it unsafe not to zero the out pointer in case of failure. Furthermore, MSDN documents that it should be zeroed. http://msdn.microsoft.com/en-us/library/windows/desktop/dd370873%28v=vs.85%29.aspx I think, philosophically, you're correct. On the other hand, Wine isn't meant to be a philosophically correct implementation of the Win32 API. And, the whole point of regression tests is that MSDN isn't to be trusted. So, while Microsoft says that the out pointer must be NULL on failure, both in this case and more generally for COM (specifically for QueryInterface), our guide has always been to trust what Microsoft does more than what it says. Our guideline for using broken() is to mark as broken the things that Microsoft has changed. Older implementations often had bugs that have subsequently been fixed, so we mark those as broken and conform to the newer behavior. On the other hand, if Windows has always had a specific behavior, than that behavior is, by definition, correct. broken() isn't appropriate in this case. Again, we must do as Microsoft does, not as it says. So I believe the correct behavior here is not to use broken(), irrespective of what MSDN says. A comment stating that MSDN is incorrect might be in order. --Juan
Re: Governance of Wine with respect to the Software Freedom Conservancy (update October 2011)
Hi Michael, Not that I have any problems with our benevolent overlords, and not that I would likely achieve franchise with a scant 2 patches under my belt, but I can't help wondering how such a revolt would succeed seeing as the only method to achieve franchise-hood is controlled by the same people one would be revolting against. You are new around here, we bottom-post ;) Not true, of course. Alexandre is at the top of the list of contributors, naturally, but he doesn't constitute a controlling majority. (I'm discounting Jeremy's contributions, which is correct within tolerable error snarky grin.) --Juan
Re: Governance of Wine with respect to the Software Freedom Conservancy (update October 2011)
As I said, our overlords are kind and benevolent and I'm sure that the mention of evil plans was simply a joke as such wise and noble developers could need harbor a malevolent thought. But, unless I've been misreading this mailing list, all patches have to go through our current enlightened leader before becoming part of the patch count in the wine tree. Not that the powers that be are susceptible to temptation, but lesser mortals might find that being more selective about whose patches are accepted during periods of discontent as an easy way to influence such a vote. Likewise, even if such a mortal didn't give into temptation, if the usurpers lose the vote they could always claim such impropriety did take place. My point is that the math isn't in your argument's favor. It would take a long period of rejection by the current overlords before being able to squelch any hypothetical usurpers, given that a) the current overlords' contributions consist of Alexandre's, and b) he does not constitute a controlling majority of contributions, nor anywhere close to it. --Juan
Re: [1/4] cmd: Avoid checking handle type when already known in WCMD_ReadFile
OK but the purpose is to avoid checking the handle type for every line read. Granted, one could use '((DWORD_PTR)h) 3 == 3' instead of GetConsoleMode or similar function. (there's currently code like BOOL is_console = GetConsoleMode(...); ...; while WCMD_fgets(..., is_console) and the handle type shouldn't change between lines/iterations, so why bother recomputing it every time?) You're probably not. The compiler will probably inline this function, or at least its return value, since it knows it's guaranteed not to change. Please don't do the compiler's work for it: IMHO, it's more readable to make an inline function to determine whether the handle is a console handle. --Juan
crypt32(1/2): Test CertCreateCertificateContext, and fix an error code in a failure case
--Juan
From 544fb76b6da191c8e5c665be11357706acae9be0 Mon Sep 17 00:00:00 2001
From: Juan Lang juan.l...@gmail.com
Date: Thu, 22 Sep 2011 05:20:50 -0700
Subject: [PATCH 17/18] Test CertCreateCertificateContext, and fix an error code in a failure case
---
dlls/crypt32/cert.c |6 +
dlls/crypt32/tests/cert.c | 46 +
2 files changed, 52 insertions(+), 0 deletions(-)
diff --git a/dlls/crypt32/cert.c b/dlls/crypt32/cert.c
index 75b0e12..0117f27 100644
--- a/dlls/crypt32/cert.c
+++ b/dlls/crypt32/cert.c
@@ -141,6 +141,12 @@ PCCERT_CONTEXT WINAPI CertCreateCertificateContext(DWORD dwCertEncodingType,
TRACE((%08x, %p, %d)\n, dwCertEncodingType, pbCertEncoded,
cbCertEncoded);
+if ((dwCertEncodingType CERT_ENCODING_TYPE_MASK) != X509_ASN_ENCODING)
+{
+SetLastError(E_INVALIDARG);
+return NULL;
+}
+
ret = CryptDecodeObjectEx(dwCertEncodingType, X509_CERT_TO_BE_SIGNED,
pbCertEncoded, cbCertEncoded, CRYPT_DECODE_ALLOC_FLAG, NULL,
certInfo, size);
diff --git a/dlls/crypt32/tests/cert.c b/dlls/crypt32/tests/cert.c
index e338740..9e7104a 100644
--- a/dlls/crypt32/tests/cert.c
+++ b/dlls/crypt32/tests/cert.c
@@ -634,6 +634,51 @@ static void testCertProperties(void)
CertFreeCertificateContext(context);
}
+static void testCreateCert(void)
+{
+PCCERT_CONTEXT cert, enumCert;
+DWORD count, size;
+BOOL ret;
+
+SetLastError(0xdeadbeef);
+cert = CertCreateCertificateContext(0, NULL, 0);
+ok(!cert GetLastError() == E_INVALIDARG,
+ expected E_INVALIDARG, got %08x\n, GetLastError());
+SetLastError(0xdeadbeef);
+cert = CertCreateCertificateContext(0, selfSignedCert,
+ sizeof(selfSignedCert));
+ok(!cert GetLastError() == E_INVALIDARG,
+ expected E_INVALIDARG, got %08x\n, GetLastError());
+SetLastError(0xdeadbeef);
+cert = CertCreateCertificateContext(X509_ASN_ENCODING, NULL, 0);
+ok(!cert
+ (GetLastError() == CRYPT_E_ASN1_EOD ||
+ broken(GetLastError() == OSS_MORE_INPUT /* NT4 */)),
+ expected CRYPT_E_ASN1_EOD, got %08x\n, GetLastError());
+
+cert = CertCreateCertificateContext(X509_ASN_ENCODING,
+ selfSignedCert, sizeof(selfSignedCert));
+ok(cert != NULL, creating cert failed: %08x\n, GetLastError());
+/* Even in-memory certs are expected to have a store associated with them */
+todo_wine
+ok(cert-hCertStore != NULL, expected created cert to have a store\n);
+/* The cert doesn't have the archived property set (which would imply it
+ * doesn't show up in enumerations.)
+ */
+size = 0;
+ret = CertGetCertificateContextProperty(cert, CERT_ARCHIVED_PROP_ID,
+ NULL, size);
+ok(!ret GetLastError() == CRYPT_E_NOT_FOUND,
+ expected CRYPT_E_NOT_FOUND, got %08x\n, GetLastError());
+/* Strangely, enumerating the certs in the store finds none. */
+enumCert = NULL;
+count = 0;
+while ((enumCert = CertEnumCertificatesInStore(cert-hCertStore, enumCert)))
+count++;
+ok(!count, expected 0, got %d\n, count);
+CertFreeCertificateContext(cert);
+}
+
static void testDupCert(void)
{
HCERTSTORE store;
@@ -3634,6 +3679,7 @@ START_TEST(cert)
testAddCert();
testCertProperties();
+testCreateCert();
testDupCert();
testFindCert();
testGetSubjectCert();
--
1.7.1
Re: Old imagehlp patches
Hi Thomas, wrong mailing list. You wanted wine-devel instead (now cc'ed.) I don't know about wine development, but are these patches still likely to work? What would need to happen for them to be merged in? To your first question, try them and see! To your second question, a lot. They're much too large to be merged at a single go, they lack tests, and they should be sent by their original author. I wouldn't hold your breath expecting these patches to go in as-is: if they weren't good enough in 2004-5, they're not any better now. You'd want someone who has an active interest in this area to have a go at them, I believe, unless you can convince the original author to take them up again. --Juan
crypt32: Test/correct CertGetNameString with NULL pvTypePara
Fix for bug 23287.
--Juan
From 1114085290c2ada360a02bae154b7167a0f2b0ca Mon Sep 17 00:00:00 2001
From: Juan Lang juan.l...@gmail.com
Date: Thu, 8 Sep 2011 11:10:08 -0700
Subject: [PATCH 2/2] Test/correct CertGetNameString with NULL pvTypePara
---
dlls/crypt32/str.c | 18 -
dlls/crypt32/tests/str.c | 162 ++
2 files changed, 176 insertions(+), 4 deletions(-)
diff --git a/dlls/crypt32/str.c b/dlls/crypt32/str.c
index 7444575..6d406d2 100644
--- a/dlls/crypt32/str.c
+++ b/dlls/crypt32/str.c
@@ -1209,8 +1209,11 @@ static DWORD cert_get_name_from_rdn_attr(DWORD
encodingType,
if (CryptDecodeObjectEx(encodingType, X509_NAME, name-pbData,
name-cbData, CRYPT_DECODE_ALLOC_FLAG, NULL, nameInfo, bytes))
{
-PCERT_RDN_ATTR nameAttr = CertFindRDNAttr(oid, nameInfo);
+PCERT_RDN_ATTR nameAttr;
+if (!oid)
+oid = szOID_RSA_emailAddr;
+nameAttr = CertFindRDNAttr(oid, nameInfo);
if (nameAttr)
ret = CertRDNValueToStrW(nameAttr-dwValueType, nameAttr-Value,
pszNameString, cchNameString);
@@ -1229,6 +1232,9 @@ DWORD WINAPI CertGetNameStringW(PCCERT_CONTEXT
pCertContext, DWORD dwType,
TRACE((%p, %d, %08x, %p, %p, %d)\n, pCertContext, dwType,
dwFlags, pvTypePara, pszNameString, cchNameString);
+if (!pCertContext)
+goto done;
+
if (dwFlags CERT_NAME_ISSUER_FLAG)
{
name = pCertContext-pCertInfo-Issuer;
@@ -1268,9 +1274,12 @@ DWORD WINAPI CertGetNameStringW(PCCERT_CONTEXT
pCertContext, DWORD dwType,
break;
}
case CERT_NAME_RDN_TYPE:
+{
+DWORD type = pvTypePara ? *(DWORD *)pvTypePara : 0;
+
if (name-cbData)
ret = CertNameToStrW(pCertContext-dwCertEncodingType, name,
- *(DWORD *)pvTypePara, pszNameString, cchNameString);
+ type, pszNameString, cchNameString);
else
{
CERT_ALT_NAME_INFO *info;
@@ -1279,12 +1288,12 @@ DWORD WINAPI CertGetNameStringW(PCCERT_CONTEXT
pCertContext, DWORD dwType,
if (entry)
ret = CertNameToStrW(pCertContext-dwCertEncodingType,
- entry-u.DirectoryName, *(DWORD *)pvTypePara, pszNameString,
- cchNameString);
+ entry-u.DirectoryName, type, pszNameString, cchNameString);
if (info)
LocalFree(info);
}
break;
+}
case CERT_NAME_ATTR_TYPE:
ret = cert_get_name_from_rdn_attr(pCertContext-dwCertEncodingType,
name, pvTypePara, pszNameString, cchNameString);
@@ -1413,6 +1422,7 @@ DWORD WINAPI CertGetNameStringW(PCCERT_CONTEXT
pCertContext, DWORD dwType,
FIXME(unimplemented for type %d\n, dwType);
ret = 0;
}
+done:
if (!ret)
{
if (!pszNameString)
diff --git a/dlls/crypt32/tests/str.c b/dlls/crypt32/tests/str.c
index cbfdf4d..1443597 100644
--- a/dlls/crypt32/tests/str.c
+++ b/dlls/crypt32/tests/str.c
@@ -199,6 +199,8 @@ typedef BOOL (WINAPI *CertStrToNameAFunc)(DWORD
dwCertEncodingType,
typedef BOOL (WINAPI *CertStrToNameWFunc)(DWORD dwCertEncodingType,
LPCWSTR pszX500, DWORD dwStrType, void *pvReserved, BYTE *pbEncoded,
DWORD *pcbEncoded, LPCWSTR *ppszError);
+typedef DWORD (WINAPI *CertGetNameStringAFunc)(PCCERT_CONTEXT cert, DWORD type,
+ DWORD flags, void *typePara, LPSTR str, DWORD cch);
static HMODULE dll;
static CertNameToStrAFunc pCertNameToStrA;
@@ -208,6 +210,7 @@ static CertRDNValueToStrAFunc pCertRDNValueToStrA;
static CertRDNValueToStrWFunc pCertRDNValueToStrW;
static CertStrToNameAFunc pCertStrToNameA;
static CertStrToNameWFunc pCertStrToNameW;
+static CertGetNameStringAFunc pCertGetNameStringA;
static void test_CertRDNValueToStrA(void)
{
@@ -933,6 +936,162 @@ static void test_CertStrToNameW(void)
}
}
+static void test_CertGetNameStringA(void)
+{
+PCCERT_CONTEXT context;
+
+if (!pCertGetNameStringA)
+{
+win_skip(CertGetNameStringA is not available\n);
+return;
+}
+
+context = CertCreateCertificateContext(X509_ASN_ENCODING, cert,
+ sizeof(cert));
+ok(context != NULL, CertCreateCertificateContext failed: %08x\n,
+ GetLastError());
+if (context)
+{
+static const char aric[] = a...@codeweavers.com;
+static const char localhost[] = localhost;
+DWORD len, type;
+LPSTR str;
+
+/* Bad string types/types missing from the cert */
+len = pCertGetNameStringA(NULL, 0, 0, NULL, NULL, 0);
+ok(len == 1, expected 1, got %d\n, len);
+len = pCertGetNameStringA(context, 0, 0, NULL, NULL, 0);
+ok(len == 1, expected 1, got %d\n, len);
+len = pCertGetNameStringA(context, CERT_NAME_URL_TYPE, 0, NULL, NULL,
+ 0);
+ok(len == 1, expected 1, got %d\n, len);
+
+len = pCertGetNameStringA(context, CERT_NAME_EMAIL_TYPE, 0, NULL, NULL,
+ 0
Re: RFC - ws2_32/socket.c: SIO_KEEPALIVE_VALS vs TCP_KEEPIDLE/TCP_KEEPINTVL
Hi Bruno, The SIO_KEEPALIVE_VALS message on windows takes parameters as miliseconds, on some other systems there are the equivalent TCP_KEEPIDLE/TCP_KEEPINTVL which takes the parameters as seconds. To solve this there is a division by 1000 on wine source code but the problem is that values 1000 become 0 making the setsockopt function return error. Is it reasonable to do a check to see if the result of the division is zero and then set the variable to one so we can get closer to the windows implementation and not fail the setsockopt function? It will Yes, it's reasonable. I think API-level compatibility is generally more important to appilcations than timing-level compatibility. I.o.w. function(args) - result should be invariant across Windows and Wine. The time required to before function(args) yields result is oftentimes not guaranteed in Windows, so a little fuzziness here is often okay. --Juan
Re: mshtml: Use the last colon in proxy url as port separator
just sent a patch doing that. Looks good to me, thanks! --Juan
Re: mshtml: Use the last colon in proxy url as port separator
Is the ProxyServer specified as an URL? If yes then use the proper API to dissect the URL instead of cobbling something together. Just to follow up in this idea, you could use InternetCrackUrl. mshtml delay loads wininet, but it also loads urlmon, which also loads wininet, so in effect wininet is always loaded and available. But a larger question is, why load the proxy settings from the registry only? If you query wininet for them instead, then the environment variable http_proxy is also checked. There was bug 5625 about this, which I set to fixed based on commit 80f02b82d68902f32578a7bcf6cfbaa715b724ce. I may have been mistaken. Under what circumstances is the network.proxy.http gecko setting still being used? Maybe for embedded content, e.g. IMG tags? If these were set by querying wininet instead, then at least we wouldn't have potentially different proxy settings for some HTTP requests than for others. --Juan
mshtml: Use the last colon in proxy url as port separator
Hi André, http://domain\user:passw...@server.com:8080 makes it visible that it's intended to get the last colon What about http://domain\user:passw...@server.com? password isn't the port number. --Juan
Re: Best way to get Program Files/Common Files?
I need to get the user configured path to Program Files/Common Files to save the dinput action mapping settings. What are the my options for doing this? One way I found is the SHGetSpecialFolderPath function, but it requires me to link to shell32 and I suspect there's a better way. I believe this is the preferred way, at least from Microsoft's point of view. --Juan
Re: Possible off-by-1 in crypt32/chain.c match_common_names
Hi William, trace:chain:match_common_name CN = L*.battle.net\ warn:chain:match_domain_component domain component Lnet too short for Lnet\ That CN is coming from the certificate. Any thoughts or ideas on whether this is actually a bug and if so, how to fix it? It's partly a bug in the certificate. x.509 states that strings do not include a terminating NULL. Some providers erroneously include one anyway. Whether it's a bug in Wine depends on what Microsoft does. It's not an off-by-one bug, though: the code is working as intended, for the moment. As a workaround, one might check all names twice: once without the terminating NULL (preferred), and once with it. You wouldn't want to use strlenW on the names in the certificates. Actually doing so throughout the code will be hard, unfortunately. --Juan
Re: NtQuerySystemInformation needs a little bit more flesh for SystemProcessorPerformanceInformation
This Reserved1[0] is the DPC Time but I'm wondering what values from /proc/stat to use here (remainder[0] is I/O wait). We could always make up something of course but being accurate would be nicer. DPC Time has no equivalent in /proc/stat. It's an NT thing. Making something up is the only appropriate thing, unless we start doing something special with DPCs someday. --Juan
Re: gsoc mentor summit
I'd be interested if there's a space going free. We just get to send 2 mentors though as I understand it? Yes. I'd be interested as well, that's why I started this thread. I've been there already in 2009, so if anyone else wants to go I'd be happy to yield. Whoever does go, I'll be happy to meet. I'm just down the road at Google. --Juan
Re: [PATCH 02/12] wscript: added helper function for tests
Hi Michal, please combine this with patch 3, it doesn't make sense on its own. --Juan
Re: [PATCH 2/2] mshtml: Mark some functions as cdecl.
Hi Jacek, These are functions that I missed when revieweing Gecko headers. I will probably change their calling convention in the next Gecko release. In that case, why not change the calling convention now? The matrix of possible configurations is much simpler that way. Right now, we have: current Wine + current gecko = crash By changing the calling convention in Wine, we have: Wine = 1.3.23 + current gecko = crash Wine 1.3.24-?? + current gecko = no crash Wine ?? + current gecko = crash Wine ?? + new gecko = no crash Wine = 1.3.23 + new gecko = crash By doing a new gecko release, we have: any Wine + current gecko = crash any Wine + new gecko = no crash --Juan
Re: [PATCH 2/2] mshtml: Mark some functions as cdecl.
BTW, the current plan is to do the new Gecko release together with Firefox 6 release, which is about 6 weeks from now. The first beta version should be out in a week or two, depending on how Firefox beta will behave. Then shouldn't the beta have the calling convention change anyway? In this case, it doesn't make sense to me to change Wine for just a few days. --Juan
Re: [PATCH 1/6] kernel32: Fix PROFILE_Load (try 4)
Hi Christian, this isn't quite what Alexandre meant. *What* did you fix? Saying you did so isn't enough. Thanks, --Juan
Re: Glitch-free iTunes?
Hi Keith, WINE in general does many things, and you've got a big team now. Your contributor list shows 1250 and you've got 2M lines of code. Few of these contributors remain active. Most had small contributions. The total number of contributors obviously can only increase over time, but the number of active contributors has always remained small. But really, what are we talking about? Talk is cheap. Code isn't. Your contributions are welcome. --Juan
Re: [PATCH 1/4] shell32: Implement erasing and restoring items from the trash
Hi Jay,
+HRESULT TRASH_RestoreItem(LPCITEMIDLIST pidl){
Nit: the brace should be on its own line.
+HRESULT TRASH_RestoreItem(LPCITEMIDLIST pidl) DECLSPEC_HIDDEN;
+HRESULT TRASH_EraseItem(LPCITEMIDLIST pidl) DECLSPEC_HIDDEN;
These two functions are never called in this patch, so you're
introducing dead code. That's not allowed. Introduce the functions
in the patch that uses them, please. If the resulting patch is too
large, you must split it another way.
--Juan
Re: gcc 4.6 warning report
David is right, the address is not stored as a pointer but as a DWORD in place of the chars. Like this: gethostbyname(winehq.org): wrong: (DWORD) host-h_addr_list[i] = 0x00cbd1c8 = 200.209.203.0 right: *(DWORD*) host-h_addr_list[i] = 0x86192ed1 = 209.46.25.134 Patch welcome ;) --Juan
Re: gcc 4.6 warning report
../../../dlls/netapi32/nbt.c:580:30: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] This is a false positive. h_addr_list is declared as a char **, and technically it is, but gethostbyname only returns IPv4 addresses, i.e. ones that can fit in a DWORD. --Juan
Re: gcc 4.6 warning report
This is a false positive. h_addr_list is declared as a char **, and technically it is, but gethostbyname only returns IPv4 addresses, i.e. ones that can fit in a DWORD. That doesn't sound like a problem that would give that warning. Why not? A cast of a pointer to a DWORD with 64-bit pointers would cause truncation, if it were indeed a pointer that were being cast. It isn't, even though it's declared that way. --Juan
Re: [PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)
Hi Lauri, ok(pdst != NULL, inet_ntoa failed %s\n, dst); -ok(!strcmp(pdst, addr0_Str),Address %s != %s\n, pdst, addr0_Str); +ok(pdst !strcmp(pdst, addr0_Str),Address %s != %s\n, pdst, addr0_Str); This change doesn't accomplish anything. In the first place, the previous ok which you did not change already ensures pdst is not NULL. While it's true that this ok could fail, leading to a following crash in the tests, the fact that the tests pass indicates this isn't happening. Second, the ok output message also dereferences pdst by printing it, so in the case that it is NULL, you haven't fixed anything. Really, I suggest you just ignore this warning, it's not worth the extra baggage. --Juan
Re: [PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)
It would be a lot easier to find and fix real things if there weren't a thousand false ones hanging around. But if you feel it's better to have a lot of warnings and some possible bugs than a lot of checks and no bugs, then maybe I'll not waste any more time fixing them. This is a false dichotomy. LLVM/Clang uses static analysis, which is a sound technique, i.e. it produces no false negatives. The downside is that it will always produce false positives. Filtering out false positives is a necessary component of any bug fixing exercise using static analysis. I'd argue that this is one such example, that the fix isn't worth the noise, as there's no actual bug being fixed. --Juan
Re: TaskDialog implementation
Hi Patrick, As this is quite a big amount of work, I would like to have some guidance (what must I do to be sure it is accepted) so that I do not end up doing all that for nothing - starting by knowing why my patch from yesterday was not accepted (what did I do wrong). The patch was marked as Pending. That usually means Alexandre's waiting for something, e.g. for you to fix something obvious, or to see what else you're planning to do. Since you say the patch is preparatory work, it doesn't make much sense to go in unless the remaining patches are also to go in, so I'd suggest sending at least another patch to show where you're going with it. Regarding splitting, sometimes it's useful to introduce code that will only be removed later. I know this isn't the usual advice, but if you introduce a new implementation of something, sometimes a stub implementation can appear first. E.g., if it's an interface you're implementing, introduce an implementation that does nothing but return E_NOTIMPL from each method. Then one by one introduce implementation for each function. Hope that helps, --Juan
Re: setupapi: Make sure machine name is non-empty before failing
What's wrong with 'if (MachineName *MachineName)' ? Nothing, it's my brain that's lacking. Thanks. --Juan
Re: [1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)
Hi Adam, perhaps I'm just being obtuse, but I don't see how Dmitry's comment has been addressed for this patch. I'm confused by the commit comment (Emulate Win9x if appropriate) and the change itself: /* Win98 returns only the swapsize in ullTotalPageFile/ullAvailPageFile, WinXP returns the size of physical memory + swapsize; - mimic the behavior of XP. You are removing the part of the comment explaining what the code actually does, which I find confusing. If you're part of the the code is the documentation crowd, just remove the entire comment. If not, please add a brief explanation. Note: Project2k refuses to start if it sees less than 1Mb of free swap. */ -lpmemex-ullTotalPageFile += lpmemex-ullTotalPhys; -lpmemex-ullAvailPageFile += lpmemex-ullAvailPhys; - -/* Titan Quest refuses to run if TotalPageFile = ullTotalPhys */ -if(lpmemex-ullTotalPageFile == lpmemex-ullTotalPhys) +if (osver.dwMajorVersion = 5 || osver.dwPlatformId != VER_PLATFORM_WIN32_WINDOWS) This change is confusing to me. dwPlatformId == VER_PLATFORM_WIN32_NT for NT4, Win2k, XP, etc. dwMajorVersion is never greater than 4 on Windows 9x. In other words, do you mean to exclude NT4 here, or not? If so, then the first check should suffice. If not, then the second check should. But in either case, what does this have to do with being appropriate for Win9x emulation? --Juan
