Menu

#201 malloc: *** error for object: incorrect checksum for freed object - object was probably modified after being freed

1.16.x
closed-fixed
nobody
None
7
2015-02-19
2013-12-31
No

when I was trying to skip several frames of a "special mp3 file", the mpg123 exits unexpectedly. This occasion may not be reproduced every time, but please try more than once.For debugging purpose, I've attached the "special mp3 file" in the post(or you can download via: https://www.dropbox.com/s/2zhc719de49eeap/65928.mp3 ). The bug should be reproduced via mpg123 -k 10 65928.mp3.I've tested it on 2 independent environments, and the outputs are:

(Linux 3.0.36-gentoo w/ mpg123 1.15.4)

$ mpg123 -k 10 65928.mp3                                                                                                                                
High Performance MPEG 1.0/2.0/2.5 Audio Player for Layers 1, 2 and 3
    version 1.15.4; written and copyright by Michael Hipp and others
    free software (LGPL/GPL) without any warranty but with best wishes

Note: Illegal Audio-MPEG-Header 0x00000000 at offset 4455.
Note: Trying to resync...
Note: Skipped 844 bytes in input.
Directory: ***
Playing MPEG stream 1 of 1: 65928.mp3 ...

MPEG 1.0 layer III, 320 kbit/s, 44100 Hz joint-stereo
mpg123: malloc.c:2451: sYSMALLOc: Assertion `(old_top == (((mbinptr) (((char *) &((av)->bins[((1) - 1) * 2])) - __builtin_offsetof (struct malloc_chunk, fd)))) && old_size == 0) || ((unsigned long) (old_size) >= (unsigned long)((((__builtin_offsetof (struct malloc_chunk, fd_nextsize))+((2 * (sizeof(size_t))) - 1)) & ~((2 * (sizeof(size_t))) - 1))) && ((old_top)->size & 0x1) && ((unsigned long)old_end & pagemask) == 0)' failed.
[1]    21947 abort      mpg123 -k 10 65928.mp3

(MacOS 10.9 Mavericks w/ mpg123 1.16.0)

$ mpg123 -k 10 -n 20 ~/Dropbox/Public/tmp/65928.mp3
High Performance MPEG 1.0/2.0/2.5 Audio Player for Layers 1, 2 and 3
    version 1.16.0; written and copyright by Michael Hipp and others
    free software (LGPL) without any warranty but with best wishes

Note: Illegal Audio-MPEG-Header 0x00000000 at offset 4455.
Note: Trying to resync...
Note: Skipped 844 bytes in input.
Directory: ~/Dropbox/Public/tmp/
Playing MPEG stream 1 of 1: 65928.mp3 ...

MPEG 1.0 layer III, 320 kbit/s, 44100 Hz joint-stereo
Title:   Sail For The River
Artist:  ÁÖÂ¡è¯ / Nat King Cole / Judy Garland / ÉñβÏÜÒ» / ¸ÇÂå¿É˹»ù / °Â˹²®É­ / °¬Ë¹Ìؼª°ØÍÐ / ³ÂÈð±ó / ÜïµÙ¼ÎÂ× / ÄǾ©¸ß / Rueibin Chen
Comment: 3102232_4717681021106_1         Album:  ³¬ÍêÃÀ¸æ°×/Perfect Unbosom
Year:    2008                            Genre:  Pop

[0:00] Decoding of 65928.mp3 finished.
mpg123(20409,0x7fff75440310) malloc: *** error for object 0x7fbfe5808208: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
[1]    20409 abort      mpg123 -k 10 -n 20 ~/Dropbox/Public/tmp/65928.mp3
1 Attachments

Discussion

  • Thomas Orgis

    Thomas Orgis - 2014-01-01

    Oh, you found a good one. The stream first indicates layer 1 (384 samples per frame) and then the change to the proper format with 1152 samples gets lost during the combination of seek and resync you have there. Subsequent decoding corrupted memory because of the buffer for 384 samples. What a frighteningly nice classic buffer overflow.

    I should be ashamed.

    Anyhow, can you verify using the sources from http://mpg123.org/snapshot ? The relevant change is in revision 3452 (http://mpg123.de/cgi-bin/scm/mpg123?view=revision&revision=3452). This sounds like something warranting a new release ASAP.

     
    • PAN Myautsai

      PAN Myautsai - 2014-01-02

      Thanks for your quick reply! The snapshot version runs as expected now. Thank you again for your quick fix!

       
    • PAN Myautsai

      PAN Myautsai - 2014-01-04

      Hi Thomas, I'm afraid there is probably another related bug in mpg123_scan .

      the code in below link should reproduce the bug in a specific condition:

      file-mpg123_bug_test.c

      I tested on

      • OSX 10.9 w/ snapshot version 20140102000000
      • Ubuntu w/ mpg123 1.12.1
      • Ubuntu w/ 20140102000000

      and all of them are properly installed.

      But only Ubuntu w/ 20140102000000 could reproduce the bug. I think it can also be easily reproduced on other Linux distributions.(~/sample_bad.mp3 is 65928.mp3):

      $ ./mpg123_bug_test ~/sample_bad.mp3
      MPG123_API_VERSION: 38, scan_flag: 1, start_frame: 0
      *** glibc detected *** ./mpg123_bug_test: double free or corruption (!prev): 0x0000000000de3290 ***
      *** glibc detected *** ./mpg123_bug_test: malloc(): memory corruption: 0x0000000000de3ec0 ***
      

      I debugged using valgrind:

      $ valgrind ./mpg123_bug_test ~/sample_bad.mp3
      
      Thread 1: status = VgTs_Runnable
      ==666==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
      ==666==    by 0x4E39CCC: INT123_frame_outbuffer (frame.c:199)
      ==666==    by 0x4E48296: INT123_decode_update (libmpg123.c:553)
      ==666==    by 0x4E4843E: get_next_frame (libmpg123.c:587)
      ==666==    by 0x4E48B17: mpg123_decode (libmpg123.c:964)
      ==666==    by 0x400BFB: process (mpg123_bug_test.c:42)
      ==666==    by 0x400CBB: main (mpg123_bug_test.c:79)
      

      It seems that the bug was introduced in a commit between 1.12.1 and the latest snapshot. I'm not familiar with the code, so could you please check the code again? Thanks very much for your efforts on the project.

      BTW, the bug is firstly found in my fork of Yaafe where mpg123 is used for decoding mp3 files. I want to decode just a portion of a given mp3(by given time_start and time_limit ). Without mpg123_scan, the start_frame may be a wrong value, but if I add mpg123_scan, the bug raises with 65928.mp3 and results to a segmentation fault unexpectedly. Anyway, the bug is very rare that only happens on some rare songs like 65928.mp3, but I think it is a very good sample for capability verification.

       

      Last edit: PAN Myautsai 2014-01-04
  • Thomas Orgis

    Thomas Orgis - 2014-01-04

    Yeah, you didn't see a release yet because I am still ironing out this parser logic. The promlematic behaviour was introduced in a parser rework in the 1.14 series. I'll include this second one in my tests.

    I'm fighting with keeping the sample accuracy (strict meaning: Whenever position x is reached, PCM value y should be returned, regardless of jumping around in stream) and ensuring correct frame buffers.

     
  • Thomas Orgis

    Thomas Orgis - 2014-01-06

    Would you check the current snapshot? I integrated both your described cases into our regression tests and they pass currently. Once we got this confirmed, I intend to produce patches for mpg123 >= 1.14.x to help those distros that need fixes but don't like updating versions.

    Note that, while the regression test for sample accurate seeking (reproducability of PCM values) works with undamaged files, your test file screws that up because of the first section appearing like some MPEG layer 1 frames (384 samples per frame instead of 1152 in the main data). I think I'll add some diagnostic for such, pardon the term, retarded streams to remove the MPG123_ACCURATE property so that you can query that seeking positions have some error.

     
    • PAN Myautsai

      PAN Myautsai - 2014-01-13

      Hi Thomas, where can I download the patches mentioned above or they're still in progress?

       
      • Thomas Orgis

        Thomas Orgis - 2014-01-13

        Am Mon, 13 Jan 2014 14:20:22 +0000
        schrieb "PAN Myautsai" mckelvin@users.sf.net:

        Hi Thomas, where can I download the patches mentioned above or they're still in progress?

        The current mpg123 trunk has all of my work so far. This is contained
        in the current http://mpg123.org/snapshot (created nightly).

        svn diff -r 3452:3466 svn://scm.orgis.org/mpg123/trunk
        

        should show all related (and some unrelated) changes. I'm still not
        sure if I will add something to detect inconsistent samples per
        frame ... perhaps triggering non-accurate mode and using a mean
        samples-per-frame value computed during scan. The other way would be to
        rework the frame index to contain samples, too. Actually, the whole
        dealing with frames to indicate position falls apart.

        I guess I'll rather find a way to make the parser more smart ... or at
        least document the issue clearly: If you got streams consisting out of
        rather incompatible parts (mp2 and mp3 intermixed), seeking will not
        work properly.

        You could help me by veryfying that this indeed is the only (known)
        problem left with the current trunk version.

         

        Last edit: Thomas Orgis 2014-01-13
  • PAN Myautsai

    PAN Myautsai - 2014-01-06

    The latest snapshot works fine, but as you mentioned, there are some error in seeking. When I was trying to seek the last 10 second of 65928.mp3, it returns the position of last 30s in the snapshot, but in the version 1.12.1, it will return the position of last 10s correctly, although it does not matters too much.I don't know whether this should be treated as a bug or a feature.

     
  • Thomas Orgis

    Thomas Orgis - 2014-01-06

    I'll have a look at the difference in seeking behaviour. 20 seconds sounds like a bit much of a difference for 844 bytes of junk in between.

    Is the data you get from 1.12.1 actually correct or does it only claim that it's at position 10 s from end? I.e., does it indeed deliver the last 10 seconds on decoding?
    One one hand, this is a damaged file and the emphasis of the decoder is to survive through it. Seeking is bound to be broken if the file is broken. But I would rather have some MPEG frames' worth of error margin than whole 20 seconds!

    Note that, while inside the short section of apparent MPEG layer 1 frames (is that deliberate?), mpg123 assumes that all frames in the file contain 384 audio samples. Then, on hitting the actual data, this switches to 1152 samples. So, seeking while on position 0 might differ wildly from seeking when some frames into the file.
    A normal reaction of the MPEG parser would be to decode the first few frames (2 or so) as MPEG layer 1 and then abort as the stream does not continue. I wanted to make it so smart to be able to continue to decode somehow. Combined with the promise to handle seeking, this becomes quite a challenge.

     
  • Thomas Orgis

    Thomas Orgis - 2014-01-06

    Addendum: CRC check might help in this specific case, from madplay.

    error: frame 0: CRC check failed
    error: frame 1: lost synchronization

    From then on it plays and since it didn't try decoding frame 0 and 1, no squeaking is to be heard. Now, mpg123 doesn't do the CRC check, as it is wasted computing time, especially with streaming over TCP, which does its own error checking. One might add it as an option ... or perhaps just for these situations when searching the first header in the stream or doing resync. On the other hand, then folks will come with files that have frames with correct CRC intermixed.

    It might really help to know how that junk went into that file. Is it some meta data? I didn't look closer yet.

     
  • PAN Myautsai

    PAN Myautsai - 2014-01-06

    Sorry I don't known much about the inner details of the MPEG-1/2 standard yet, But I know it's quite not easy to build a RIGHT mp3 decoder as there are too many bad case mp3 files.

    It is a little bit hard to reproduce in just one .c file without any other dependencies, but you can download and install the Yaafe, follow install.sh and build/install it. Then run python examples/frames.py 65928.mp3 this should plot exactly the last 10s(you can compare that with the waveform of 65928.mp3 using Sonic Visualiser ) if a version 1.12.1 mpg123 is compiled with Yaafe. There is still another strange issue, in example/frames.py, TIME_START is a parameter to control the position where to start decoding, a negative value means the last abs(TIME_START) second. TIME_LIMIT is a parameter to control the max duration to decode. if TIME_LIMIT = 0, it means no max duration limit. TIME_START = -10; TIME_LIMIT=0 and TIME_START = -10; TIME_LIMIT=10 is supposed to yield two same(near-same) value, but actually they differs greatly(one has a final duration of 10s and another one is 3s), even in version 1.12.1. The detail of implement is here.I'm not sure that I'm on the right of decoding, may be there is a bug in my code of decoding. Can you help me review it?

     
  • Thomas Orgis

    Thomas Orgis - 2014-01-07

    Sorry, I have trouble sparing time for mpg123 alone:-( The seeking test should be easy after all. Perhaps we can summarize the mpg123 usage:

    1. mpg123_open()
    2. mpg123_scan() (?)
    3. mpg123_seek(handle, 10*44100, SEEK_END)
    4. mpg123_read() till end

    Is that about it? Or do you jump/decode into the file first? The question of mpg123_scan() there is crucial. Of course, if the 384 samples per frame from the beginning are assumed for the seek, mpg123 would try to seek past the end. So, it must be something different here, but related to the funky file, of course.

     
    • PAN Myautsai

      PAN Myautsai - 2014-01-08

      after mpg123_open, there are mpg123_format and some other stuff. I wrote a simpler version where only mpg123 related code is kept. Am I walking in the right way?

       
  • Thomas Orgis

    Thomas Orgis - 2014-01-08

    Thanks for taking care to prepare the nice compact test case. One information is that you use frame-based seeking. Also I notice potential trouble with the buffer size ... even if you fixed the output format (which you didn't by the way ... mpg123_format_none() beforehand is needed): When the samples per frame change from 384 to 1152 ... you don't get proper advance notice about the changed output block size. You get an error indicator, though:

    if(mh->buffer.size < mh->outblock) return MPG123_NO_SPACE;

    The mpg123 console app just uses mpg123_safe_buffer(), the maximum possible output size per frame. Though, granted, that usually wastes memory when you don't use resampling.

    Anyhow, I'll have a close look at that seeking behaviour and will give you either feedback on what you did wrong or fix what I did wrong in mpg123;-)

     
  • Thomas Orgis

    Thomas Orgis - 2014-01-09

    Just noticed: mpg123_read() doesn't care for your buffer size. So, ignore my rambling about safe size.

     
  • Thomas Orgis

    Thomas Orgis - 2014-01-09

    First thing about the seeking with current trunk mpg123: Yes, the calculations are done with 384 samples per frame at the beginning ... then things switch to 1152 samples. Of course the frame calculations have to be wrong.

    The issue with the limiting is a misunderstanding about the decoding API: You insist on doing the frame calculations yourself but use mpg123_read(). mpg123_read() wraps the frame decoding and advancing, as well as possible cutting of samples for gapless decoding. If the buffer size happens to match the size of one decoded frame, you get one frame advancement per mpg123_read() call in average. But it's not definite and also unnecessary work on your side.

    Why don't you rather work with mpg123_seek() and mpg123_read(), just focusing on counting PCM samples and converting those to/from seconds via the sample rate? When you do not care about MPEG internals, you do not need to talk to libmpg123 using MPEG frames. This would only make sense if you also used mpg123_decode_frame(), avoiding memcpy() in mpg123_read.

    Still, libmpg123 will be confused by the junk data at the beginning. A hack to fix the seeking behaviour is to insert

    mpg123_seek_frame(mh, 10, SEEK_SET);
    

    before doing any position work (matching -k 10 in your initial mpg123 command). I'll think about making mpg123 more robust with that, at least if you already did scanning. Though, I am still undecided about really adding overhead to normal operation for seeking accurately in such nasty streams. You'd get a better deal when I implement more thorough checking of initial frames (CRC) so that those funny 384-sample-frames are ignored to begin with.

    Damn, now that I made the decoder flexible enough to actually be able to decode wildly concatenaded streams with differing properties, I am faced with fixing up seeking to work with those, too! Basically, this would need more storage in the frame index (store sample offset, too) and constant checking of mpg123_spf()... all nice to have, but not really requested intentionally by anyone... so, some little pain for just about no gain.

    Note, that, when you need to work with the decoded data in more elaborate ways and be sure of positions however damaged the file, it might be practical to just decode the whole track into memory and work on the raw PCM data. In times of several gigabytes of RAM in a PC, this works for hour-long audio tracks.

     
  • PAN Myautsai

    PAN Myautsai - 2014-01-11

    Thanks for your kindly detailed explanation. mpg123_seek_frame(mh, 10, SEEK_SET); is a solution but it makes the code more complex and hard to describe. Another solution you mentioned is to read all samples and clip only specific range. It's easy to implement if only the start position is always positive(start from the beginning), but not when the start position is a negative position(say -x: start from the last x seconds) in a data flow programming system such as Yaafe. At last, I decide to keep that bug as a feature so long as the program will not quit unexpectedly, as the junk mp3 file should be blame, and for most mp3 files this bug rarely happens.

     
  • Thomas Orgis

    Thomas Orgis - 2014-01-27

    OK, one addendum: While I did not fix the seeking troubles, I removed the audible glitch when playing your test file. With current trunk (snapshot not yet), you should see such:

    High Performance MPEG 1.0/2.0/2.5 Audio Player for Layers 1, 2 and 3
        version 1.18.0; written and copyright by Michael Hipp and others
        free software (LGPL) without any warranty but with best wishes
    
    Playing MPEG stream 1 of 1: bug201_wrong_frame_buffer.mp3 ...
    
    MPEG 1.0 layer I, 160 kbit/s, 44100 Hz stereo
    Title:   Sail For The River
    Artist:  ÁÖÂ¡è¯ / Nat King Cole / Judy Garland / ÉñβÏÜÒ» / ¸ÇÂå¿É˹»ù / °Â˹²®É­ /     °¬Ë¹Ìؼª°ØÍÐ / ³ÂÈð±ó / ÜïµÙ¼ÎÂ× / ÄǾ©¸ß / Rueibin Chen
    Comment: 3102232_4717681021106_1         Album:  ³¬ÍêÃÀ¸æ°×/Perfect Unbosom
    Year:    2008                            Genre:  Pop
    [layer1.c:30] error: Illegal bit allocation value.
    [layer1.c:172] error: Aborting layer I decoding after step one.
    
    [layer1.c:30] error: Illegal bit allocation value.
    [layer1.c:172] error: Aborting layer I decoding after step one.
    
    Warning: Big change (MPEG version, layer, rate). Frankenstein stream?
    Note: Illegal Audio-MPEG-Header 0x00000000 at offset 4455.
    Note: Trying to resync...
    Note: Skipped 844 bytes in input.
    

    I realized that just adding CRC check would not catch it. There are also some "good" MPEG layer I frames. Adding a check for illegal bit allocation values worked for making them silent, though.

    Note about the seeking: You can always check if your seeks are going to be inconsistent by asking mpg123_spf() for the samples per frame. If that changes during decoding, seeking will suffer. You could combine that information with playback position in a seeking table of your own ... or just ignore that one erroneous file. Really, without malice, such a file should not happen.

     
    • PAN Myautsai

      PAN Myautsai - 2014-01-27

      Thanks for your great effort! I think that is fine with an error output, so long as the program didn't quit unexpectedly, and a mp3 file like that is really rare. BTW, I think the bug 201 should be close now but I cannot find a "close" button.

       
  • Thomas Orgis

    Thomas Orgis - 2014-02-02
    • status: open --> closed-fixed
     

Log in to post a comment.