[PATCH] secur32: Split push/pull schannel transports

[Anton Romanov]

>>> 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).
Well, my point is that adding CS there would introduce contention in a
place where app tries to avoid it.


>>> 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
>>>
>>> [1] https://gnutls.org/manual/html_node/Thread-safety.html
>> 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.
It does say that there is locking needed for libcurl's general stuff.
I was talking about TLS section in that page specifically. But sure, I
might be reading it wrong.

So, anyway, would adding CS just for SecureTransport code path be fine?