Hi Michael, that was quick! Okay, more feedback:
1. English note: destruct_handle_table really should be destroy_handle_table. I know, "destructor" came into common use, but the verb is to destroy. Same with destruct_key, destruct_hash, etc.
2. A CSP isn't allocating a "real" handle, it's sort of a pseudohandle, so the multiple-of-four business isn't necessary. When I was testing SSPs, I found some of them return 0 as a valid handle value. So I think using the index directly would be fine.
3. Do you really need to separate all the functions into an impl version and a non-impl version? It looks as though the non-impl version does the locking, while the impl assumes the table is locked. It seems you could at least document this, and it also doesn't seem necessary in every case, if at all--EnterCriticalSection won't deadlock if a thread that's already entered it calls it again.
4. Some of the OpenSSL checks don't seem valid. In init_hash_impl, for example, you fail if libcrypto isn't available, even if it isn't necessary for the hash. Please fail only if you really need an OpenSSL func. E.g., if (!pMD2_Init) { SetLastError()... } seems better to me.
5. This may be open to debate, but as far as a "more elegant way": perhaps you should protect each typedef with the header file that defines it, e.g. in HASH_CONTEXT, rather than: #ifdef HAVE_OPENSSL MD2_CTX md2; #endif you could use: #ifdef HAVE_OPENSSL_MD2_H MD2_CTX md2; #endif This way you could support the algorithms you were compiled with, even if they aren't the entire set.
Sorry I didn't mention these last time.. --Juan
__________________________________ Do you Yahoo!? Yahoo! Mail Address AutoComplete - You start. We finish. http://promotions.yahoo.com/new_mail
On Wednesday 27 October 2004 03:33, Juan Lang wrote:
- English note: destruct_handle_table really should be
destroy_handle_table. I know, "destructor" came into common use, but the verb is to destroy. Same with destruct_key, destruct_hash, etc.
Sure, no problem.
- A CSP isn't allocating a "real" handle, it's sort of a pseudohandle, so
the multiple-of-four business isn't necessary. When I was testing SSPs, I found some of them return 0 as a valid handle value. So I think using the index directly would be fine.
I would rather keep this, since it isn't really a performance problem. Perhaps sometime somebody else will use the code with "real" handles. This handle managing code is actually another example where I would love to have a library, which would be linked statically by multiple dlls. There are at least three distinct handle table implementations in wine.
- Do you really need to separate all the functions into an impl version
and a non-impl version? It looks as though the non-impl version does the locking, while the impl assumes the table is locked. It seems you could at least document this, and it also doesn't seem necessary in every case, if at all--EnterCriticalSection won't deadlock if a thread that's already entered it calls it again.
When I wrote the code I thought a double call to EnterCriticalSection on the same mutex might deadlock. I later found out that I doesn't, but thought that it might be faster to avoid multiple calls to EnterCriticalSection, since those call wineserver. Is this the case? Otherwise, I will be happy to remove the _impl functions.
- Some of the OpenSSL checks don't seem valid. In init_hash_impl, for
example, you fail if libcrypto isn't available, even if it isn't necessary for the hash. Please fail only if you really need an OpenSSL func. E.g., if (!pMD2_Init) { SetLastError()... } seems better to me.
Are you saying that we should consider the case where we actually have a libcrypto.so at runtime, but which only exports a subset of the functions rsaenh was compiled with?
- This may be open to debate, but as far as a "more elegant way": perhaps
you should protect each typedef with the header file that defines it, e.g. in HASH_CONTEXT, rather than: #ifdef HAVE_OPENSSL MD2_CTX md2; #endif you could use: #ifdef HAVE_OPENSSL_MD2_H MD2_CTX md2; #endif This way you could support the algorithms you were compiled with, even if they aren't the entire set.
Now, this would mean we are considering the case, where we only have a subset of the libssl-dev headers at compile time, right?
Thanks for your comments, Michael
--- Michael Jung mjung@iss.tu-darmstadt.de wrote:
- A CSP isn't allocating a "real" handle, it's sort of a
pseudohandle, so
the multiple-of-four business isn't necessary. When I was testing
SSPs, I
found some of them return 0 as a valid handle value. So I think using
the
index directly would be fine.
I would rather keep this, since it isn't really a performance problem. Perhaps sometime somebody else will use the code with "real" handles.
No, this won't be the case. By real handle I mean one which is sharable between processes, is refcounted by the wineserver, etc. That isn't the case with the Crypto API handles. So no, it doesn't cause a performance problem, but it is confusing.
This handle managing code is actually another example where I would love to have a library, which would be linked statically by multiple dlls.
I agree that one should be available. Robert Shearman posted a patch to add handle tables to ntdll: http://www.winehq.org/hypermail/wine-patches/2004/10/0339.html It isn't committed yet, but it might be a usable replacement. It depends how much you need the features it doesn't provide.
When I wrote the code I thought a double call to EnterCriticalSection on the same mutex might deadlock. I later found out that I doesn't, but thought that it might be faster to avoid multiple calls to EnterCriticalSection, since those call wineserver.
It doesn't appear as though critical sections involve the wine server. Take a look at ntdll/critsection.c and RtlEnterCriticalSection.
- Some of the OpenSSL checks don't seem valid. In init_hash_impl,
for
example, you fail if libcrypto isn't available, even if it isn't
necessary
for the hash. Please fail only if you really need an OpenSSL func.
E.g.,
if (!pMD2_Init) { SetLastError()... } seems better to me.
Are you saying that we should consider the case where we actually have a libcrypto.so at runtime, but which only exports a subset of the functions rsaenh was compiled with?
No, in this case I'm pointing out that most of the hash functions don't require OpenSSL at all (MD4, MD5, and SHA1), so why should they be prevented from working if it isn't available? Right now the check is done at the entry to the function, so all will fail if libcrypto.so isn't there. In my above example I move the check to the function that depends on libcrypto.so.
Now, this would mean we are considering the case, where we only have a subset of the libssl-dev headers at compile time, right?
Right, or that the OpenSSL you compiled against had some of the algorithms disabled. The check as you have it will prevent MD2 from working if DES wasn't available at compile time, and vice-versa. There's no guarantee how OpenSSL was configured.
A similar comment, that is also open to debate, is the implementation of gen_rand_impl. I know, rsabase already does it this way.. but it might be better to try to use /dev/random instead of RAND_bytes, since that will work on many platforms whether or not OpenSSL is available.
--Juan
__________________________________ Do you Yahoo!? Yahoo! Mail Address AutoComplete - You start. We finish. http://promotions.yahoo.com/new_mail
-
Juan Lang -
Michael Jung