Image rewriting check-fails on images with invalid dimensions #1078

jeffkaufman · 2015-05-13T17:41:10Z

When image_rewrite_filter optimizes a truncated image it can CHECK-fail:

[Wed May 13 11:51:21 2015] [alert] [mod_pagespeed 1.10.0.0-7210 @23900] [0513/115120:FATAL:image_rewrite_filter.cc(807)]
Check failed: image_dim.width() > 0 && image_dim.height() > 0. 
third_party/chromium/src/base/debug/stack_trace_posix.cc:475: base::debug::StackTrace::StackTrace [0x2b835547e00c]
third_party/chromium/src/base/logging.cc:562: logging::LogMessage::~LogMessage [0x2b835547f40d]
net/instaweb/rewriter/image_rewrite_filter.cc:807: net_instaweb::ImageRewriteFilter::ResizeImageIfNecessary [0x2b835556859e]
net/instaweb/rewriter/image_rewrite_filter.cc:1005 (discriminator 2): net_instaweb::ImageRewriteFilter::RewriteLoadedResourceImpl [0x2b8355569381]
net/instaweb/rewriter/image_rewrite_filter.cc:439 (discriminator 3): net_instaweb::ImageRewriteFilter::Context::RewriteSingle [0x2b83555660c9]
net/instaweb/rewriter/single_rewrite_context.cc:80 (discriminator 1): net_instaweb::SingleRewriteContext::Rewrite [0x2b83555bc8cc]
net/instaweb/rewriter/rewrite_context.cc:1204: net_instaweb::RewriteContext::InvokeRewriteFunction::Run [0x2b83555b44fe]
pagespeed/kernel/base/function.cc:52: net_instaweb::Function::CallRun [0x2b8355760ea6]
pagespeed/kernel/thread/queued_worker_pool.cc:164: net_instaweb::QueuedWorkerPool::Run [0x2b83557a89a9]
./third_party/pagespeed/kernel/base/function.h:202 (discriminator 3): net_instaweb::MemberFunction2<net_instaweb::QueuedWorkerPool, net_instaweb::QueuedWorkerPool::Sequence*, net_instaweb::QueuedWorker*>::Run [0x2b83557af9ea]
pagespeed/kernel/base/function.cc:52: net_instaweb::Function::CallRun [0x2b8355760ea6]
pagespeed/kernel/thread/worker.cc:81 (discriminator 1): net_instaweb::Worker::WorkThread::Run [0x2b83557b52e4]
pagespeed/kernel/thread/pthread_thread_system.cc:123: net_instaweb::PthreadThreadImpl::InvokeRun [0x2b83559c1d9b]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x8182) [0x2b8354be4182]
/lib/x86_64-linux-gnu/libc.so.6(clone+0x6d) [0x2b83550f847d]

This shouldn't happen. Invalid images should only generate kInfo messages and +debug comments.

The text was updated successfully, but these errors were encountered:

jeffkaufman · 2015-05-13T17:42:48Z

The code:

  ImageDim image_dim;
  image->Dimensions(&image_dim);

  DCHECK(image_dim.width() > 0 && image_dim.height() > 0);
  if (image_dim.width() == 0 || image_dim.height() == 0) {
    cached->add_debug_message(
        "Cannot resize: Image must have nonzero dimensions");
    return false;
  }

We should probably just remove this DCHECK. I'll make a CL for that.

jeffkaufman · 2015-05-13T17:44:25Z

The comment on Dimensions is:

  // This
  // method can fail (ImageUrlEncoder::HasValidDimensions(natural_dim)
  // == false) for various reasons: we don't understand the image
  // format, we can't find the headers, the library doesn't support a
  // particular encoding, etc.  In that case the other fields are left
  // alone.

So in addition to removing the DCHECK we should update the debug message.

morlovich · 2015-05-13T17:45:46Z

Probably better change the if to be <= 0, too?

On Wed, May 13, 2015 at 1:44 PM, Jeff Kaufman notifications@github.com
wrote:

The comment on Dimensions is:

This
// method can fail (ImageUrlEncoder::HasValidDimensions(natural_dim)
// == false) for various reasons: we don't understand the image
// format, we can't find the headers, the library doesn't support a
// particular encoding, etc. In that case the other fields are left
// alone.

So in addition to removing the DCHECK we should update the debug message.

—
Reply to this email directly or view it on GitHub
#1078 (comment)
.

jmaessen · 2015-05-13T17:46:06Z

Hmm, this was a DCHECK because we should be checking this at the caller.
It'd be good to add a test case if we're not doing that.

On Wed, May 13, 2015 at 1:41 PM, Jeff Kaufman notifications@github.com
wrote:

When image_rewrite_filter optimizes a truncated image it can CHECK-fail:

[Wed May 13 11:51:21 2015] [alert] [mod_pagespeed 1.10.0.0-7210 @23900] [0513/115120:FATAL:image_rewrite_filter.cc(807)] Check failed: image_dim.width() > 0 && image_dim.height() > 0.
third_party/chromium/src/base/debug/stack_trace_posix.cc:475: base::debug::StackTrace::StackTrace [0x2b835547e00c]
third_party/chromium/src/base/logging.cc:562: logging::LogMessage::~LogMessage [0x2b835547f40d]
net/instaweb/rewriter/image_rewrite_filter.cc:807: net_instaweb::ImageRewriteFilter::ResizeImageIfNecessary [0x2b835556859e]
net/instaweb/rewriter/image_rewrite_filter.cc:1005 (discriminator 2): net_instaweb::ImageRewriteFilter::RewriteLoadedResourceImpl [0x2b8355569381]
net/instaweb/rewriter/image_rewrite_filter.cc:439 (discriminator 3): net_instaweb::ImageRewriteFilter::Context::RewriteSingle [0x2b83555660c9]
net/instaweb/rewriter/single_rewrite_context.cc:80 (discriminator 1): net_instaweb::SingleRewriteContext::Rewrite [0x2b83555bc8cc]
net/instaweb/rewriter/rewrite_context.cc:1204: net_instaweb::RewriteContext::InvokeRewriteFunction::Run [0x2b83555b44fe]
pagespeed/kernel/base/function.cc:52: net_instaweb::Function::CallRun [0x2b8355760ea6]
pagespeed/kernel/thread/queued_worker_pool.cc:164: net_instaweb::QueuedWorkerPool::Run [0x2b83557a89a9]
./third_party/pagespeed/kernel/base/function.h:202 (discriminator 3): net_instaweb::MemberFunction2<net_instaweb::QueuedWorkerPool, net_instaweb::QueuedWorkerPool::Sequence*, net_instaweb::QueuedWorker*>::Run [0x2b83557af9ea]
pagespeed/kernel/base/function.cc:52: net_instaweb::Function::CallRun [0x2b8355760ea6]
pagespeed/kernel/thread/worker.cc:81 (discriminator 1): net_instaweb::Worker::WorkThread::Run [0x2b83557b52e4]
pagespeed/kernel/thread/pthread_thread_system.cc:123: net_instaweb::PthreadThreadImpl::InvokeRun [0x2b83559c1d9b]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x8182) [0x2b8354be4182]
/lib/x86_64-linux-gnu/libc.so.6(clone+0x6d) [0x2b83550f847d]

This shouldn't happen. Invalid images should only generate kInfo messages
and +debug comments.

—
Reply to this email directly or view it on GitHub
#1078.

jeffkaufman · 2015-05-13T17:48:03Z

It's a little weird that there's a DCHECK and also a debug message.

jmaessen · 2015-05-13T17:52:16Z

On Wed, May 13, 2015 at 1:48 PM, Jeff Kaufman notifications@github.com
wrote:

It's a little weird that there's a DCHECK and also a debug message.

The intent was to catch bugs in our dev flow, but fail gracefully on real
systems.

It looks like this comment talks about what broke things:
https://cs.corp.google.com/#piper///depot/google3/net/instaweb/rewriter/image_rewrite_filter.cc&l=985

—
Reply to this email directly or view it on GitHub
#1078 (comment)
.

huibaolin · 2015-05-13T17:53:47Z

DCHECK and debug message work for different purpose.

DCHECK warns us, only in debug mode, that something bad happened, e.g.,
the image dimensions were not correctly computed.

Debug message tells us that we can't resize the image at this moment. The
image can be valid, e.g., GIF image might have 0-by-0 dimension.

I'm not sure if we should remove DCHECK.

Huibao

On Wed, May 13, 2015 at 1:48 PM, Jeff Kaufman notifications@github.com
wrote:

It's a little weird that there's a DCHECK and also a debug message.

—
Reply to this email directly or view it on GitHub
#1078 (comment)
.

jeffkaufman · 2015-05-13T17:55:01Z

We need to remove the DCHECK because PageSpeed shouldn't crash on invalid resources, even in debug mode. DCHECKs should only be for programming errors.

huibaolin · 2015-05-13T18:00:16Z

Ah, then blending DCHECK into the if-statement below, as Maks suggested,
sounds good.

On Wed, May 13, 2015 at 1:55 PM, Jeff Kaufman notifications@github.com
wrote:

We need to remove the DCHECK because PageSpeed shouldn't crash on invalid
resources, even in debug mode. DCHECKs should only be for programming
errors.

—
Reply to this email directly or view it on GitHub
#1078 (comment)
.

jmaessen · 2015-05-13T20:00:16Z

On Wed, May 13, 2015 at 1:52 PM, Jan-Willem Maessen jmaessen@google.com
wrote:

On Wed, May 13, 2015 at 1:48 PM, Jeff Kaufman notifications@github.com
wrote:

It's a little weird that there's a DCHECK and also a debug message.

The intent was to catch bugs in our dev flow, but fail gracefully on real
systems.

It looks like this comment talks about what broke things:

https://cs.corp.google.com/#piper///depot/google3/net/instaweb/rewriter/image_rewrite_filter.cc&l=985

Right, looking at this some more I think just axing the DCHECK is the right
answer.

-Jan

—
Reply to this email directly or view it on GitHub
#1078 (comment)
.

jeffkaufman mentioned this issue May 13, 2015

Apache stuck indefinitely waiting for PSOL #1048

Closed

jeffkaufman closed this as completed in 0b399cc May 14, 2015

jeffkaufman added a commit that referenced this issue May 15, 2015

Fix #1078 by removing a DCHECK for valid image dimensions.

9842e18

jeffkaufman added a commit that referenced this issue May 27, 2015

Fix #1078 by removing a DCHECK for valid image dimensions.

166151f

jeffkaufman changed the title ~~check failure in image_rewrite_filter on truncated image~~ Image rewriting check-fails on images with invalid dimensions Jul 27, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image rewriting check-fails on images with invalid dimensions #1078

Image rewriting check-fails on images with invalid dimensions #1078

jeffkaufman commented May 13, 2015

jeffkaufman commented May 13, 2015

jeffkaufman commented May 13, 2015

morlovich commented May 13, 2015

jmaessen commented May 13, 2015

jeffkaufman commented May 13, 2015

jmaessen commented May 13, 2015

huibaolin commented May 13, 2015

jeffkaufman commented May 13, 2015

huibaolin commented May 13, 2015

jmaessen commented May 13, 2015

Image rewriting check-fails on images with invalid dimensions #1078

Image rewriting check-fails on images with invalid dimensions #1078

Comments

jeffkaufman commented May 13, 2015

jeffkaufman commented May 13, 2015

jeffkaufman commented May 13, 2015

morlovich commented May 13, 2015

jmaessen commented May 13, 2015

jeffkaufman commented May 13, 2015

jmaessen commented May 13, 2015

huibaolin commented May 13, 2015

jeffkaufman commented May 13, 2015

huibaolin commented May 13, 2015

jmaessen commented May 13, 2015