1.10.33.6 -> 1.10.33.7

Fixes https://github.com/pagespeed/ngx_pagespeed/issues/1138

config

@@ -29,8 +29,8 @@ if [ "$mod_pagespeed_dir" = "unset" ] ; then
     echo "   You need to separately download the pagespeed library:"
     echo ""
     echo "     $ cd /path/to/ngx_pagespeed"
-    echo "     $ wget https://dl.google.com/dl/page-speed/psol/1.10.33.5.tar.gz"
-    echo "     $ tar -xzvf 1.10.33.5.tar.gz # expands to psol/"
+    echo "     $ wget https://dl.google.com/dl/page-speed/psol/1.10.33.7.tar.gz"
+    echo "     $ tar -xzvf 1.10.33.7.tar.gz # expands to psol/"
     echo ""
     echo "   Or see the installation instructions:"
     echo "     https://github.com/pagespeed/ngx_pagespeed#how-to-build"

src/ngx_base_fetch.cc

@@ -118,6 +118,8 @@ const char* BaseFetchTypeToCStr(NgxBaseFetchType type) {
   return "can't get here";
 }
 
+// TODO(oschaaf): replace the ngx_log_error with VLOGS or pass in a
+// MessageHandler and use that.
 void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
   NgxBaseFetch* base_fetch = reinterpret_cast<NgxBaseFetch*>(data.sender);
   ngx_http_request_t* r = base_fetch->request();
@@ -138,19 +140,40 @@ void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
   if (refcount == 0 || detached) {
     return;
   }
+
   ps_request_ctx_t* ctx = ps_get_request_context(r);
 
-  CHECK(data.sender == ctx->base_fetch);
-  CHECK(r->count > 0) << "r->count: " << r->count;
+  // If our request context was zeroed, skip this event.
+  // See https://github.com/pagespeed/ngx_pagespeed/issues/1081
+  if (ctx == NULL) {
+    // Should not happen normally, when it does this message will cause our
+    // system tests to fail.
+    ngx_log_error(NGX_LOG_WARN, ngx_cycle->log, 0,
+        "pagespeed [%p] skipping event: request context gone", r);
+    return;
+  }
+
+  // Normally we expect the sender to equal the active NgxBaseFetch instance.
+  DCHECK(data.sender == ctx->base_fetch);
+
+  // If someone changed our request context or NgxBaseFetch, skip processing.
+  if (data.sender != ctx->base_fetch) {
+      ngx_log_error(NGX_LOG_WARN, ngx_cycle->log, 0,
+          "pagespeed [%p] skipping event: event originating from disassociated"
+          " NgxBaseFetch instance.", r);
+      return;
+  }
 
   int rc;
+  bool run_posted = true;
   // If we are unlucky enough to have our connection finalized mid-ipro-lookup,
   // we must enter a different flow. Also see ps_in_place_check_header_filter().
   if ((ctx->base_fetch->base_fetch_type_ != kIproLookup)
       && r->connection->error) {
     ngx_log_error(NGX_LOG_DEBUG, ngx_cycle->log, 0,
-      "pagespeed [%p] request already finalized", r);
+      "pagespeed [%p] request already finalized %d", r, r->count);
     rc = NGX_ERROR;
+    run_posted = false;
   } else {
     rc = ps_base_fetch::ps_base_fetch_handler(r);
   }
@@ -163,8 +186,11 @@ void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
 
   ngx_connection_t* c = r->connection;
   ngx_http_finalize_request(r, rc);
