1184066 - Buffer overrun in android::AudioTrack::set when opening a AudioStream & cubeb backend

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Julian Seward [:jseward]]

[Please change Component and/or declassify as appropriate]

STR:
(1) apply patch below.  It disables the camera timeouts.

(2) build for Aries (Z3C), "./config.sh aries", run Valgrind as per
instructions at
https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Debugging_B2G_using_valgrind

(3) Select Camera app.  Take a photograph.  The buffer overruns are
reported when the "click" (fake old-style camera) noise is played.

Also at that point, a native run often crashes (for me).


-----------
diff --git a/apps/camera/bower_components/requirejs/index.js b/apps/camera/bower_components/requirejs/index.js
index 7f31fa2..032168c 100644
--- a/apps/camera/bower_components/requirejs/index.js
+++ b/apps/camera/bower_components/requirejs/index.js
@@ -201,7 +201,7 @@ var requirejs, require, define;
                 //Defaults. Do not set a default for map
                 //config to speed up normalize(), which
                 //will run faster if there is no default.
-                waitSeconds: 7,
+                waitSeconds: 0,
                 baseUrl: './',
                 paths: {},
                 bundles: {},

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaints

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Called from mozilla::AudioStream::OpenCubeb()

Comment 3

[Matthew Gregan [:kinetik]]

Just a guess but given the stack and sizes, it looks a bit like libwilhelm (where the OpenSL ES to AudioTrack wrapper lives) and libmedia (where AudioTrack lives) disagree on the size of the AudioTrack class.  A rough scan of AudioTrack shows ~52 members plus a vtable so ~212 bytes.  Not sure why this would happen as I think we build both libraries from source during the B2G build.

Comment 4

[Paul Adenot (:padenot)]

Are we using Sony's .so or building our own? I seem to recall Sony are known to change their .so quite a lot.

Alexandre, do you know what we do here? Specifically, do you know if we use the device's libmedia.so and libwilhelm.so, or if we build our own?

Comment 5

[:gerard-majax]

I already checked this yesterday on irc with Julian and it looks like we reuse libmedia.so blob but we build libwilhelm.so.

I cannot tell if we could build libmedia.

Comment 6

[Paul Adenot (:padenot)]

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> I already checked this yesterday on irc with Julian and it looks like we
> reuse libmedia.so blob but we build libwilhelm.so.
> 
> I cannot tell if we could build libmedia.

Could we not reuse libwilhelm.so ? We would otherwise need to patch it.

Comment 7

[:gerard-majax]

