820316 - Out of range read in memcpy called from android::GraphicBuffer::flatten

Status: RESOLVED DUPLICATE of bug 867996

(Core :: Graphics: Layers, defect)

Description

[Julian Seward [:jseward]]

b2g running on the emulator, every startup.  I don't know if this is a
bug in our code (as in: is this a bug in the android libs, or did we 
allocate a buffer of the wrong size?, etc), but filing anyway.

Treat the stacks with skepticism.  They are generated by stack
scanning (a nasty kludge at the best of times) and may mention
functions which aren't really present in the stacks.

Invalid read of size 4
   at 0x480AFA0: memcpy (mc_replace_strmem.c:883)
   by 0x4D92ADD: android::GraphicBuffer::flatten(void*, unsigned int, int*, unsigned int) const (in /system/lib/libui.so)
   by 0x48C8A45: std::__node_alloc::_M_allocate(unsigned int&) (in /system/lib/libstlport.so)
   by 0x4806C0B: realloc (vg_replace_malloc.c:662)
   by 0x5C52CE7: std::hashtable<std::pair<int const, IPC::Channel::Listener*>, int, std::hash<int>, std::priv::_HashMapTraitsT<std::pair<int const, IPC::Channel::Listener*> >, std::priv::_Select1st<std::pair<int const, IPC::Channel::Listener*> >, std::equal_to<int>, std::allocator<std::pair<int const, IPC::Channel::Listener*> > >::insert_unique_noresize(std::pair<int const, IPC::Channel::Listener*> const&) (_hashtable.c:223)
   by 0x5C7F681: mozilla::layers::PLayersParent::Write(mozilla::layers::MaybeMagicGrallocBufferHandle const&, IPC::Message*) (ipc_message_utils.h:74)
   by 0x5DD15C3: IPC::Message::Message(int, unsigned int, IPC::Message::PriorityValue, IPC::Message::MessageCompression, char const*) (ipc_message.cc:32)
   by 0x5C83267: mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PLayersParent.cpp:467)
   by 0x5C82D57: mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const&) (nsTArray.h:735)
   by 0x5C416B3: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (RPCChannel.cpp:400)

 Address 0x8669344 is 8 bytes after a block of size 76 alloc'd
   at 0x48071F4: operator new[](unsigned int) (vg_replace_malloc.c:357)
   by 0x6BF828B: ??? (in /system/lib/hw/gralloc.goldfish.so)
   by 0x4D92C31: android::GraphicBuffer::initSize(unsigned int, unsigned int, int, unsigned int) (in /system/lib/libui.so)
   by 0x4D92FA5: android::GraphicBuffer::GraphicBuffer(unsigned int, unsigned int, int, unsigned int) (in /system/lib/libui.so)
   by 0x5E450E9: mozilla::layers::GrallocBufferActor::Create(nsIntSize const&, gfxASurface::gfxContentType const&, mozilla::layers::MaybeMagicGrallocBufferHandle*) (ShadowLayerUtilsGralloc.cpp:206)
   by 0x5C52BF1: std::hashtable<std::pair<int const, IPC::Channel::Listener*>, int, std::hash<int>, std::priv::_HashMapTraitsT<std::pair<int const, IPC::Channel::Listener*> >, std::priv::_Select1st<std::pair<int const, IPC::Channel::Listener*> >, std::equal_to<int>, std::allocator<std::pair<int const, IPC::Channel::Listener*> > >::_M_insert_noresize(unsigned int, std::pair<int const, IPC::Channel::Listener*> const&) (_alloc.h:158)
   by 0x5E43747: mozilla::layers::ShadowLayersParent::AllocPGrallocBuffer(nsIntSize const&, gfxASurface::gfxContentType const&, mozilla::layers::MaybeMagicGrallocBufferHandle*) (ShadowLayersParent.cpp:502)
   by 0x5C52CE7: std::hashtable<std::pair<int const, IPC::Channel::Listener*>, int, std::hash<int>, std::priv::_HashMapTraitsT<std::pair<int const, IPC::Channel::Listener*> >, std::priv::_Select1st<std::pair<int const, IPC::Channel::Listener*> >, std::equal_to<int>, std::allocator<std::pair<int const, IPC::Channel::Listener*> > >::insert_unique_noresize(std::pair<int const, IPC::Channel::Listener*> const&) (_hashtable.c:223)
   by 0x5C82D57: mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const&) (nsTArray.h:735)
   by 0x5C416B3: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (RPCChannel.cpp:400)

Comment 1

[Julian Seward [:jseward]]

