| Message ID | 1415702041-44573-2-git-send-email-yszhou4tech@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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;
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(-)