-  // See http://forum.nginx.org/read.php?2,253006,253061
-  ngx_http_run_posted_requests(c);
+
+  if (run_posted) {
+    // See http://forum.nginx.org/read.php?2,253006,253061
+    ngx_http_run_posted_requests(c);
+  }
 }
 
 void NgxBaseFetch::Lock() {

src/ngx_pagespeed.cc

@@ -284,7 +284,8 @@ ngx_int_t ps_base_fetch_handler(ngx_http_request_t* r) {
     // modules running after us to manipulate those responses.
     if (!status_ok && (ctx->base_fetch->base_fetch_type() != kHtmlTransform
                        && ctx->base_fetch->base_fetch_type() != kIproLookup)) {
-      return status_code;
+      ps_release_base_fetch(ctx);
+      return ngx_http_filter_finalize_request(r, NULL, status_code);
     }
 
     if (ctx->preserve_caching_headers != kDontPreserveHeaders) {
@@ -491,6 +492,10 @@ ngx_int_t copy_response_headers_to_ngx(
 
   ngx_int_t i;
   for (i = 0 ; i < pagespeed_headers.NumAttributes() ; i++) {
+    // For IPRO cache misses, these gs_ variables may point to freed memory
+    // when nginx writes the headers to the output as the NgxBaseFetch instance
+    // that owns this memory gets released during request processing. So we
+    // copy these strings later on.
     const GoogleString& name_gs = pagespeed_headers.Name(i);
     const GoogleString& value_gs = pagespeed_headers.Value(i);
 
@@ -511,6 +516,9 @@ ngx_int_t copy_response_headers_to_ngx(
     }  // else we don't preserve any headers.
 
     ngx_str_t name, value;
+    value.len = value_gs.size();
+    value.data = reinterpret_cast<u_char*>(
+        string_piece_to_pool_string(r->pool, value_gs.c_str()));
 
     // To prevent the gzip module from clearing weak etags, we output them
     // using a different name here. The etag header filter module runs behind
@@ -521,11 +529,15 @@ ngx_int_t copy_response_headers_to_ngx(
       name.data = reinterpret_cast<u_char*>(
           const_cast<char*>(kInternalEtagName));
     } else {
-      name.len = name_gs.length();
-      name.data = reinterpret_cast<u_char*>(const_cast<char*>(name_gs.data()));
+      name.len = name_gs.size();
+      name.data = reinterpret_cast<u_char*>(
+          string_piece_to_pool_string(r->pool, name_gs.c_str()));
+    }
+
+    // In case string_piece_to_pool_string failed:
+    if (name.data == NULL || value.data == NULL) {
+        return NGX_ERROR;
     }
-    value.len = value_gs.length();
-    value.data = reinterpret_cast<u_char*>(const_cast<char*>(value_gs.data()));
 
     // TODO(jefftk): If we're setting a cache control header we'd like to
     // prevent any downstream code from changing it.  Specifically, if we're
@@ -536,24 +548,17 @@ ngx_int_t copy_response_headers_to_ngx(
     // net/instaweb/apache/header_util:AddResponseHeadersToRequest
 
     // Make copies of name and value to put into headers_out.
-
-    u_char* value_s = ngx_pstrdup(r->pool, &value);
-    if (value_s == NULL) {
-      return NGX_ERROR;
-    }
-
     if (STR_EQ_LITERAL(name, "Cache-Control")) {
-      ps_set_cache_control(r, const_cast<char*>(value_gs.c_str()));
+      ps_set_cache_control(r, reinterpret_cast<char*>(value.data));
       continue;
     } else if (STR_EQ_LITERAL(name, "Content-Type")) {
       // Unlike all the other headers, content_type is just a string.
-      headers_out->content_type.data = value_s;
-      headers_out->content_type.len = value.len;
+      headers_out->content_type = value;
 
       // We should not include the charset when determining content_type_len, so
       // scan for the ';' that marks the start of the charset part.
       for (ngx_uint_t i = 0; i < value.len; i++) {
-        if (value_s[i] == ';') {
+        if (value.data[i] == ';') {
           break;
         }
         headers_out->content_type_len = i + 1;
@@ -573,11 +578,6 @@ ngx_int_t copy_response_headers_to_ngx(
       continue;
     }
 
-    u_char* name_s = ngx_pstrdup(r->pool, &name);
-    if (name_s == NULL) {
-      return NGX_ERROR;
-    }
-
     ngx_table_elt_t* header = static_cast<ngx_table_elt_t*>(
         ngx_list_push(&headers_out->headers));
     if (header == NULL) {
@@ -585,10 +585,10 @@ ngx_int_t copy_response_headers_to_ngx(
     }
 
     header->hash = 1;  // Include this header in the output.
+    header->key.data = name.data;
     header->key.len = name.len;
-    header->key.data = name_s;
+    header->value.data = value.data;
     header->value.len = value.len;
-    header->value.data = value_s;
 
     // Populate the shortcuts to commonly used headers.
     if (STR_EQ_LITERAL(name, "Date")) {

test/nginx_system_test.sh

@@ -552,7 +552,7 @@ fetch_until "$URL" "fgrep -c .pagespeed." 1 --header=Host:www.google.com
 
 # If this accepts the Host header and fetches from google.com it will fail with
 # a 404.  Instead it should use a loopback fetch and succeed.
-URL="$HOSTNAME/mod_pagespeed_example/.pagespeed.ce.8CfGBvwDhH.css"
+URL="$HOSTNAME/mod_pagespeed_example/styles/big.css.pagespeed.ce.8CfGBvwDhH.css"
 check wget -O /dev/null --header=Host:www.google.com "$URL"
 
 start_test statistics load
@@ -1241,6 +1241,14 @@ check_from "$OUT" fgrep -qi '404'
 MATCHES=$(echo "$OUT" | grep -c "Cache-Control: override") || true
 check [ $MATCHES -eq 1 ]
 
+start_test Custom 404 does not crash.
+URL=http://custom404.example.com/mod_pagespeed_test/
+URL+=A.doesnotexist.css.pagespeed.cf.0.css
+# The 404 response makes wget exit with an error code, which we ignore.
+OUT=$(http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP -O /dev/null -S $URL 2>&1) || true
+# We ignored the exit code, check if we got a 404 response.
+check_from "$OUT" fgrep -qi '404'
+
 start_test Shutting down.
 
 # Fire up some heavy load if ab is available to test a stressed shutdown
@@ -1316,6 +1324,8 @@ OUT=$(cat "$ERROR_LOG" \
     | grep -v "\\[warn\\].*remote\.cfg.*" \
     | grep -v "\\[warn\\].*end token not received.*" \
     | grep -v "\\[warn\\].*failed to hook next event.*" \
+    | grep -v "\\[warn\\].*Rewrite.*failed.*.pagespeed....0.foo.*" \
+    | grep -v "\\[warn\\].*A.blue.css.*but cannot access the original.*" \
     || true)
 
 check [ -z "$OUT" ]

test/pagespeed_test.conf.template

@@ -967,6 +967,14 @@ http {
     }
   }
 
+  server {
+    listen @@SECONDARY_PORT@@;
+    listen [::]:@@SECONDARY_PORT@@;
+    server_name custom404.example.com;
+    pagespeed FileCachePath "@@SECONDARY_CACHE@@";
+    error_page 404 /404.html;
+  }
+
   server {
     listen @@SECONDARY_PORT@@;
     listen [::]:@@SECONDARY_PORT@@;