The above shows a 32-bit read 8 bytes after the end of the block.  In the
same run I also see reads with the same stack at 4 past the end of the block
and 0 past the end of the block.

Comment 2

[Julian Seward [:jseward]]

Here's my best guess at cleaned-up stacks.

Invalid read of size 4
   at 0x480AFA0: memcpy (mc_replace_strmem.c:883)
   by 0x4D92ADD: android::GraphicBuffer::flatten(void*, unsigned int, int*, unsigned int)
                 const (in /system/lib/libui.so)
   by 0x5C52CE7: std::hashtable<std::pair<int const, IPC::Channel::Listener*>, int,
                 std::hash<int>, std::priv::_HashMapTraitsT<std::pair<int const,
                 IPC::Channel::Listener*> >, std::priv::_Select1st<std::pair<int const,
                 IPC::Channel::Listener*> >, std::equal_to<int>,
                 std::allocator<std::pair<int const, IPC::Channel::Listener*> >
                 >::insert_unique_noresize(std::pair<in const, IPC::Channel::Listener*>
                 const&) (_hashtable.c:223)
   by 0x5C7F681: mozilla::layers::PLayersParent::Write(
                 mozilla::layers::MaybeMagicGrallocBufferHandle const&, IPC::Message*)
                 (ipc_message_utils.h:74)
   by 0x5DD15C3: IPC::Message::Message(int, unsigned int, IPC::Message::PriorityValue,
                 IPC::Message::MessageCompression, char const*) (ipc_message.cc:32
   by 0x5C83267: mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const&,
                 IPC::Message*&) (PLayersParent.cpp:467)
   by 0x5C82D57: mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const&)
                 (nsTArray.h:735)
   by 0x5C416B3: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (RPCChannel.cpp:400)

 Address 0x8669344 is 8 bytes after a block of size 76 alloc'd
   at 0x48071F4: operator new[](unsigned int) (vg_replace_malloc.c:357)
   by 0x6BF828B: ??? (in /system/lib/hw/gralloc.goldfish.so)
   by 0x4D92C31: android::GraphicBuffer::initSize(unsigned int, unsigned int, int, unsigned
                 int) (in /system/lib/libui.so)
   by 0x4D92FA5: android::GraphicBuffer::GraphicBuffer(unsigned int, unsigned int, int,
                 unsigned int) (in /system/lib/libui.so)
   by 0x5E450E9: mozilla::layers::GrallocBufferActor::Create(nsIntSize const&,
                 gfxASurface::gfxContentType const&,
                 mozilla::layers::MaybeMagicGrallocBufferHandle*)
                 (ShadowLayerUtilsGralloc.cpp:206)
   by 0x5C52BF1: std::hashtable<std::pair<int const, IPC::Channel::Listener*>, int,
                 std::hash<int>, std::priv::_HashMapTraitsT<std::pair<int const,
                 IPC::Channel::Listener*> >, std::priv::_Select1st<std::pair<int const,
                 IPC::Channel::Listener*> >, std::equal_to<int>,
                 std::allocator<std::pair<int const, IPC::Channel::Listener*> >
                 >::_M_insert_noresize(unsigned int, std::pair<int const,
                 IPC::Channel::Listener*> const&) (_alloc.h:158)
   by 0x5E43747: mozilla::layers::ShadowLayersParent::AllocPGrallocBuffer(nsIntSize const&,
                 gfxASurface::gfxContentType const&,
                 mozilla::layers::MaybeMagicGrallocBufferHandle*)
                 (ShadowLayersParent.cpp:502)
   by 0x5C52CE7: std::hashtable<std::pair<int const, IPC::Channel::Listener*>, int,
                 std::hash<int>, std::priv::_HashMapTraitsT<std::pair<int const,
                 IPC::Channel::Listener*> >, std::priv::_Select1st<std::pair<int const,
                 IPC::Channel::Listener*> >, std::equal_to<int>,
                 std::allocator<std::pair<int const, IPC::Channel::Listener*> >
                 >::insert_unique_noresize(std::pair<int const, IPC::Channel::Listener*> 
                 const&) (_hashtable.c:223)
   by 0x5C82D57: mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const&)
                 (nsTArray.h:735)
   by 0x5C416B3: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (RPCChannel.cpp:400)

Comment 3

[Benoit Girard (:BenWa)]

According to hg blame it likely comes from bug 765734 changeset:
http://hg.mozilla.org/mozilla-central/rev/089b9510e595

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Chris want to take this one?

Comment 5

[Daniel Veditz [:dveditz]]

picking chris based on likely regressor.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's try to keep Chris out of this :-)

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