(In reply to Paul Adenot (:padenot) from comment #6)
> (In reply to Alexandre LISSY :gerard-majax from comment #5)
> > I already checked this yesterday on irc with Julian and it looks like we
> > reuse libmedia.so blob but we build libwilhelm.so.
> > 
> > I cannot tell if we could build libmedia.
> 
> Could we not reuse libwilhelm.so ? We would otherwise need to patch it.

We don't reuse it ...

Comment 8

[:gerard-majax]

> alex@portable-alex:~/codaz/Mozilla/b2g/devices/XperiaZ3c/B2G$ sha1sum ./out/target/product/aries/system/lib/libmedia.so backup-aries/system/lib/libmedia.so ./out/target/product/aries/obj/lib/libmedia.so
> 18a16fd89232793a093c8aa2057306ea7767eaa4  ./out/target/product/aries/system/lib/libmedia.so
> 18a16fd89232793a093c8aa2057306ea7767eaa4  backup-aries/system/lib/libmedia.so
> af8f4656d1c07422efde44d06d182a9c6c2a57cf  ./out/target/product/aries/obj/lib/libmedia.so
> alex@portable-alex:~/codaz/Mozilla/b2g/devices/XperiaZ3c/B2G$ ls -hal ./out/target/product/aries/system/lib/libmedia.so backup-aries/system/lib/libmedia.so ./out/target/product/aries/obj/lib/libmedia.so
> -rw-r--r-- 1 alex alex 566K févr. 12 08:34 backup-aries/system/lib/libmedia.so
> -rwxrwxr-x 1 alex alex 558K juil. 10 14:13 ./out/target/product/aries/obj/lib/libmedia.so
> -rw-r--r-- 1 alex alex 566K juil. 10 14:13 ./out/target/product/aries/system/lib/libmedia.so

Maybe we could also use built libmedia.so then?

Comment 9

[Paul Adenot (:padenot)]

I mean, either we build both from AOSP, or we reuse both from the device, but we can't mix and match, apparently.

Comment 10

[Julian Seward [:jseward]]

(In reply to Matthew Gregan [:kinetik] from comment #3)
> Just a guess but given the stack and sizes, it looks a bit like libwilhelm
> (where the OpenSL ES to AudioTrack wrapper lives) and libmedia (where
> AudioTrack lives) disagree on the size of the AudioTrack class.

My reading of it also points that way.  I would guess that the AOSP and Sony
versions of libmedia disagree on the size the AudioTrack class.  Or something
like that.

I managed to get some source locations for the libwilhelm.so frames that were
lacking them in the previous stacks.  The cleaned-up version is below, and it
makes it pretty clear that the mismatched class is android::AudioTrack.  That's
clear from frameworks/wilhelm/src/android/AudioPlayer_to_android.cpp:1446 in
the second stack below, since that's a call to its constructor.

Invalid write of size 4
   at 0x7B7C5A0: android::AudioTrack::set+63 (in /system/lib/libmedia.so)
   by 0x7B7CD15: android::AudioTrack::set+64 (in /system/lib/libmedia.so)
   by 0x7B7CE53: android::AudioTrack::AudioTrack+150 (in /system/lib/libmedia.so)
   by 0xDD13851: system/core/include/utils/StrongPointer.h:159
   by 0xDD22CB9: frameworks/wilhelm/src/itf/IObject.c:138
   by 0x68AE111: opensl_stream_init+508 (gecko/media/libcubeb/src/cubeb_opensl.c:611)

 Address 0xb3fb870 is 4 bytes after a block of size 212 alloc'd
   at 0x489B860: operator new[](unsigned int)+136 (vg_replace_malloc.c:386)
   by 0xDD1381D: frameworks/wilhelm/src/android/AudioPlayer_to_android.cpp:1446
   by 0xDD22CB9: frameworks/wilhelm/src/itf/IObject.c:138
   by 0x68AE111: opensl_stream_init+508 (gecko/media/libcubeb/src/cubeb_opensl.c:611)
   by 0x68AD8D5: cubeb_stream_init+120 (gecko/media/libcubeb/src/cubeb.c:230)
   by 0x603DC23: mozilla::AudioStream::OpenCubeb+70 (gecko/dom/media/AudioStream.cpp:503)

Comment 11

[:gerard-majax]

(In reply to Paul Adenot (:padenot) from comment #9)
> I mean, either we build both from AOSP, or we reuse both from the device,
> but we can't mix and match, apparently.

Either ways, it's not AOSP but CAF. I'll check if we can push libmedia.so built from sources instead of blob one.

Comment 12

[:gerard-majax]

Bad news, pushing libmedia.so built from source fails. Gecko does not boot anymore. So you'll have to use blob for the other one:
> adb root && adb wait-for-device && adb remount && adb wait-for-device && adb push backup-aries/system/lib/libwilhelm.so /system/lib/ && adb reboot

Comment 13

[Julian Seward [:jseward]]

(In reply to Alexandre LISSY :gerard-majax from comment #12)
I tried this.  IIUC, the result is to use the blob versions of both
libmedia and libwilhelm.  This stops the crashing but unfortunately
sound doesn't work at all now, and youtube videos won't play any more.
So, not sure where to go from there.

I spent quite a lot of time today running Valgrind on the Z3c.  I get
the impression that this heap corruption (with the mismatched lib pair)
is causing crashing or potential crashing at any place where the phone
has to make a sound -- which is a lot of places.  I saw the heap
corruption in Valgrind, for example, also when playing youtube videos,
and when getting a "file transfer failed" audio notification from the
Bluetooth file-transfer facility.

Comment 14

[Gregor Wagner [:gwagner]]

What are the next steps here and who will take this bug?

Comment 15

[Paul Adenot (:padenot)]

Can we simply try to push to the device both libraries that we have pulled from it?

Comment 16

[:gerard-majax]

(In reply to Paul Adenot (:padenot) from comment #15)
> Can we simply try to push to the device both libraries that we have pulled
> from it?

As explained in comment 13, doing so breaks the sound playback.

Comment 17

[Paul Adenot (:padenot)]

I think we need someone to sit down with a debugger to fix this, and that means we'd carry a patch on some part of the CAF tree.

Alexandre, can you explain me how I can change the compile options and the like of what we build ? I'll then do some investigation, I know quite a lot of the android audio stack, so I should be able to find why we have a mismatch.

It's not very urgent, I have other things to do first.

Comment 18

[Daniel Veditz [:dveditz]]

Moving to Firefox OS because it's more of a build-config/packaging issue with mismatched binary blobs, not a code bug in our cubeb backend code. Asking someone to take a photo isn't a very effective trigger for an exploit, but there's no reason to believe web or app content couldn't do the same thing on their own without user interaction.

I assume media runs in the app process, but if it's in the parent Gecko we should re-rate this sec-critical.

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Blake - can you find someone to resolve this?

Comment 20

[Blake Wu [:bwu][:blakewu]]

We use Sony's libmedia.so, so I think the root cause should be as comment 4.
I think many problems are caused by the same root cause, like bug 1148049 comment 9 and bug 1166758 comment 34. It would be better to ask Sony to provide the AudioTrack.h and we can know what is their changes. 
I will find the contact window to make this request. If anyone knows the contact window, please let me know. Thanks!

Comment 21

[Blake Wu [:bwu][:blakewu]]

Hi Alin, 
May I have your help to check if Z3C's (KK 4.4.2) AudioTrack is different from AOSP's.

Comment 22

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

After replacing AudioTrack.h with the version partner provided and rebuilt gecko & libwilhelm.so, Valgrind still output the same complaint. That means the problem is not due to mismatched library.
This problem should be inside the function

android::AudioTrack::set(audio_stream_type_t, unsigned int, audio_format_t, unsigned int, drm_flags_t, int, audio_output_flags_t, void (*)(int, void*, void*), void*, int, android::sp<android::IMemory> const&, bool, int, android::AudioTrack::transfer_type, audio_offload_info_t const*, int, unsigned int, audio_dsee_info_t const*),

which is a new added overloaded function by partner.

We need partner to help check its implementation. It's quite possible that aries KK Android also has the same problem.

Comment 23

[Blake Wu [:bwu][:blakewu]]

(In reply to Munro Chiang [:munro_chiang] from comment #22)
> After replacing AudioTrack.h with the version partner provided and rebuilt
> gecko & libwilhelm.so, Valgrind still output the same complaint. That means
> the problem is not due to mismatched library.
> This problem should be inside the function
> 
> android::AudioTrack::set(audio_stream_type_t, unsigned int, audio_format_t,
> unsigned int, drm_flags_t, int, audio_output_flags_t, void (*)(int, void*,
> void*), void*, int, android::sp<android::IMemory> const&, bool, int,
> android::AudioTrack::transfer_type, audio_offload_info_t const*, int,
> unsigned int, audio_dsee_info_t const*),
> 
> which is a new added overloaded function by partner.
> 
> We need partner to help check its implementation. It's quite possible that
> aries KK Android also has the same problem. 
Is it possible that we can flash Z3C to Android KK image to check if it has the same problem?

Comment 24

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Due to the package name length limitation, I cannot run valgrind to monitor Android KK camera app (com.sonyericsson.android.camera). Detail: http://stackoverflow.com/questions/19011887/how-do-i-run-valgrind-with-an-android-app

I run valgrind-3.10.1 with another Google play app (Metronome Beats) which use audiotrack.
There is no such alarm.

It seems the valgrind alarm is relate to the parameters Gecko camera pass to AudioTrack.
However, it is difficult to figure out the root cause without the source code of AudioTrack:set().

Alin, is it possible for you to share AudioTrack.cpp (23.0.1.A.5.77) with us for debugging purpose?

Comment 25

[Paul Adenot (:padenot)]

(In reply to Munro Chiang [:munro_chiang] from comment #22)
> After replacing AudioTrack.h with the version partner provided and rebuilt
> gecko & libwilhelm.so, Valgrind still output the same complaint. That means
> the problem is not due to mismatched library.
> This problem should be inside the function

Maybe they are using a different libwilhelm.so that calls differently into libmedia.so.

Comment 26

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Paul, this bug has been sitting awhile waiting for a response. See comment 21. 

Can we find someone else to poke at it? Or enlist other help from the party responsible for this code?

Comment 27

[Paul Adenot (:padenot)]

(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #26)
> Hi Paul, this bug has been sitting awhile waiting for a response. See
> comment 21. 
> 
> Can we find someone else to poke at it? Or enlist other help from the party
> responsible for this code?

This is a bug in Gonk, and we've not been able to get the right file to debug. Work in happening in bug 1166758 to mitigate/solve this.

Comment 28

[Blake Wu [:bwu][:blakewu]]

ni Juan to see if the patch in bug 1200111 can fix this bug.

Comment 29

[Juan Gomez [:_AtilA_] (CET/CEST)]

Well, it fixes that bug, but then the rest of the sounds in the system stop working (see comment 8 bug 1200111)
I'm not familiar with the sound system code base, and cannot spend time on this right now, so I'd suggest that someone familiar with the code apply my patch and debug to see where's the problem then.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Is this issue device specific? If so, does it matter anymore?

Comment 31

[Paul Adenot (:padenot)]

Yes, z3c, and I can't say I care anymore.