Last Comment Bug 723453 - Heap overrun (read + write) in nsBMPEncoder::ConvertHostARGBRow
: Heap overrun (read + write) in nsBMPEncoder::ConvertHostARGBRow
Status: RESOLVED FIXED
[sg:vector-critical (gcc)][qa-]
: valgrind
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla13
Assigned To: Jeff Gilbert [:jgilbert]
:
Mentors:
Depends on: 650720
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 03:28 PST by Julian Seward [:jseward]
Modified: 2012-06-21 17:06 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
12+
fixed


Attachments
Split out the 32-bit case from ConvertHostARGB (1.88 KB, patch)
2012-02-07 14:13 PST, Jeff Gilbert [:jgilbert]
joe: review+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description Julian Seward [:jseward] 2012-02-02 03:28:37 PST
x86_64-linux, running on a 16 bit deep vnc server.
TEST_PATH=content/canvas/test/test_toDataURL_alpha.html

(DISPLAY=:1 TEST_PATH=content/canvas/test/test_toDataURL_alpha.html make -C ff-opt mochitest-plain EXTRA_TEST_ARGS='--close-when-done --debugger=vTRUNK --debugger-args="--tool=memcheck --suppressions=/home/sewardj/MOZ/fglrx-supp.supp --suppressions=/home/sewardj/MOZ/moz-supp.supp --error-limit=no --stats=yes --smc-check=all-non-file --trace-children=yes --child-silent-after-fork=yes '--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel' --track-origins=no"') 2>&1 | tee spew2-memcheck-1

produces the errors below -- looks like a read-modify-write of an invalid address

Invalid read of size 1
   at 0x808209D: nsBMPEncoder::ConvertHostARGBRow(unsigned char const*, unsigned char*, unsigned int) (nsBMPEncoder.cpp:450)
   by 0x80828CD: nsBMPEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:228)
   by 0x8081EE9: nsBMPEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:107)
   by 0x835111B: nsCanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (nsCanvasRenderingContext2D.cpp:1345)
   by 0x83E9C9D: nsHTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (nsHTMLCanvasElement.cpp:247)
   by 0x83EA1B2: nsHTMLCanvasElement::ToDataURLImpl(nsAString_internal const&, nsIVariant*, nsAString_internal&) (nsHTMLCanvasElement.cpp:340)
   by 0x88607FA: nsIDOMHTMLCanvasElement_ToDataURL(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:17910)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x908762D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2801)
   by 0x908DACC: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:537)
   by 0x90554C2: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:157)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
 Address 0x201e63ac is 0 bytes after a block of size 300 alloc'd
   at 0x4C28223: malloc (vg_replace_malloc.c:263)
   by 0x8082787: nsBMPEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (mozalloc.h:309)
   by 0x8081EE9: nsBMPEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:107)
   by 0x835111B: nsCanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (nsCanvasRenderingContext2D.cpp:1345)
   by 0x83E9C9D: nsHTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (nsHTMLCanvasElement.cpp:247)
   by 0x83EA1B2: nsHTMLCanvasElement::ToDataURLImpl(nsAString_internal const&, nsIVariant*, nsAString_internal&) (nsHTMLCanvasElement.cpp:340)
   by 0x88607FA: nsIDOMHTMLCanvasElement_ToDataURL(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:17910)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x908762D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2801)
   by 0x908DACC: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:537)
   by 0x90554C2: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:157)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)

Invalid write of size 1
   at 0x80820A6: nsBMPEncoder::ConvertHostARGBRow(unsigned char const*, unsigned char*, unsigned int) (nsBMPEncoder.cpp:450)
   by 0x80828CD: nsBMPEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:228)
   by 0x8081EE9: nsBMPEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:107)
   by 0x835111B: nsCanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (nsCanvasRenderingContext2D.cpp:1345)
   by 0x83E9C9D: nsHTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (nsHTMLCanvasElement.cpp:247)
   by 0x83EA1B2: nsHTMLCanvasElement::ToDataURLImpl(nsAString_internal const&, nsIVariant*, nsAString_internal&) (nsHTMLCanvasElement.cpp:340)
   by 0x88607FA: nsIDOMHTMLCanvasElement_ToDataURL(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:17910)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x908762D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2801)
   by 0x908DACC: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:537)
   by 0x90554C2: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:157)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
 Address 0x201e63ac is 0 bytes after a block of size 300 alloc'd
   at 0x4C28223: malloc (vg_replace_malloc.c:263)
   by 0x8082787: nsBMPEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (mozalloc.h:309)
   by 0x8081EE9: nsBMPEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:107)
   by 0x835111B: nsCanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (nsCanvasRenderingContext2D.cpp:1345)
   by 0x83E9C9D: nsHTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (nsHTMLCanvasElement.cpp:247)
   by 0x83EA1B2: nsHTMLCanvasElement::ToDataURLImpl(nsAString_internal const&, nsIVariant*, nsAString_internal&) (nsHTMLCanvasElement.cpp:340)
   by 0x88607FA: nsIDOMHTMLCanvasElement_ToDataURL(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:17910)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x908762D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2801)
   by 0x908DACC: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:537)
   by 0x90554C2: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:157)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
Comment 1 Julian Seward [:jseward] 2012-02-02 08:56:18 PST
Given that the offending line, allegedly nsBMPEncoder.cpp:450 is

    if (mBMPInfoHeader.bpp == 32) {
      pixelOut[3] = alpha;   // line 450
    }

it seems odd that there's invalid read as well as an invalid write.
It looks as if gcc has compiled it as if it were

   pixelOut[3] =  mBMPInfoHeader.bpp == 32  ?  alpha  :  pixelOut[3];

which is bizarre, and (I thought) disallowed by the C++11 memory model.

  862096:       66 83 7f 2a 20          cmpw   $0x20,0x2a(%rdi)
  86209b:       74 05                   je     8620a2
                                               <_ZN12nsBMPEncoder18ConvertHostARGBRowEPKhPhj+0x52>
  86209d:       44 0f b6 50 03          movzbl 0x3(%rax),%r10d
  8620a2:       49 83 c0 01             add    $0x1,%r8
  8620a6:       44 88 50 03             mov    %r10b,0x3(%rax)
Comment 2 Joe Drew (not getting mail) 2012-02-07 11:54:00 PST
Jeff is working on changing a lot of the ARGB conversion code, so he should be aware of this.

Brian did a significant amount of work on the BMP encoder; perhaps he has some ideas?
Comment 3 Bas Schouten (:bas.schouten) 2012-02-07 13:31:56 PST
I have to agree with Julian on this one. This seems to be gcc compiling to illegal code. I can see why this would be a useful optimization if pixelOut[3] were a legal address. But since it is perfectly fine in this case for that to only be a valid address when mBMPInfoHeader.bpp == 32 it cannot make that assumption.
Comment 4 Julian Seward [:jseward] 2012-02-07 14:07:03 PST
Ah; slight misunderstanding.  I was merely trying to figure out why
pixelOut[3] = alpha; lead to a read as well as a write.  But now Bas
points it out, perhaps we have no error at all, and both the invalid
read and write are caused gcc's code generation strategy.

In fact, if our code was really broken, we'd only see an invalid
write.  The invalid (read,write) pair implies gcc is broken.  I wonder
if there's a gcc bug open on this.  This was with vanilla 4.6.2, built
from source, building Fx at -O2 (IIRC).
Comment 5 Jeff Gilbert [:jgilbert] 2012-02-07 14:11:29 PST
This is annoying, but at least the workaround should be easy.

The worrisome part is that this meme is not uncommon. Generally, you don't want to put a branch in the loop, but when we choose to, we shouldn't need to worry about an invalid access popping out of a branch.

Can we reduce the testcase and file a GCC bug?
Comment 6 Jeff Gilbert [:jgilbert] 2012-02-07 14:13:09 PST
Created attachment 595172 [details] [diff] [review]
Split out the 32-bit case from ConvertHostARGB

This should do it.
Comment 8 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-13 16:51:53 PST
> Can we reduce the testcase and file a GCC bug?

I started a build of firefox to try to reproduce this, but if someone has a .ii file  and the necessary command line, that would save some time.
Comment 9 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-13 17:25:32 PST
This reproduces with just

