malloc: *** error for object: incorrect checksum for freed object - object...
Brought to you by:
sobukus
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
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.
Thanks for your quick reply! The snapshot version runs as expected now. Thank you again for your quick fix!
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
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):
I debugged using valgrind:
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
andtime_limit
). Withoutmpg123_scan
, thestart_frame
may be a wrong value, but if I addmpg123_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
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.
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.
Hi Thomas, where can I download the patches mentioned above or they're still in progress?
Am Mon, 13 Jan 2014 14:20:22 +0000
schrieb "PAN Myautsai" mckelvin@users.sf.net:
The current mpg123 trunk has all of my work so far. This is contained
in the current http://mpg123.org/snapshot (created nightly).
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
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.
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.
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.
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 runpython 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, inexample/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. ifTIME_LIMIT = 0
, it means no max duration limit.TIME_START = -10; TIME_LIMIT=0
andTIME_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?Sorry, I have trouble sparing time for mpg123 alone:-( The seeking test should be easy after all. Perhaps we can summarize the mpg123 usage:
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.
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?
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;-)
Just noticed: mpg123_read() doesn't care for your buffer size. So, ignore my rambling about safe size.
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
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.
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.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:
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.
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.