On 09.09.2017 02:24, Anton Romanov wrote:
Jacek,
The code looks generally good and it matches what MSDN states about thread safety of those functions. Still, having little trust to MSDN, I wonder if it would make sense to make them more generally thread safe. We could, for example, store critical section inside schan_context and lock it in all functions accessing it. Did you consider that? It would make all those functions thread safe (with little overhead). It just seems more straightforward solution to me.
Well, I'd say that would just introduce additional complexity without much/any gain. I mean - if MSDN does not state other use is supposed to be safe - why would we try and make it? Unless there is an app that would actually benefit that.
Yeah, making it all may be questionable, but the solution does not really add complexity. A simple enter/leave critical section inside schan_EncryptMessage and schan_DecryptMessage would fix the problem that you're trying to fix. It would also avoid dependency on backend libraries to do the right thing (see more below).
As for the approach you took, it may be correct. According to [1] that's also how gnutls works. Do you know if Secure Transport on Mac is similar? I couldn't find any documentation about it.
Thanks, Jacek
Unfortunately I am not entirely sure if SecureTransport is (supposed to be) thread-safe/unsafe either... Yeah, documentation does not really clarify that. Source code [2] is not too clear either.
Actually, looking at the source, I don't see any evidence it allows what we need. SSLRead/SSLWrite seem to freely access context without any locking and call functions like SSLHandshakeProceed, which accesses both read and write related fields. That said, I don't think it guarantees what we need.
However, libcurl (which I would trust) states that it is using Secure Transport in an "entirely thread safe way" [3],
That page says the opposite. It says that if you share a handle between threads, you need to guaranty that it's not accessed concurrently yourself. What we need is read/write threads sharing a handle.
and I don't see it making any locks when doing sslread/write [4].
[2] https://opensource.apple.com/source/Security/Security-55179.13/libsecurity_s... [3] https://curl.haxx.se/libcurl/c/threadsafe.html [4] https://github.com/curl/curl/blob/master/lib/vtls/darwinssl.c
Cheers,
Jacek