Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c

2014-11-14 Thread Jeff Trawick 
On Fri, Nov 14, 2014 at 1:18 PM, yla...@apache.org wrote:

 Author: ylavic
 Date: Fri Nov 14 18:18:15 2014
 New Revision: 1639717

 URL: http://svn.apache.org/r1639717
 Log:
 mod_authnz_fcgi: Fix a potential crash with response headers' size above
 8K.
 (similar to r1638818 for mod_proxy_fcgi).

 Modified:
 httpd/httpd/trunk/CHANGES
 httpd/httpd/trunk/docs/log-message-tags/next-number
 httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c

 Modified: httpd/httpd/trunk/CHANGES
 URL:
 http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1639717r1=1639716r2=1639717view=diff

 ==
 --- httpd/httpd/trunk/CHANGES [utf-8] (original)
 +++ httpd/httpd/trunk/CHANGES [utf-8] Fri Nov 14 18:18:15 2014
 @@ -5,6 +5,9 @@ Changes with Apache 2.5.0
   mod_proxy_fcgi: Fix a potential crash with response headers' size
 above 8K.
   [Teguh chain rop.io, Yann Ylavic]

 +  *) mod_authnz_fcgi: Fix a potential crash with response headers' size
 above 8K.
 + [Yann Ylavic]
 +
