httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: svn commit: r599385 - in /httpd/httpd/trunk: ./ modules/ssl/
Date Thu, 29 Nov 2007 21:03:29 GMT
On Thu, Nov 29, 2007 at 09:02:43PM +0100, Ruediger Pluem wrote:
> > ==============================================================================
> > --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> > +++ httpd/httpd/trunk/CHANGES [utf-8] Thu Nov 29 03:18:40 2007
> > @@ -2,6 +2,9 @@
> >  Changes with Apache 2.3.0
> >  [ When backported to 2.2.x, remove entry from this file ]
> >  
> > +  *) mod_ssl: Add support for OCSP validation of client certificates.
> > +     PR 41123.  [Marc Stern <marc.stern approach.be>, Joe Orton]
> > +
> 
> Shouldn't we add Steve to this? As far as I followed the discussion in 
> Bugzilla he contributed very valuable points and we have credited 
> people for less in the past :-).

Discussing this stuff makes me slightly uncomfortable, but I try to 
follow a strict rule: credit in CHANGES exactly the set of people who 
contributed actual lines of code.  Steve was credited in the commit 
messages as a reviewer.  Such review is invaluable, no argument at all 
:)

...
> > +/* Send the OCSP request serialized into BIO 'request' to the
> > + * responder at given server given by URI.  Returns socket object or
> > + * NULL on error. */
> > +static apr_socket_t *send_request(BIO *request, const apr_uri_t *uri, 
> > +                                  conn_rec *c, apr_pool_t *p)
> > +{
> > +    apr_status_t rv;
> > +    apr_sockaddr_t *sa;
> > +    apr_socket_t *sd;
> > +    char buf[HUGE_STRING_LEN];
> 
> Is it really a good idea to store this on the stack? Shouldn't we allocate
> this from the pool?

Hmmm, "shrug" - stack is cheaper than heap.

...
> > +    while ((len = BIO_read(request, buf, sizeof buf)) > 0) {
> > +        char *wbuf = buf;
> > +        apr_size_t remain = len;
> > +        
> > +        do {
> > +            apr_size_t wlen = remain;
> > +
> > +            rv = apr_socket_send(sd, wbuf, &wlen);
> > +            wbuf += remain;
> > +            remain -= wlen;
> > +        } while (rv == APR_SUCCESS && remain > 0);
> 
> Why do we need remain here and do not use len directly?

Really only to make the types match; len is an int but wlen must be an 
apr_size_t.

...
> > +    count = 0;
> > +    while ((line = get_line(tmpbb, bb, c, p)) != NULL && line[0]
> > +           && ++count < MAX_HEADERS) {
> > +        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> > +                      "OCSP response header: %s", line);
> > +    }
> 
> Why don't we exit with an error message if count > MAX_HEADERS?

Good catch, will fix.

...
> > +    apr_brigade_destroy(bb);
> > +    apr_brigade_destroy(tmpbb);
> 
> We miss to destroy the brigades in the cases above where we return NULL.

They are allocated out of the pool, so it Shouldn't Matter (TM).

Thanks for the review!

joe

Mime
View raw message