Softcrash on invalid ID3 validation

[Zyphuris55]

Issue description

ExoPlayer 2.x isn't supporting corrupt ID3 tags while the rest of the audio is still playable.

Reproduction steps

1. In the demo app, add a new media source linking to "http://stream.audiobooks.com/STREAM/SABLIB9784075.mp3?AudTTL=1490460462&AudSig=578c4100af873ae93e2dd69ec36ccf3a&rs=68&ri=2000000&ib=13&fs=1544000"
2. (attempted) playing the audio
3. ExoPlayer refuses to play content due to a crash while validating the ID3v2.4 data

com.google.android.exoplayer2.demo E/LoadTask: Unexpected exception loading stream
  java.lang.IllegalStateException: Top bit not zero: -388812344
    at com.google.android.exoplayer2.util.ParsableByteArray.readUnsignedIntToInt(ParsableByteArray.java:370)
    at com.google.android.exoplayer2.metadata.id3.Id3Decoder.validateV4Frames(Id3Decoder.java:224)
    at com.google.android.exoplayer2.metadata.id3.Id3Decoder.decode(Id3Decoder.java:134)
    at com.google.android.exoplayer2.extractor.mp3.Mp3Extractor.peekId3Data(Mp3Extractor.java:324)
    at com.google.android.exoplayer2.extractor.mp3.Mp3Extractor.synchronize(Mp3Extractor.java:242)
    at com.google.android.exoplayer2.extractor.mp3.Mp3Extractor.sniff(Mp3Extractor.java:147)
    at com.google.android.exoplayer2.source.ExtractorMediaPeriod$ExtractorHolder.selectExtractor(ExtractorMediaPeriod.java:701)
    at com.google.android.exoplayer2.source.ExtractorMediaPeriod$ExtractingLoadable.load(ExtractorMediaPeriod.java:636)
    at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:295)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
    at java.lang.Thread.run(Thread.java:818)
com.google.android.exoplayer2.demo E/EventLogger: internalError [0.55, loadError]
  com.google.android.exoplayer2.upstream.Loader$UnexpectedLoaderException: Unexpected IllegalStateException: Top bit not zero: -388812344
    at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:317)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
    at java.lang.Thread.run(Thread.java:818)
  Caused by: java.lang.IllegalStateException: Top bit not zero: -388812344
    at com.google.android.exoplayer2.util.ParsableByteArray.readUnsignedIntToInt(ParsableByteArray.java:370)
    at com.google.android.exoplayer2.metadata.id3.Id3Decoder.validateV4Frames(Id3Decoder.java:224)
    at com.google.android.exoplayer2.metadata.id3.Id3Decoder.decode(Id3Decoder.java:134)
    at com.google.android.exoplayer2.extractor.mp3.Mp3Extractor.peekId3Data(Mp3Extractor.java:324)
    at com.google.android.exoplayer2.extractor.mp3.Mp3Extractor.synchronize(Mp3Extractor.java:242)
    at com.google.android.exoplayer2.extractor.mp3.Mp3Extractor.sniff(Mp3Extractor.java:147)
    at com.google.android.exoplayer2.source.ExtractorMediaPeriod$ExtractorHolder.selectExtractor(ExtractorMediaPeriod.java:701)
    at com.google.android.exoplayer2.source.ExtractorMediaPeriod$ExtractingLoadable.load(ExtractorMediaPeriod.java:636)
    at com.google.android.exoplayer2.upstream.Loader$LoadTask.run(Loader.java:295)

Returned ID3 data (bytes):

73 68 51 4 0 0 0 0 1 105 84 82 67 -12 -84 70 105 108 -97 119 -1 -28 -88 26 -96 56 16 16 115 9 -106 42 -121 61 0 42 126 121 5 105 -111 -107 20 49 13 -115 44 64 -67 -88 106 -102 66 -60 -69 -44 -98 99 -19 -17 -11 127 -93 -4 -1 -47 -24 -3 -73 127 -89 -77 -89 -39 -87 127 -1 -11 -81 -21 -123 -128 120 -20 69 111 18 -54 13 40 -39 79 0 50 -45 9 18 -86 79 57 81 90 -82 111 -82 -81 75 -57 -102 21 69 109 98 -22 33 40 -118 44 52 69 53 91 -102 -43 21 26 -106 -84 84 -64 22 31 60 89 63 -4 127 -1 -13 -126 100 57 18 41 71 86 123 61 15 127 10 64 2 46 52 0 0 0 29 -89 29 39 -33 17 55 63 -5 -39 -31 -67 -76 67 44 -127 67 36 59 6 -25 -71 118 -117 46 115 -73 -71 26 15 11 122 63 -64 58 19 -126 72 98 43 -45 -124 -95 41 -90 -12 61 -98 -5 -68 56 -8 96 82 42 51 -105 -109 95 -30 6 -73 -104 -126 112 -5 -89 22 31 20 -3 76 94 -88 42 37 20 124 107 84 -58 -128 125 -38 -66

Proposed fix

id3Decoder.decode(...) @ ln 131

boolean unsignedIntFrameSizeHack = false;
if (id3Header.majorVersion == 4) {
   try {
      if (!validateV4Frames(id3Data, false)) {
         if (validateV4Frames(id3Data, true)) {
            unsignedIntFrameSizeHack = true;
         } else {
            Log.w(TAG, "Failed to validate V4 ID3 tag");
            return null;
         }
      }
   } catch (Exception e) {// ADDED CATCH CASE
      Log.w(TAG, "Failed to validate V4 ID3 tag");
      return null;
   }
}

Audio plays successfully with the proposed fix. ID3 tag is scrambled, attempt to salvage anything wouldn't work anyway. Audiobooks is also being notified of their corrupt book, but the problem could happen with others too (outside of audiobooks' books).

Version of ExoPlayer being used

v2.2.0 and v2.3.0

Device(s) and version(s) of Android being used

• Custom device using Android v5.1 (Api 22)
• Samsung Galaxy S7 using Android v6.0.1 (Api 23)
  Issue is reproducible on any device used

A full bug report captured from the device

Issue is linked to known corrupt ID3 tags, device report not needed.

[rustyshelf]

We have this same issue playing some downloaded files. Sample mp3 attached. It feels like ID3 parse errors for most fields shouldn't stop playback?
sample.zip

[ojw28]

I think we're already at the point where most ID3 parse errors don't stop playback. I'm hesitant to put a big try/catch block around the whole thing; we're pretty close. Seems there are still a few edge cases we're failing on, however.

We'll get the two raised here fixed. After those fixes go in, please let us know if you're still encountering failures. We can then decide whether the continue playing whack-a-mole for a little bit longer, or to give up and go for the brute force try/catch.

[ojw28]

Pretty sure the issues here are fixed in dev-v2. Please give it a try. Yet more robustness fixes will arrive as a result of #2663.

[Zyphuris55]

Yup, issue is fixed and media plays. Any update on when it would be moved into release for maven/bintray access? (as opposed to building locally; company doesn't like using a local aar, even if unchanged from master)

[ojw28]

Sometime this week, with any luck.

[kashif135]

Is this fix released? If yes which version please?

[ojw28]

Fixed in 2.4.0. By the way, you can find this out by clicking on the referenced commit(s) in the issue thread and then looking at the tags listed at the bottom of the commit message :) (it's not obvious, but they're there!).

[kashif135]

Sure. Thanks @ojw28