We just went over "six weeks" with this one - does it still happen?  Kan-Ru, is this something you can look at in the short term?  Let us know either way.

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

I've never saw this on real devices. I would like to know if this is still reproducible on the emulator.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Kan-Ru Chen [:kanru] from comment #8)
> I've never saw this on real devices. I would like to know if this is still
> reproducible on the emulator.

Kan-Ru, I just added qawanted, let me know if you're asking somebody else to reproduce instead.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

ccing Tony to help find QA for this.

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Yes. We need to know if this is still reproducible in our emulator and/or devices. It looks like it's a issue for a specific revision of the android library.

Comment 12

[Mike Habicher [:mikeh] (high bugzilla latency)]

I've been investigating a crash in ::flatten() in bug 867996 (and by extension, bug 818103) that _only_ happens on automated-test builds (e.g. the try server) but not on local emulator builds.

If these two issues are related (and it looks strongly like they are), this is still happening: see https://bugzilla.mozilla.org/show_bug.cgi?id=818103#c686

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

In ::flatten()[1], the memcpy() is copying data out of an internal structure and into user-supplied buffers, the latter of which are sized based on information obtained by calling GraphicBuffer::getFlattenedSize()[2] and ::setFdCount()[3].

1. https://github.com/android/platform_frameworks_base/blob/ics-mr1-release/libs/ui/GraphicBuffer.cpp#L208
2. https://github.com/android/platform_frameworks_base/blob/ics-mr1-release/libs/ui/GraphicBuffer.cpp#L200
3. https://github.com/android/platform_frameworks_base/blob/ics-mr1-release/libs/ui/GraphicBuffer.cpp#L204

The source buffer object is type 'native_handle_t'[4].

4. https://github.com/marcofreda527/jb412gecko/blob/8b11d14dfa0dc5876eba1b5ca8a76ff88e10c839/media/omx-plugin/include/ics/cutils/native_handle.h#L30

Does Valgrind say where these objects are being allocated?

Comment 14

[Mike Habicher [:mikeh] (high bugzilla latency)]

Looks like the native_handle_t object is being allocated by a call to mallocDev->alloc(), a reference to which is obtained by calling hw_get_module(GRALLOC_HARDWARE_MODULE_ID) and gralloc_open().

jseward, how did you obtain the output in comment 0?  Valgrind running in the emulator?

Comment 15

[Mike Habicher [:mikeh] (high bugzilla latency)]

Okay, I have Valgrind running on a local emulator build (*PHEW!*) and with some extra debug prints in ParamTraits<MagicGrallocBufferHandle>::Write() and GrallocBufferActor::Create(), I see:

I(  100:0x83) aSize.width=320 .height=480 format=2
E(  100:0x83) mikeh: alloc: w=320 h=480
E(  100:0x83) mikeh: handle=0xc2e6c90, stride=320
I(  100:0x83) buffer.get()=0xc519000
I(  100:0x83) Trying to get flattenable, aMsg=0xaa7f8a0
I(  100:0x83) flattenable=0xc519000
I(  100:0x83) nbytes=104
I(  100:0x83) nfds=1
I(  100:0x83) flattening
E(  100:0x83) mikeh: h=0xc2e6c90 ->numFds=1 ->numInts=18 ->data=0xc2e6c9c ->data+numFds=0xc2e6ca0
I(  100:0x83) write nbytes
I(  100:0x83) write size
I(  100:0x83) write data
I(  100:0x83) write fd 0

This is on start-up, and Valgrind reports:

==100== Invalid read of size 4
==100==    at 0x480AF1C: memcpy (mc_replace_strmem.c:883)
==100==    by 0x4CE7B03: android::GraphicBuffer::flatten(void*, unsigned int, int*, unsigned int) const (GraphicBuffer.cpp:240)
==100==    by 0x630B8B1: IPC::ParamTraits<mozilla::layers::MagicGrallocBufferHandle>::Write(IPC::Message*, mozilla::layers::MagicGrallocBufferHandle const&) (ShadowLayerUtilsGralloc.cpp:56)
==100==    by 0x60580FD: mozilla::layers::PLayerTransactionParent::Write(mozilla::layers::MaybeMagicGrallocBufferHandle const&, IPC::Message*) (ipc_message_utils.h:115)
==100==    by 0x605B2FB: mozilla::layers::PLayerTransactionParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PLayerTransactionParent.cpp:551)
==100==    by 0x605301B: mozilla::layers::PCompositorParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PCompositorParent.cpp:361)
==100==    by 0x6015CC3: mozilla::ipc::SyncChannel::OnDispatchMessage(IPC::Message const&) (SyncChannel.cpp:145)
==100==    by 0x601466F: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (RPCChannel.cpp:400)
==100==    by 0x5FF31FB: RunnableMethod<mozilla::dom::TabChild, void (mozilla::dom::TabChild::*)(), Tuple0>::Run() (tuple.h:383)
==100==    by 0x6012EC9: mozilla::ipc::RPCChannel::DequeueTask::Run() (RPCChannel.h:425)
==100==    by 0x629710D: MessageLoop::RunTask(Task*) (message_loop.cc:337)
==100==    by 0x62980DF: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (message_loop.cc:345)
==100==  Address 0xc2e6ce0 is 0 bytes after a recently re-allocated block of size 80 alloc'd
==100==    at 0x4815B18: arena_malloc (jemalloc.c:4169)
==100==    by 0x4815CCF: imalloc (jemalloc.c:4247)
==100==    by 0x4815CF3: je_malloc (jemalloc.c:6302)
==100==    by 0x48120D3: malloc (replace_malloc.c:150)
==100==    by 0x4811D8F: operator new[](unsigned int) (mozmemory_wrap.c:20)
==100==    by 0x77C428B: ??? (in /system/lib/hw/gralloc.goldfish.so)

The address 0xc2e6ce0 is exactly 80 bytes after the native_handle_t at 0xc2e6c90, and GraphicBuffer.cpp:240 is:

   memcpy(&buf[8], h->data + h->numFds, h->numInts*sizeof(int));

If all of this is accurate, it means the native_handle_t object (which includes a terminal 'int data[0]' member) that is being allocated by gralloc.goldfish.so is either too small, or has its 'numInts' member too high by 1.

Comment 16

[Mike Habicher [:mikeh] (high bugzilla latency)]

format=2 is the value returned by calling PixelFormatForContentType()[1], and corresponds to PIXEL_FORMAT_RGBX_8888[2] and HAL_PIXEL_FORMAT_RGBX_8888[3].

1. http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#150
2. http://mxr.mozilla.org/mozilla-central/source/media/omx-plugin/include/ics/ui/PixelFormat.h#62
3. http://mxr.mozilla.org/mozilla-central/source/media/omx-plugin/include/ics/system/graphics.h#44

The only reasonable call to operator new() in this path is here[4].

4. http://androidxref.com/4.0.4/xref/development/tools/emulator/opengl/system/gralloc/gralloc.cpp#217

Comment 17

[Mike Habicher [:mikeh] (high bugzilla latency)]

It looks like sizeof( cb_handle_t ) = 76; cb_handle_t is a subclass of native_handle[_t], which has sizeof(native_handle_t)=12.  This means numInts _should_ be 64/6 = 16, not the 18 we are actually getting*.

*based on nbytes=104 as seen in comment 15[1].

1. http://androidxref.com/4.0.4/xref/frameworks/base/libs/ui/GraphicBuffer.cpp#200

Comment 18

[Mike Habicher [:mikeh] (high bugzilla latency)]

(Ahem... 64/4 = 16.)

Comment 19

[Mike Habicher [:mikeh] (high bugzilla latency)]

Okay, here's what's going on: when the cb_handle_t object is created, it starts off with numFds=0 and numInts=sizeof(cb_handle_t)/sizeof(int)=76/4=19.  Eventually gralloc_alloc() calls cb_handle_t.setFd(), which sets numFds=1 and numInts=(76-4)/4=18.

When this object makes it to ::flatten(), that function calls:

  memcpy(&buf[8], h->data + h->numFds, h->numInts*sizeof(int));

h->data points to the beginning of the ints that are specific to cb_handle_t.  These appear in memory after the members of native_handle_t, but h->numInts _includes_ the size of the native_handle_t superclass members as well, so this memcpy() will _always_ read off the end of the object.

This is a pretty horrid bug, but all of it exists in AOSP code, so there's no simple fix for us.

Comment 20

[Mike Habicher [:mikeh] (high bugzilla latency)]

Exercising a potential emulator fix:
https://tbpl.mozilla.org/?tree=Try&rev=a229b5394bb1

Comment 21

[Mike Habicher [:mikeh] (high bugzilla latency)]

A fix for the emulator landed in bug 867996:
  https://github.com/mozilla-b2g/android-development/pull/2

Bug 818103 will remain open to track uploading a new emulator build to the infrastructure.

With the above patch, I get a clean Valgrind run (at least as far as ::flatten() is concerned), I'm going to close this as a duplicate.