*) mod_authnz_ldap: Resolve crashes with LDAP authz and non-LDAP authn
 since
   r1608202. [Eric Covener]


 Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
 URL:
 http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1639717r1=1639716r2=1639717view=diff

 ==
 --- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
 +++ httpd/httpd/trunk/docs/log-message-tags/next-number Fri Nov 14
 18:18:15 2014
 @@ -1 +1 @@
 -2821
 +2822

 Modified: httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
 URL:
 http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c?rev=1639717r1=1639716r2=1639717view=diff

 ==
 --- httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c (original)
 +++ httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c Fri Nov 14 18:18:15
 2014
 @@ -406,13 +406,12 @@ enum {
   *
   * Returns 0 if it can't find the end of the headers, and 1 if it found
 the
   * end of the headers. */
 -static int handle_headers(request_rec *r,
 -  int *state,
 -  char *readbuf)
 +static int handle_headers(request_rec *r, int *state,
 +  char *readbuf, apr_size_t readlen)
  {
  const char *itr = readbuf;

 -while (*itr) {
 +while (readlen) {
  if (*itr == '\r') {
  switch (*state) {
  case HDR_STATE_GOT_CRLF:
 @@ -443,13 +442,17 @@ static int handle_headers(request_rec *r
   break;
  }
  }
 -else {
 +else if (*itr == '\t' || !apr_iscntrl(*itr)) {
  *state = HDR_STATE_READING_HEADERS;
  }
 +else {
 +return -1;
 +}


First, thanks for taking care of this issue.

I guess I don't understand the importance of this code which returns an
error on non-tab control characters.  Why isn't the character checking the
same as in ap_scan_script_header_err_brigade_ex() since that is what will
finally parse the buffer?

Thanks!


  if (*state == HDR_STATE_DONE_WITH_HEADERS)
  break;

 +--readlen;
  ++itr;
  }

 @@ -555,7 +558,17 @@ static apr_status_t handle_response(cons
  APR_BRIGADE_INSERT_TAIL(ob, b);

  if (!seen_end_of_headers) {
 -int st = handle_headers(r, header_state, readbuf);
 +int st = handle_headers(r, header_state, readbuf,
 +readbuflen);
 +
 +if (st == -1) {
 +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
 +  APLOGNO(02821) %s: error reading 
 +  headers from %s,
 +  fn, conf-backend);
 +rv = APR_EINVAL;
 +break;
 +}

  if (st == 1) {
  int status;
 @@ -646,7 +659,7 @@ static apr_status_t handle_response(cons
  /*
   * Read/discard any trailing padding.
   */
 -if (plen) {
 +if (rv == APR_SUCCESS  plen) {
  rv = recv_data_full(conf, r, s, readbuf, plen);
  if (rv != APR_SUCCESS) {
  ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,





-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c

2014-11-14 Thread Yann Ylavic 
On Fri, Nov 14, 2014 at 11:25 PM, Jeff Trawick traw...@gmail.com wrote:
 On Fri, Nov 14, 2014 at 1:18 PM, yla...@apache.org wrote:
[]
 -static int handle_headers(request_rec *r,
 -  int *state,
 -  char *readbuf)
 +static int handle_headers(request_rec *r, int *state,
 +  char *readbuf, apr_size_t readlen)
  {
  const char *itr = readbuf;

 -while (*itr) {
 +while (readlen) {
  if (*itr == '\r') {
  switch (*state) {
  case HDR_STATE_GOT_CRLF:
 @@ -443,13 +442,17 @@ static int handle_headers(request_rec *r
   break;
  }
  }
 -else {
 +else if (*itr == '\t' || !apr_iscntrl(*itr)) {
  *state = HDR_STATE_READING_HEADERS;
  }
 +else {
 +return -1;
 +}

[]
 I guess I don't understand the importance of this code which returns an
 error on non-tab control characters.  Why isn't the character checking the
 same as in ap_scan_script_header_err_brigade_ex() since that is what will
 finally parse the buffer?

Well, the previous code was stopping on '\0' (hopefully a '\n' was
reached before), and I wanted to preserve this behaviour.
While at it, I chose to also stop on any control char since they are
not valid in HTTP headers (but '\t').
getsfunc_BRIGADE() does not seem to care about those, and is not
supposed to return anything but -1 (timeout) as error...

Isn't handle_headers() handling HTTP headers?
If not I'm afraid I overdid that fix.

Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c

2014-11-14 Thread Jeff Trawick 
On Fri, Nov 14, 2014 at 6:00 PM, Yann Ylavic ylavic@gmail.com wrote:

 On Fri, Nov 14, 2014 at 11:25 PM, Jeff Trawick traw...@gmail.com wrote:
  On Fri, Nov 14, 2014 at 1:18 PM, yla...@apache.org wrote:
 []
  -static int handle_headers(request_rec *r,
  -  int *state,
  -  char *readbuf)
  +static int handle_headers(request_rec *r, int *state,
  +  char *readbuf, apr_size_t readlen)
   {
   const char *itr = readbuf;
 
  -while (*itr) {
  +while (readlen) {
   if (*itr == '\r') {
   switch (*state) {
   case HDR_STATE_GOT_CRLF:
  @@ -443,13 +442,17 @@ static int handle_headers(request_rec *r
break;
   }
   }
  -else {
  +else if (*itr == '\t' || !apr_iscntrl(*itr)) {
   *state = HDR_STATE_READING_HEADERS;
   }
  +else {
  +return -1;
  +}
 
 []
  I guess I don't understand the importance of this code which returns an
  error on non-tab control characters.  Why isn't the character checking
 the
  same as in ap_scan_script_header_err_brigade_ex() since that is what will
  finally parse the buffer?

 Well, the previous code was stopping on '\0' (hopefully a '\n' was
 reached before), and I wanted to preserve this behaviour.
 While at it, I chose to also stop on any control char since they are
 not valid in HTTP headers (but '\t').
 getsfunc_BRIGADE() does not seem to care about those, and is not
 supposed to return anything but -1 (timeout) as error...

 Isn't handle_headers() handling HTTP headers?
 If not I'm afraid I overdid that fix.


handle_headers() is processing these headers just enough to tell when the
complete set of headers is in the brigade, at which point it will be
processed by ap_scan_script_header_err_brigade_ex().  I don't think
catching errors that ap_scan_* don't catch is a serious problem, but it
might cause confusion in some extremely odd cases such as programmers
comparing the code :) or having a bad script work with a module that only
uses ap_scan_* but fail with one of these).

Is the header syntax actually supported by ap_scan_script_header* HTTP or
HTTP-like?  Here's an easy way out of that:  It doesn't support folded
lines AFAICT, so it is HTTP-like ;)  But seriously, it seems very plausible
that the CGI spec is intended to describe script headers as following the
HTTP header syntax.  (It doesn't bother describing LWS.)

Summary: I don't think it is a big deal in practice but I think it can be
confusing that the code that just needs to determine if the end has been
found can discover errors that otherwise would be unnoticed.

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: svn commit: r1639717 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_fcgi.c

2014-11-14 Thread Yann Ylavic 
On Sat, Nov 15, 2014 at 12:40 AM, Jeff Trawick traw...@gmail.com wrote:
 Summary: I don't think it is a big deal in practice but I think it can be
 confusing that the code that just needs to determine if the end has been
 found can discover errors that otherwise would be unnoticed.

I agree, the checks belong to ap_scan_script_header*().
Maybe we need (yet) another ap_scan_script_header_parse(), reentrant
(optionaly taking care of folding), and returning a relevant value for
the caller to know.
All cgi handlers (by the existing functions) would use it.

I will undo these change for now and let all the data reach
ap_scan_script_header_err_brigade_ex(), that's indeed better.
Likewise wrt r1638818.

Thanks for the review,
Yann.

Re: svn commit: r1638879 - /httpd/httpd/trunk/server/mpm/event/event.c

2014-11-14 Thread Marion Christophe JAILLET 

Hi,

the same pattern exists in eventopt.

CJ


Le 12/11/2014 18:32, cove...@apache.org a écrit :

Author: covener
Date: Wed Nov 12 17:32:24 2014
New Revision: 1638879

URL: http://svn.apache.org/r1638879
Log:
avoid dereferencing a recently apr_pool_clear()'ed event_conn_state_t *cs
in several paths where ptrans is being recycled at the end of a request.


Modified:
 httpd/httpd/trunk/server/mpm/event/event.c

Modified: httpd/httpd/trunk/server/mpm/event/event.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1638879r1=1638878r2=1638879view=diff
==
--- httpd/httpd/trunk/server/mpm/event/event.c (original)
+++ httpd/httpd/trunk/server/mpm/event/event.c Wed Nov 12 17:32:24 2014
@@ -852,6 +852,7 @@ static int start_lingering_close_common(
  rv = apr_pollset_add(event_pollset, cs-pfd);
  apr_thread_mutex_unlock(timeout_mutex);
  if (rv != APR_SUCCESS  !APR_STATUS_IS_EEXIST(rv)) {
+apr_pool_t *p = cs-p;
  ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf,
   start_lingering_close: apr_pollset_add failure);
  apr_thread_mutex_lock(timeout_mutex);
@@ -859,7 +860,7 @@ static int start_lingering_close_common(
  apr_thread_mutex_unlock(timeout_mutex);
  apr_socket_close(cs-pfd.desc.s);
  apr_pool_clear(cs-p);
-ap_push_pool(worker_queue_info, cs-p);
+ap_push_pool(worker_queue_info, p);
  return 0;
  }
  return 1;

[...]