Re: [PATCH 1/2] secur32: Split push/pull schannel transports

[Jacek Caban]

On 13.09.2017 07:33, Anton Romanov wrote:
> This makes schan_EncryptMessage/schan_DecryptMessage thread safe between
> each other.
> MSDN states that it's supposed to be thread safe.
> Fixes Magic The Gathering: Online crash:
> https://bugs.winehq.org/show_bug.cgi?id=43453
>
> Signed-off-by: Anton Romanov <theli.ua(a)gmail.com>
> ---
>  dlls/secur32/schannel.c        | 33 +++++++++++++++++++--------------
>  dlls/secur32/schannel_gnutls.c | 12 +++++++-----
>  dlls/secur32/schannel_macosx.c | 21 ++++++++++++---------
>  dlls/secur32/secur32_priv.h    |  5 +++--
>  4 files changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c
> index 82374efd55..1f938e1a37 100644
> --- a/dlls/secur32/schannel.c
> +++ b/dlls/secur32/schannel.c
> @@ -59,6 +59,8 @@ struct schan_context
>      schan_imp_session session;
>      ULONG req_ctx_attr;
>      const CERT_CONTEXT *cert;
> +    struct schan_transport *push_transport;
> +    struct schan_transport *pull_transport;

Do you really need separated transports? schan_transport already has
separated in and out buffers. It seems to me that as long as you use
only in buffers in reader and out buffers in writer (and it's already
the case), there is no need for separated transport instances.

Also, why do you use pointers here? Couldn't you just store the struct
inside schan_context?
> diff --git a/dlls/secur32/schannel_gnutls.c b/dlls/secur32/schannel_gnutls.c
> index bc3bbaf67d..97b2c2d767 100644
> --- a/dlls/secur32/schannel_gnutls.c
> +++ b/dlls/secur32/schannel_gnutls.c
> @@ -73,7 +73,7 @@ MAKE_FUNCPTR(gnutls_record_send);
>  MAKE_FUNCPTR(gnutls_server_name_set);
>  MAKE_FUNCPTR(gnutls_transport_get_ptr);
>  MAKE_FUNCPTR(gnutls_transport_set_errno);
> -MAKE_FUNCPTR(gnutls_transport_set_ptr);
> +MAKE_FUNCPTR(gnutls_transport_set_ptr2);

It's related to the previous comment, it seems to me that
gnutls_transport_set_ptr should be enough.

Thanks,
Jacek

[PATCH 1/2] secur32: Split push/pull schannel transports

[Anton Romanov]

> Do you really need separated transports? schan_transport already has
> separated in and out buffers. It seems to me that as long as you use only in
> buffers in reader and out buffers in writer (and it's already the case),
> there is no need for separated transport instances.
>
> Also, why do you use pointers here? Couldn't you just store the struct
> inside schan_context?
> I don't really remember why I did it like this anymore. I will send new version.