Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

Commit

Permalink
Fix crasher on 404 .pagespeed. resources w/a custom location
Browse files Browse the repository at this point in the history
- Fix crasher on .pagespeed. resources that return 404
- Additionally, check for a wiped request context and make sure
  we do not dereference a null pointer, which is what hurt in
  the flow we hit above.
- Also, do not check fail when we would receive a stale event
  originating from a NgxBaseFetch that is no longer associated
  with our request context.

This is not the final change yet, as I want to back this with
tests where possible, and log warning when appropriate.

For #1081
  • Loading branch information
oschaaf committed Dec 24, 2015
1 parent 56a5d41 commit 8bde044
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 2 deletions.
25 changes: 24 additions & 1 deletion src/ngx_base_fetch.cc
Expand Up @@ -140,7 +140,30 @@ void NgxBaseFetch::ReadCallback(const ps_event_data& data) {
}
ps_request_ctx_t* ctx = ps_get_request_context(r);

CHECK(data.sender == ctx->base_fetch);
// If someone else wiped our request context, skip this event
// https://github.com/pagespeed/ngx_pagespeed/issues/1081
if (ctx == NULL) {
// TODO(oschaaf): log warning
return;
}

DCHECK(data.sender == ctx->base_fetch);
if (data.sender != ctx->base_fetch) {
// If someone else changed our request context, skip this event.
// Changing of request contexts may occur when nginx performs internal
// redirects, at which point we stil may have in-flight events.
// TODO(oschaaf): maintain a single request context associated to the
// request. Doing so also allows us to avoid doing the IPRO lookup
// multiple times, and makes things a fraction less complex.
// When we fix that, we can turn the DCHECK above into a full CHECK
// again.
// NOTE: Currently I have not been able to reproduce this actually
// causing trouble, but there may be corner cases I missed.

// TODO(oschaaf): log warning(?)
return;
}

CHECK(r->count > 0) << "r->count: " << r->count;

int rc;
Expand Down
3 changes: 2 additions & 1 deletion src/ngx_pagespeed.cc
Expand Up @@ -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) {
Expand Down

0 comments on commit 8bde044

Please sign in to comment.