[OpenWrt-Devel,2/3] Fix SSL negotiation being interrupted by .notify_write from BIO method.
diff mbox

Message ID 1415702041-44573-2-git-send-email-yszhou4tech@gmail.com
State Changes Requested
Headers show

Commit Message

Yousong Zhou Nov. 11, 2014, 10:34 a.m. UTC
ustream_ssl_check_conn() may be called by .notify_write while a previous
SSL_connect() is still in process.  This can happen because the
.notify_write callback will may be triggered by writes in the BIO
methods.

Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
---
 ustream-ssl.c |   19 +++++++++++++++----
 ustream-ssl.h |    1 +
 2 files changed, 16 insertions(+), 4 deletions(-)

Comments

Felix Fietkau Dec. 11, 2014, 4:42 p.m. UTC | #1
On 2014-11-11 11:34, Yousong Zhou wrote:
> ustream_ssl_check_conn() may be called by .notify_write while a previous
> SSL_connect() is still in process.  This can happen because the
> .notify_write callback will may be triggered by writes in the BIO
> methods.
> 
> Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
> ---
>  ustream-ssl.c |   19 +++++++++++++++----
>  ustream-ssl.h |    1 +
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/ustream-ssl.c b/ustream-ssl.c
> index dd0faf9..84104b0 100644
> --- a/ustream-ssl.c
> +++ b/ustream-ssl.c
> @@ -34,12 +34,22 @@ static void ustream_ssl_error_cb(struct uloop_timeout *t)
>  		us->notify_error(us, error, __ustream_ssl_strerror(us->error, buffer, sizeof(buffer)));
>  }
>  
> +static enum ssl_conn_status ustream_ssl_do_connect(struct ustream_ssl *us)
> +{
> +	enum ssl_conn_status status;
> +
> +	us->connecting = true;
> +	status = __ustream_ssl_connect(us);
> +	us->connecting = false;
> +	return status;
> +}
> +
I think this can be fixed in a much simpler way. Simply prevent
re-entrant calls to __ustream_ssl_connect through a static variable. The
other checks for us->connecting should be unnecessary, I think
!us->connected is enough.

- Felix
Yousong Zhou Dec. 12, 2014, 4:16 a.m. UTC | #2
On 12 December 2014 at 00:42, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2014-11-11 11:34, Yousong Zhou wrote:
>> ustream_ssl_check_conn() may be called by .notify_write while a previous
>> SSL_connect() is still in process.  This can happen because the
>> .notify_write callback will may be triggered by writes in the BIO
>> methods.
>>
>> Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
>> ---
>>  ustream-ssl.c |   19 +++++++++++++++----
>>  ustream-ssl.h |    1 +
>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/ustream-ssl.c b/ustream-ssl.c
>> index dd0faf9..84104b0 100644
>> --- a/ustream-ssl.c
>> +++ b/ustream-ssl.c
>> @@ -34,12 +34,22 @@ static void ustream_ssl_error_cb(struct uloop_timeout *t)
>>               us->notify_error(us, error, __ustream_ssl_strerror(us->error, buffer, sizeof(buffer)));
>>  }
>>
>> +static enum ssl_conn_status ustream_ssl_do_connect(struct ustream_ssl *us)
>> +{
>> +     enum ssl_conn_status status;
>> +
>> +     us->connecting = true;
>> +     status = __ustream_ssl_connect(us);
>> +     us->connecting = false;
>> +     return status;
>> +}
>> +
> I think this can be fixed in a much simpler way. Simply prevent
> re-entrant calls to __ustream_ssl_connect through a static variable.

Guarding it with a single static variable do not work well with
multiple instances of ustream_ssl.

> The
> other checks for us->connecting should be unnecessary, I think
> !us->connected is enough.

Yes.

                yousong

>
> - Felix
Felix Fietkau Dec. 12, 2014, 11:36 a.m. UTC | #3
On 2014-12-12 05:16, Yousong Zhou wrote:
> On 12 December 2014 at 00:42, Felix Fietkau <nbd@openwrt.org> wrote:
>> On 2014-11-11 11:34, Yousong Zhou wrote:
>>> ustream_ssl_check_conn() may be called by .notify_write while a previous
>>> SSL_connect() is still in process.  This can happen because the
>>> .notify_write callback will may be triggered by writes in the BIO
>>> methods.
>>>
>>> Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
>>> ---
>>>  ustream-ssl.c |   19 +++++++++++++++----
>>>  ustream-ssl.h |    1 +
>>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ustream-ssl.c b/ustream-ssl.c
>>> index dd0faf9..84104b0 100644
>>> --- a/ustream-ssl.c
>>> +++ b/ustream-ssl.c
>>> @@ -34,12 +34,22 @@ static void ustream_ssl_error_cb(struct uloop_timeout *t)
>>>               us->notify_error(us, error, __ustream_ssl_strerror(us->error, buffer, sizeof(buffer)));
>>>  }
>>>
>>> +static enum ssl_conn_status ustream_ssl_do_connect(struct ustream_ssl *us)
>>> +{
>>> +     enum ssl_conn_status status;
>>> +
>>> +     us->connecting = true;
>>> +     status = __ustream_ssl_connect(us);
>>> +     us->connecting = false;
>>> +     return status;
>>> +}
>>> +
>> I think this can be fixed in a much simpler way. Simply prevent
>> re-entrant calls to __ustream_ssl_connect through a static variable.
> 
> Guarding it with a single static variable do not work well with
> multiple instances of ustream_ssl.
How so? Even with multiple instances a call stack from one instance that
calls do_connect should not end up with re-entrant calls for a different
instance.

