New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jeffkaufman opened this Issue May 13, 2015 · 10 comments

Comments

Projects
None yet
4 participants
@jeffkaufman
Copy link
Contributor

jeffkaufman commented May 13, 2015

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.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 13, 2015

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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 13, 2015

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

This comment has been minimized.

Copy link
Contributor

morlovich commented May 13, 2015

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

This comment has been minimized.

Copy link
Contributor

jmaessen commented May 13, 2015

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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 13, 2015

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

@jmaessen

This comment has been minimized.

Copy link
Contributor

jmaessen commented May 13, 2015

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

This comment has been minimized.

Copy link
Contributor

huibaolin commented May 13, 2015

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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented May 13, 2015

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

This comment has been minimized.

Copy link
Contributor

huibaolin commented May 13, 2015

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

This comment has been minimized.

Copy link
Contributor

jmaessen commented May 13, 2015

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 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment