Closed Bug 1205900 (CVE-2015-7189) Opened 9 years ago Closed 9 years ago

Heap Buffer Overflow in nsJPEGEncoder::ConvertHostARGBRow()

Categories

(Core :: Graphics: Canvas2D, defect)

43 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla44
Tracking Status
firefox41 --- wontfix
firefox42 + verified
firefox43 + verified
firefox44 + verified
firefox-esr38 42+ verified

People

(Reporter: loobenyang, Assigned: milan)

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [adv-main42+][adv-esr38.4+])

Attachments

(3 files, 3 obsolete files)

Convert the canvas to blob in DOM attribute modified event can cause memory corruption.

Reproduce test case:

<html><body><canvas id="canvas0"></canvas></body><script>
var canvas0=document.getElementById("canvas0");
var ctx=canvas0.getContext("2d");
canvas0.addEventListener("DOMAttrModified", function(event) {canvas0.toBlob(function(){},"image/jpeg", 1);}, true); 
canvas0.setAttribute("height",470)
setTimeout(function(){location.reload()},300);
</script></html>

Steps to reproduce:
1. Open the attached test case (Uaf_ConvertHostARGBRow_Repro.html) in Firefox browser.
2. Firefox crashes at random address in nsJPEGEncoder::ConvertHostARGBRow()


First-chance exception at 0x104D34C8 (xul.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x1A690002.
Unhandled exception at 0x104D34C8 (xul.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x1A690002.


Firefox version: 43.0a1 (2015-09-09)

Call stack:

 	xul.dll!nsJPEGEncoder::ConvertHostARGBRow(const unsigned char * aSrc, unsigned char * aDest, unsigned int aPixelWidth) Line 364	C++
 	xul.dll!nsJPEGEncoder::InitFromData(const unsigned char * aData, unsigned int aLength, unsigned int aWidth, unsigned int aHeight, unsigned int aStride, unsigned int aInputFormat, const nsAString_internal & aOutputOptions) Line 175	C++
 	xul.dll!mozilla::dom::ImageEncoder::GetInputStream(int aWidth, int aHeight, unsigned char * aImageBuffer, int aFormat, imgIEncoder * aEncoder, const wchar_t * aEncoderOptions, nsIInputStream * * aStream) Line 329	C++
 	xul.dll!mozilla::dom::ImageEncoder::ExtractDataInternal(const nsAString_internal & aType, const nsAString_internal & aOptions, unsigned char * aImageBuffer, int aFormat, const mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> aSize, mozilla::layers::Image * aImage, nsICanvasRenderingContextInternal * aContext, nsIInputStream * * aStream, imgIEncoder * aEncoder) Line 363	C++
>	xul.dll!mozilla::dom::EncodingRunnable::ProcessImageData(unsigned __int64 * aImgSize, void * * aImgData) Line 169	C++
 	xul.dll!mozilla::dom::EncodingRunnable::Run() Line 202	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 874	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 277	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 355	C++
 	xul.dll!MessageLoop::RunHandler() Line 228	C++
 	xul.dll!MessageLoop::Run() Line 202	C++
 	xul.dll!nsThread::ThreadFunc(void * aArg) Line 365	C++
 	nss3.dll!_PR_NativeRunThread(void * arg) Line 419	C
 	nss3.dll!pr_root(void * arg) Line 90	C
 	[External Code]	
 	[Frames below may be incorrect and/or missing, no symbols loaded for msvcr120.dll]	


Checking the variables of EncodingRunnable in debugger, the 0x5a5a5a5a pattern indicates an Use After Free:

-		mEncoder	{...}	nsCOMPtr<imgIEncoder>
+		nsCOMPtr_base	{mRawPtr=0x5a5a5a5a {...} }	nsCOMPtr_base
-		mImage	{mRawPtr=0x5a5a5a5a {mRefCnt={mValue={...} } mBackendData={mArray={mArr=0x5a5a5a62 {...} } } mImplData=...} }	nsRefPtr<mozilla::layers::Image>
+		mRawPtr	0x5a5a5a5a {mRefCnt={mValue={...} } mBackendData={mArray={mArr=0x5a5a5a62 {{...}, {...}, {...}, {...}, ...} } } ...}	mozilla::layers::Image *
-		stream	{...}	nsCOMPtr<nsIInputStream>
+		nsCOMPtr_base	{mRawPtr=0x00000000 <NULL> }	nsCOMPtr_base
-		this	0x194b4af6 {mType={mStorage=0x194b4b16 L"" } mOptions={mStorage=0x194b4baa L"婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚... } ...}	mozilla::dom::EncodingRunnable *
+		nsRunnable	{mRefCnt={mValue={...} } _mOwningThread={mThread=0x00000000 } }	nsRunnable
+		mType	{mStorage=0x194b4b16 L"" }	nsAutoString
+		mOptions	{mStorage=0x194b4baa L"婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚婚... }	nsAutoString
+		mImageBuffer	{mRawPtr=0x5a5a5a5a <Error reading characters of string.> }	nsAutoArrayPtr<unsigned char>
+		mImage	{mRawPtr=0x5a5a5a5a {mRefCnt={mValue={...} } mBackendData={mArray={mArr=0x5a5a5a62 {...} } } mImplData=...} }	nsRefPtr<mozilla::layers::Image>
+		mEncoder	{...}	nsCOMPtr<imgIEncoder>
+		mEncodingCompleteEvent	{mRawPtr=0x5a5a5a5a {mImgSize=??? mType={mStorage=0x5a5a5a86 <Error reading characters of string.> } ...} }	nsRefPtr<mozilla::dom::EncodingCompleteEvent>
		mFormat	1515870810	int
+		mSize	{...}	const mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>
		mUsingCustomOptions	true (90)	bool
This bug also affects stable release (40.0.3):


(2738.3158): Access violation - code c0000005 (!!! second chance !!!)
eax=1748cf00 ebx=00000001 ecx=12e7b0aa edx=1748d001 esi=000000f4 edi=0000012c
eip=02f64769 esp=1edcf48c ebp=1edcf754 iopl=0         nv up ei pl nz na po nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00210202
xul!nsJPEGEncoder::ConvertHostARGBRow+0x15:
02f64769 8a4201          mov     al,byte ptr [edx+1]        ds:002b:1748d002=??
It turns out a Buffer Overflow. I ran the test case in official Linux asan build, it reported a heap-buffer-overflow in ConvertHostARGBRow():

Firefox version: 43.0a1 (2015-09-08)

=================================================================
==2681==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f1d4b36c720 at pc 0x7f1d6c8f06f4 bp 0x7f1d49fea7f0 sp 0x7f1d49fea7e8
READ of size 4 at 0x7f1d4b36c720 thread T23
    #0 0x7f1d6c8f06f3 in ConvertHostARGBRow /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/encoders/jpeg/nsJPEGEncoder.cpp:364
    #1 0x7f1d6c8f06f3 in nsJPEGEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/encoders/jpeg/nsJPEGEncoder.cpp:174
    #2 0x7f1d6cb5ab08 in GetInputStream /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/ImageEncoder.cpp:326
    #3 0x7f1d6cb5ab08 in mozilla::dom::ImageEncoder::ExtractDataInternal(nsAString_internal const&, nsAString_internal const&, unsigned char*, int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::layers::Image*, nsICanvasRenderingContextInternal*, nsIInputStream**, imgIEncoder*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/ImageEncoder.cpp:356
    #4 0x7f1d6cb7bb84 in mozilla::dom::EncodingRunnable::ProcessImageData(unsigned long*, void**) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/ImageEncoder.cpp:161
    #5 0x7f1d6cb78d29 in mozilla::dom::EncodingRunnable::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/ImageEncoder.cpp:201
    #6 0x7f1d6a8c42c3 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:874
    #7 0x7f1d6a93b49a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:277
    #8 0x7f1d6b1bd7ef in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:326
    #9 0x7f1d6b1490ac in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #10 0x7f1d6b1490ac in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #11 0x7f1d6b1490ac in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #12 0x7f1d6a8c06e5 in nsThread::ThreadFunc(void*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:363
    #13 0x7f1d77b704b5 in _pt_root /builds/slave/m-cen-l64-asan-000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
    #14 0x7f1d781b8181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)
    #15 0x7f1d6838b30c (/lib/x86_64-linux-gnu/libc.so.6+0xfb30c)

0x7f1d4b36c720 is located 0 bytes to the right of 180000-byte region [0x7f1d4b340800,0x7f1d4b36c720)
allocated by thread T0 (Web Content) here:
    #0 0x474fe1 in __interceptor_malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7f1d6c25ea04 in operator new[] /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/gfx/2d/../../dist/include/mozilla/mozalloc.h:204
    #2 0x7f1d6c25ea04 in mozilla::gfx::SurfaceToPackedBGRA(mozilla::gfx::DataSourceSurface*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/2d/DataSurfaceHelpers.cpp:123
    #3 0x7f1d6e8ffed4 in mozilla::dom::CanvasRenderingContext2D::GetImageBuffer(unsigned char**, int*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/canvas/CanvasRenderingContext2D.cpp:1646
    #4 0x7f1d6ed554b9 in mozilla::dom::HTMLCanvasElement::ToBlob(JSContext*, mozilla::dom::FileCallback&, nsAString_internal const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/html/HTMLCanvasElement.cpp:587
    #5 0x7f1d6e4dd92b in mozilla::dom::HTMLCanvasElementBinding::toBlob(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLCanvasElement*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/./HTMLCanvasElementBinding.cpp:345
    #6 0x7f1d6e856097 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/bindings/BindingUtils.cpp:2602
    #7 0x7f1d7370d85b in CallJSNative /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jscntxtinlines.h:235
    #8 0x7f1d7370d85b in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:751
    #9 0x7f1d7374dcf4 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:3067
    #10 0x7f1d7372d317 in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:704
    #11 0x7f1d7370dfbf in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:781
    #12 0x7f1d736b648d in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:818
    #13 0x7f1d742965d5 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jsapi.cpp:4610
    #14 0x7f1d6e3d36ff in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/./EventListenerBinding.cpp:47
    #15 0x7f1d6ebfda29 in HandleEvent<mozilla::dom::EventTarget *> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/events/../../dist/include/mozilla/dom/EventListenerBinding.h:54
    #16 0x7f1d6ebfda29 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventListenerManager.cpp:1005
    #17 0x7f1d6ebff237 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventListenerManager.cpp:1136
    #18 0x7f1d6ebee9d1 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:314
    #19 0x7f1d6ebf2eaa in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:651
    #20 0x7f1d6ebcbb95 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:715
    #21 0x7f1d6cde44f4 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsINode.cpp:1295
    #22 0x7f1d6ebafe37 in mozilla::AsyncEventDispatcher::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/AsyncEventDispatcher.cpp:52
    #23 0x7f1d6c93a3b4 in nsContentUtils::RemoveScriptBlocker() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsContentUtils.cpp:5202
    #24 0x7f1d6cb0eefa in ~nsAutoScriptBlocker /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsContentUtils.h:2661
    #25 0x7f1d6cb0eefa in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/Element.cpp:2269
    #26 0x7f1d6ef1f051 in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/html/nsGenericHTMLElement.cpp:913
    #27 0x7f1d6ed516da in mozilla::dom::HTMLCanvasElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/html/HTMLCanvasElement.cpp:174
    #28 0x7f1d6cb06ede in SetAttr /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/base/../../dist/include/mozilla/dom/Element.h:447
    #29 0x7f1d6cb06ede in mozilla::dom::Element::SetAttribute(nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/Element.cpp:1186
    #30 0x7f1d6e45e757 in mozilla::dom::ElementBinding::setAttribute(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/./ElementBinding.cpp:653
    #31 0x7f1d6e856097 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/bindings/BindingUtils.cpp:2602
    #32 0x7f1d7370d85b in CallJSNative /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jscntxtinlines.h:235
    #33 0x7f1d7370d85b in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:751
    #34 0x7f1d7374dcf4 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:3067
    #35 0x7f1d7372d317 in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:704

Thread T23 created by T0 (Web Content) here:
    #0 0x461855 in pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:175
    #1 0x7f1d77b6ce3d in _PR_CreateThread /builds/slave/m-cen-l64-asan-000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:453
    #2 0x7f1d77b6c9ba in PR_CreateThread /builds/slave/m-cen-l64-asan-000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:544
    #3 0x7f1d6a8c1cbd in nsThread::Init() /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:474
    #4 0x7f1d6a8c7e5e in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThreadManager.cpp:249
    #5 0x7f1d6a93a718 in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:68
    #6 0x7f1d6cb5c8ad in mozilla::dom::ImageEncoder::ExtractDataAsync(nsAString_internal&, nsAString_internal const&, bool, unsigned char*, int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::dom::EncodeCompleteCallback*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/ImageEncoder.cpp:298
    #7 0x7f1d6ed55759 in mozilla::dom::HTMLCanvasElement::ToBlob(JSContext*, mozilla::dom::FileCallback&, nsAString_internal const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/html/HTMLCanvasElement.cpp:631
    #8 0x7f1d6e4dd92b in mozilla::dom::HTMLCanvasElementBinding::toBlob(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLCanvasElement*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/./HTMLCanvasElementBinding.cpp:345
    #9 0x7f1d6e856097 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/bindings/BindingUtils.cpp:2602
    #10 0x7f1d7370d85b in CallJSNative /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jscntxtinlines.h:235
    #11 0x7f1d7370d85b in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:751
    #12 0x7f1d7374dcf4 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:3067
    #13 0x7f1d7372d317 in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:704
    #14 0x7f1d7370dfbf in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:781
    #15 0x7f1d736b648d in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:818
    #16 0x7f1d742965d5 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jsapi.cpp:4610
    #17 0x7f1d6e3d36ff in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/./EventListenerBinding.cpp:47
    #18 0x7f1d6ebfda29 in HandleEvent<mozilla::dom::EventTarget *> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/events/../../dist/include/mozilla/dom/EventListenerBinding.h:54
    #19 0x7f1d6ebfda29 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventListenerManager.cpp:1005
    #20 0x7f1d6ebff237 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventListenerManager.cpp:1136
    #21 0x7f1d6ebee9d1 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:314
    #22 0x7f1d6ebf2eaa in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:651
    #23 0x7f1d6ebcbb95 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:715
    #24 0x7f1d6cde44f4 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsINode.cpp:1295
    #25 0x7f1d6ebafe37 in mozilla::AsyncEventDispatcher::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/AsyncEventDispatcher.cpp:52
    #26 0x7f1d6c93a3b4 in nsContentUtils::RemoveScriptBlocker() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsContentUtils.cpp:5202
    #27 0x7f1d6cb0eefa in ~nsAutoScriptBlocker /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsContentUtils.h:2661
    #28 0x7f1d6cb0eefa in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/Element.cpp:2269
    #29 0x7f1d6ef1f051 in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/html/nsGenericHTMLElement.cpp:913
    #30 0x7f1d6ed516da in mozilla::dom::HTMLCanvasElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/html/HTMLCanvasElement.cpp:174
    #31 0x7f1d6cb06ede in SetAttr /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/base/../../dist/include/mozilla/dom/Element.h:447
    #32 0x7f1d6cb06ede in mozilla::dom::Element::SetAttribute(nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/Element.cpp:1186
    #33 0x7f1d6e45e757 in mozilla::dom::ElementBinding::setAttribute(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/./ElementBinding.cpp:653
    #34 0x7f1d6e856097 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/bindings/BindingUtils.cpp:2602
    #35 0x7f1d7370d85b in CallJSNative /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jscntxtinlines.h:235
    #36 0x7f1d7370d85b in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:751
    #37 0x7f1d7374dcf4 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:3067
    #38 0x7f1d7372d317 in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:704
    #39 0x7f1d73760648 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:978
    #40 0x7f1d73760ca8 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:1011
    #41 0x7f1d74294dcd in Evaluate(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::ScopeObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jsapi.cpp:4446
    #42 0x7f1d7429554b in Evaluate(JSContext*, JS::AutoVectorRooter<JSObject*>&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jsapi.cpp:4473
    #43 0x7f1d6ce13304 in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsJSUtils.cpp:224
    #44 0x7f1d6ce13f61 in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, void**) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsJSUtils.cpp:286
    #45 0x7f1d6ce99603 in nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, JS::SourceBufferHolder&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsScriptLoader.cpp:1197
    #46 0x7f1d6ce96c1a in nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsScriptLoader.cpp:1016
    #47 0x7f1d6ce90087 in nsScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsScriptLoader.cpp:799
    #48 0x7f1d6ce8bade in nsScriptElement::MaybeProcessScript() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsScriptElement.cpp:142
    #49 0x7f1d6c2300c4 in operator-> /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsIScriptElement.h:221
    #50 0x7f1d6c2300c4 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:662
    #51 0x7f1d6c22e6ff in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/slave/m-cen-l64-asan-000000000000000/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:487
    #52 0x7f1d6c23484b in nsHtml5ExecutorFlusher::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/parser/html/nsHtml5StreamParser.cpp:127
    #53 0x7f1d6a8c42c3 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:874
    #54 0x7f1d6a93b49a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:277
    #55 0x7f1d6b1bc879 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:95
    #56 0x7f1d6b1490ac in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #57 0x7f1d6b1490ac in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #58 0x7f1d6b1490ac in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #59 0x7f1d7010d1b7 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/widget/nsBaseAppShell.cpp:156
    #60 0x7f1d71f61122 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:785
    #61 0x7f1d6b1490ac in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #62 0x7f1d6b1490ac in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #63 0x7f1d6b1490ac in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #64 0x7f1d71f60819 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:621
    #65 0x48d670 in content_process_main(int, char**) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:237
    #66 0x7f1d682b1ec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/encoders/jpeg/nsJPEGEncoder.cpp:364 ConvertHostARGBRow
Shadow bytes around the buggy address:
  0x0fe429665890: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe4296658a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe4296658b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe4296658c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe4296658d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fe4296658e0: 00 00 00 00[fa]fa fa fa fa fa fa fa fa fa fa fa
  0x0fe4296658f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe429665900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe429665910: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe429665920: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe429665930: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack l==2681==ABORTING
Summary: Use After Free in nsJPEGEncoder::ConvertHostARGBRow() → Heap Buffer Overflow in nsJPEGEncoder::ConvertHostARGBRow()
Flags: sec-bounty?
Group: core-security → gfx-core-security
Assignee: nobody → milan
We were asserting in the debug; since this can happen when we call ToBlob in the update callback, we can't just assert and have to handle the two values not matching.
Attachment #8666028 - Flags: review?(gwright)
Comment on attachment 8666028 [details] [diff] [review]
Canvas and context should match sizes. r=gwright

Hold off a bit.
Attachment #8666028 - Flags: review?(gwright)
Attachment #8666028 - Attachment is obsolete: true
ToBlob gets called before we had a chance to react to the change in dimensions.
The base class updates the attribute(s) and calls the callback function (in this case, ToBlob is the callback function.)  After the base class is done, the derived class updates the context to match the size.  Probably not the right order of operations.
For the security sensitive patch, something we could choose to uplift, just deal with the mismatched sizes and throw an error.

I'll take a look at how simple the non-wallpaper patch would be.
Attachment #8666198 - Flags: review?(gwright)
Attachment #8666198 - Flags: review?(gwright) → review+
Comment on attachment 8666198 [details] [diff] [review]
Part 1. Canvas and context should match sizes. r=gwright

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Medium - it is obvious that we're trying to protect the sizes mismatch, and they would need to trace our update implementation to figure out what is the time period during which these two values mismatch.  At that point, it isn't clear how easily they could exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Somewhat, but indirectly.

Which older supported branches are affected by this flaw?

All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Same risk, should be a simple rebase.

How likely is this patch to cause regressions; how much testing does it need?

Not likely; this was already part of the debug builds, and the extra tests that are done are not expensive enough to show up as a performance issue.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b295f4e4734
Attachment #8666198 - Flags: sec-approval?
Conversations with :smaug and :bz suggest that HTMLCanvasElement should be overriding BeforeSetAttr and/or AfterSetAttr instead of SetAttr itself.
Attachment #8667339 - Attachment is obsolete: true
Attachment #8667339 - Flags: review?(bugs)
Comment on attachment 8667340 [details] [diff] [review]
Part 2. Use AfterSetAttr instead of SetAttr with code after the base class call. r=smaug

Simpler code, isn't it :)

As far as I see UpdateContext() can't run any scripts so this should be safe.


Unrelated extra newline in HTMLCanvasElement::ToBlob
Attachment #8667340 - Flags: review?(bugs) → review+
Attachment #8666198 - Attachment description: Canvas and context should match sizes. r=gwright → Option 1. Canvas and context should match sizes. r=gwright
Attachment #8666198 - Flags: sec-approval?
Attachment #8667340 - Attachment description: Use AfterSetAttr instead of SetAttr with code after the base class call. r=smaug → Option 2. Use AfterSetAttr instead of SetAttr with code after the base class call. r=smaug
Here is the dilemma.

Option 1 catches the problem regardless of how it came to be.  The patch is larger, but trivially so, as most of the changes are exposing some previously debug only functions.  The operation fails because we detect inconsistencies in size.  On the other hand, while it stops the crash, it actually behaves incorrectly from the users' point of view.

Option 2 removes the reason the problem showed up in the first place, and is the way the code should be.  It has a higher chance of unwanted consequences, and doesn't cover potentially different path of getting to the same inconsistency (which is not likely, but there it is.)

If we decide to uplift, either one of these would fix the original problem on its own, but I'd probably be more comfortable uplifting option 1.
fwiw, option 2 looks rather safe to me, but sounds like we want both patches - to be safe.
(In reply to Olli Pettay [:smaug] from comment #14)
> fwiw, option 2 looks rather safe to me, but sounds like we want both patches
> - to be safe.

That's good to hear.  I was mostly worried about HTMLCanvasElement's immediate base class nsGenericHTMLElement which still overrides SetAttr, even though it also overrides BeforeSetAttr and AfterSetAttr, but I'm good with doing both.
Comment on attachment 8666198 [details] [diff] [review]
Part 1. Canvas and context should match sizes. r=gwright

[Security approval request comment]
See comment 8.
Attachment #8666198 - Attachment description: Option 1. Canvas and context should match sizes. r=gwright → Part 1. Canvas and context should match sizes. r=gwright
Attachment #8666198 - Flags: sec-approval?
Comment on attachment 8667340 [details] [diff] [review]
Part 2. Use AfterSetAttr instead of SetAttr with code after the base class call. r=smaug

[Security approval request comment]
See comment 8
Attachment #8667340 - Attachment description: Option 2. Use AfterSetAttr instead of SetAttr with code after the base class call. r=smaug → Part 2. Use AfterSetAttr instead of SetAttr with code after the base class call. r=smaug
Attachment #8667340 - Flags: sec-approval?
Attachment #8666198 - Flags: sec-approval? → sec-approval+
Comment on attachment 8667340 [details] [diff] [review]
Part 2. Use AfterSetAttr instead of SetAttr with code after the base class call. r=smaug

sec-approval+ for trunk.

Once it is in, we'll want patches made and nominated for Aurora, Beta, and ESR38.
Attachment #8667340 - Flags: sec-approval? → sec-approval+
Part 2 introduces a change in behaviour, though I'm not sure if the change is bad.  Before Part 2 patch, canvas context would be updated when a widht/height/transparency was set, whether it was set to the current value or not.  After this patch, the canvas context would only be updated if the new value is different from the old value.  This is because AfterSetAttr doesn't even get called if the new value is the same as the old value.

It feels right not to update things if they don't change, but the web-platform tests, the way they are currently written, are counting on things getting updated even if SetAttr is given the current value of the attribute being modified.  See, for example, https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/semantics/embedded-content/the-canvas-element/initial.reset.clip.html#25 where we set the width to 100 (it was already 100) and we expect the clip to reset.

I'd say the test is wrong, and though I'm not an expert on examining the standards spec, I can't find something in it that says that the context needs to be updated when the value is not changed.

In short though, I don't see us landing Part 2 any time soon.  Part 1 should still fix the problem.
> I'd say the test is wrong, and though I'm not an expert on examining the standards spec,
> I can't find something in it that says that the context needs to be updated when the
> value is not changed.

Sadly, its there.  https://html.spec.whatwg.org/multipage/scripting.html#canvas a ways down has this paragraph:

  Whenever the width and height content attributes are set, removed, changed, or
  redundantly set to the value they already have, if the canvas context mode is direct-2d,
  the user agent must set bitmap dimensions to the numeric values of the width and height
  content attributes.

where "set the bitmap dimensions" is https://html.spec.whatwg.org/multipage/scripting.html#concept-canvas-set-bitmap-dimensions which is what that test is testing.

So part 2 is in fact wrong, per spec.  Yay tests!

Of course this spec pretends mutation events don't exist...
Attachment #8666198 - Flags: checkin? → checkin+
Attachment #8667340 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] from comment #21)
> ...
> 
> So part 2 is in fact wrong, per spec.  Yay tests!
> 
> Of course this spec pretends mutation events don't exist...

I'm going to open a separate, non-security bug, for cleaning up HTMLCanvasElement::SetAttr.
Keywords: leave-open
Sorry for not realizing the spec can be so odd here.
We don't really have any good way to deal with this case.
One would need to override SetAttr after all, and before calling parent class' SetAttr push a scriptblocker on stack so that mutationevent will happen only once the scriptblocker goes out from the stack, not during calling parent's SetAttr.
Opened bug 1210812, and will pretend in it that we never had a security bug on the topic :)
https://hg.mozilla.org/mozilla-central/rev/f6c99c8baa9b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: gfx-core-security → core-security-release
Comment on attachment 8666198 [details] [diff] [review]
Part 1. Canvas and context should match sizes. r=gwright

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Buffer overflow, though we don't know if there is an easy exploit. 
[Describe test coverage new/current, TreeHerder]: web platform test the code around this.
[Risks and why]: Low - the methods used were available in debug, we are exposing them in release, and they are accessors, not setters.
[String/UUID change made/needed]:

sec-high - not sure if we need it for ESR?

I will attach an automated test for this, but that shouldn't land until all the releases have this fix.
Attachment #8666198 - Flags: approval-mozilla-beta?
Attachment #8666198 - Flags: approval-mozilla-aurora?
Yes, we need this for ESR38 as it is a sec-high issue affecting ESR.
Flags: sec-bounty? → sec-bounty+
Attachment #8666198 - Flags: approval-mozilla-beta?
Attachment #8666198 - Flags: approval-mozilla-beta+
Attachment #8666198 - Flags: approval-mozilla-aurora?
Attachment #8666198 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Reproduced the crash on 40.0.2 build 2, under Windows 7 64-bit → bp-7a191e8f-5e66-4347-919c-9486e2151021
Verified fixed with 42.0b8 (Build ID: 20151019161651), latest esr tinderbox build (Build ID: 20151020024209), latest 43.0a2 and 44.0a1 (from 2015-10-20), under Windows 7 64-bit and Windows 10 32-bit.
Whiteboard: [adv-main42+][adv-esr38.4+]
Alias: CVE-2015-7189
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.