- Felix
Yousong Zhou Dec. 12, 2014, 2 p.m. UTC | #4
On 12 December 2014 at 19:36, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2014-12-12 05:16, Yousong Zhou wrote:
>> On 12 December 2014 at 00:42, Felix Fietkau <nbd@openwrt.org> wrote:
>>> On 2014-11-11 11:34, Yousong Zhou wrote:
>>>> ustream_ssl_check_conn() may be called by .notify_write while a previous
>>>> SSL_connect() is still in process.  This can happen because the
>>>> .notify_write callback will may be triggered by writes in the BIO
>>>> methods.
>>>>
>>>> Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
>>>> ---
>>>>  ustream-ssl.c |   19 +++++++++++++++----
>>>>  ustream-ssl.h |    1 +
>>>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/ustream-ssl.c b/ustream-ssl.c
>>>> index dd0faf9..84104b0 100644
>>>> --- a/ustream-ssl.c
>>>> +++ b/ustream-ssl.c
>>>> @@ -34,12 +34,22 @@ static void ustream_ssl_error_cb(struct uloop_timeout *t)
>>>>               us->notify_error(us, error, __ustream_ssl_strerror(us->error, buffer, sizeof(buffer)));
>>>>  }
>>>>
>>>> +static enum ssl_conn_status ustream_ssl_do_connect(struct ustream_ssl *us)
>>>> +{
>>>> +     enum ssl_conn_status status;
>>>> +
>>>> +     us->connecting = true;
>>>> +     status = __ustream_ssl_connect(us);
>>>> +     us->connecting = false;
>>>> +     return status;
>>>> +}
>>>> +
>>> I think this can be fixed in a much simpler way. Simply prevent
>>> re-entrant calls to __ustream_ssl_connect through a static variable.
>>
>> Guarding it with a single static variable do not work well with
>> multiple instances of ustream_ssl.
> How so? Even with multiple instances a call stack from one instance that
> calls do_connect should not end up with re-entrant calls for a different
> instance.

But we do not want do_connect() of different instances intervening
with each other, do we?


                yousong

Patch
diff mbox

diff --git a/ustream-ssl.c b/ustream-ssl.c
index dd0faf9..84104b0 100644
--- a/ustream-ssl.c
+++ b/ustream-ssl.c
@@ -34,12 +34,22 @@  static void ustream_ssl_error_cb(struct uloop_timeout *t)
 		us->notify_error(us, error, __ustream_ssl_strerror(us->error, buffer, sizeof(buffer)));
 }
 
+static enum ssl_conn_status ustream_ssl_do_connect(struct ustream_ssl *us)
+{
+	enum ssl_conn_status status;
+
+	us->connecting = true;
+	status = __ustream_ssl_connect(us);
+	us->connecting = false;
+	return status;
+}
+
 static void ustream_ssl_check_conn(struct ustream_ssl *us)
 {
-	if (us->connected || us->error)
+	if (us->connected || us->error || us->connecting)
 		return;
 
-	if (__ustream_ssl_connect(us) == U_SSL_OK) {
+	if (ustream_ssl_do_connect(us) == U_SSL_OK) {
 		us->connected = true;
 		if (us->notify_connected)
 			us->notify_connected(us);
@@ -55,7 +65,7 @@  static bool __ustream_ssl_poll(struct ustream *s)
 	bool more = false;
 
 	ustream_ssl_check_conn(us);
-	if (!us->connected || us->error)
+	if (!us->connected || us->error || us->connecting)
 		return false;
 
 	do {
@@ -106,7 +116,7 @@  static int ustream_ssl_write(struct ustream *s, const char *buf, int len, bool m
 {
 	struct ustream_ssl *us = container_of(s, struct ustream_ssl, stream);
 
-	if (!us->connected || us->error)
+	if (!us->connected || us->error || us->connecting)
 		return 0;
 
 	if (us->conn->w.data_bytes)
@@ -141,6 +151,7 @@  static void ustream_ssl_free(struct ustream *s)
 	us->ssl = NULL;
 	us->conn = NULL;
 	us->peer_cn = NULL;
+	us->connecting = false;
 	us->connected = false;
 	us->error = false;
 	us->valid_cert = false;
diff --git a/ustream-ssl.h b/ustream-ssl.h
index 0c55344..1d2a8f9 100644
--- a/ustream-ssl.h
+++ b/ustream-ssl.h
@@ -37,6 +37,7 @@  struct ustream_ssl {
 	char *server_name;
 
 	int error;
+	bool connecting;
 	bool connected;
 	bool server;