void
foo(char* aDest, int y)
{
  aDest[0] = 41;
  if (y == 32) {
    aDest[1] = 42;
  }
}

and
 /usr/libexec/gcc/x86_64-redhat-linux/4.6.2/cc1plus test.ii -quiet -Os -o nsBMPEncoder.s

This doesn't happen with our gcc, it produces:

_Z3fooPci:
.LFB0:
	cmpl	$32, %esi
	movb	$41, (%rdi)
	jne	.L1
	movb	$42, 1(%rdi)
.L1:
	ret

Should we make this bug block one about upgrading gcc?
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-13 17:40:41 PST
This still reproduces with gcc trunk r184188.
Comment 11 Daniel Holbert [:dholbert] (largely AFK until June 28) 2012-03-12 16:48:31 PDT
Looks like this made it to m-c a month ago (shortly after the inbound landing in comment 7):
 https://hg.mozilla.org/mozilla-central/rev/b84e21604bc4

(I'll leave it to someone else to determine whether this should be RESOLVED|FIXED, given comment 10)
Comment 12 Daniel Veditz [:dveditz] 2012-03-14 16:31:20 PDT
I'm calling this fixed based on the workaround checkin, although it's troubling we probably have more places like this. Although we do run Valgrind a fair bit and don't seem to hit this that I've seen.

Do we need a separate bug to track the gcc issue or did we file a bug in their tracker? If we did please add it to the See Also field
Comment 13 Alex Keybl [:akeybl] 2012-03-19 16:04:33 PDT
Tracking for ESR 13+, since this is first fixed on 13. Is there something we can uplift to resolve this security bug on Beta 12?
Comment 14 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-20 08:00:20 PDT
> Do we need a separate bug to track the gcc issue or did we file a bug in
> their tracker? If we did please add it to the See Also field

We should probably have a bug in our bugzilla so that we can block upgrading gcc on it.
Comment 15 Jeff Gilbert [:jgilbert] 2012-04-10 15:22:18 PDT
Comment on attachment 595172 [details] [diff] [review]
Split out the 32-bit case from ConvertHostARGB

[Approval Request Comment]
Regression caused by (bug #): None.
User impact if declined: Potential security issue with BMPs.
Testing completed (on m-c, etc.): m-c, currently on aurora.
Risk to taking this patch (and alternatives if risky): Very low.
String changes made by this patch: None.
Comment 16 Alex Keybl [:akeybl] 2012-04-11 16:37:50 PDT
Comment on attachment 595172 [details] [diff] [review]
Split out the 32-bit case from ConvertHostARGB

[Triage Comment]
Approving for Beta 12 and the ESR branch given the fact that this is a critical security vulnerability and has been deemed low risk.
Comment 17 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-13 12:06:22 PDT
Going to try to autoland this - might not get comments back here from automation because this is a security bug and our release@ account doesn't have sec-group permissions, but I will monitor this push and report back if successful.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-13 14:12:48 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #17)
> Going to try to autoland this - might not get comments back here from
> automation because this is a security bug and our release@ account doesn't
> have sec-group permissions, but I will monitor this push and report back if
> successful.

Autoland for security bugs doesn't currently work, so nothing has landed here - please go ahead with a normal landing.
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2012-04-16 10:51:36 PDT
Sorry for the mis-push on esr10. Re-landed:
https://hg.mozilla.org/releases/mozilla-esr10/rev/9fe4480ebd01

What happened is the patch failed to apply and I didn't pay attention. The reject was ugly, so I copied and pasted the body of ConvertHostARGBRow from the mozilla-beta patch. Seems entirely safe...
Comment 21 Jeff Gilbert [:jgilbert] 2012-04-16 11:45:56 PDT
Note that this changes the functionality slightly, in that we no longer zero the pixel if alpha is zero. This should have no significant effect, though.

Functionality change is from bug 650720:
http://hg.mozilla.org/mozilla-central/diff/8abaa54efb2a/image/encoders/bmp/nsBMPEncoder.cpp
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-29 11:26:29 PDT
Julian, are you able to reproduce this bug anymore in Firefox 13.0 Beta 6?

Note You need to log in before you can comment on or make changes to this bug.