Logging $ssl_session_id can crash Nginx 1.5.9 worker

[Ruslan Ermilov]

On Thu, Jan 23, 2014 at 01:48:58AM +0400, Maxim Dounin wrote:
> On Thu, Jan 23, 2014 at 12:06:55AM +0400, Ruslan Ermilov wrote:
> 
> > On Wed, Jan 22, 2014 at 02:02:51PM -0500, mnordhoff wrote:
> > > I run the nginx.org mainline packages on Ubuntu 12.04, 32- and 64-bit. I use
> > > a wacky custom log format, and after 1.5.9 was released today, I enabled
> > > logging the $ssl_session_id variable. I later ran an SSL Labs SSL Server
> > > Test, [0] which makes numerous HTTPS requests of various sorts, and lo and
> > > behold, one of my worker processes core dumped. I fooled around with my
> > > configuration and determined that the problem was logging $ssl_session_id.
> > > If I don't log it, it's fine. If I enable a log that contains
> > > $ssl_session_id -- even only $ssl_session_id -- it crashes.
> > 
> > The following patch fixes this:
> > 
> > diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
> > --- a/src/event/ngx_event_openssl.c
> > +++ b/src/event/ngx_event_openssl.c
> > @@ -2509,6 +2509,10 @@ ngx_ssl_get_session_id(ngx_connection_t 
> >  
> >      sess = SSL_get0_session(c->ssl->connection);
> >  
> > +    if (sess == NULL) {
> > +        return NGX_ERROR;
> > +    }
> > +
> >      buf = sess->session_id;
> >      len = sess->session_id_length;
> 
> You were faster. :)
> I think that len = 0 + NGX_OK is better than NGX_ERROR here 
> though, and also in line with other similar functions like 
> ngx_ssl_get_[raw_]certificate().

It's also consistent with the previous behavior, so I agree.