File mp3gain-1.6.1-libmpg123_fixup.patch of Package mp3gain

Overview Repositories Revisions Requests Users Attributes Meta

File mp3gain-1.6.1-libmpg123_fixup.patch of Package mp3gain (Revision c2f8e32aad8c64b5d3c86df605d62467)

Currently displaying revision c2f8e32aad8c64b5d3c86df605d62467 , Show latest

From: Thomas Orgis <thomas.orgis@gmx.de>
Date: 2018-01-04 13:48:11 +0100

------------------------------------------------------------------------
r2 | thomas | 2018-02-04 10:48:22 +0100 (dom 04 de feb de 2018) | 16 líneas

Clean up the libmpg123 usage, also fixing crash by reproducer for CVE-2017-14407.

This adds checks for return values to the mpg123 API calls, those
were missing in my first dirty patch. Also, the output format
(channel count) and the number of decoded samples are checked
after each decoded frame now, to ensure that we do not deviate from
mp3gain's view. A test for this the reproducer file for CVE-2017-14407,
available at  https://github.com/asarubbo/poc/blob/master/00345-aacgain-stackoverflow-filterYule .

The CVE itself is older than the libmpg123 usage, but the bad file is
nevertheless a nice example of mp3gain and mpg123 parsers deviating:
Mp3gain code things that the channel count changed, while libmpg123
rejected that change from broken frame headers. Buffer overflow results
(observable with CFLAGS=-fsanitize=address LDFLAGS=-lasan).

Gapless decoding is deactivated, figuring that we want the raw samples
from the file without offset games. There can be small changes of
computed gain from that, but usually the padding should be short bits
of silence anyway, nothing too fancy.

Index: mp3gain.c
===================================================================
--- mp3gain.c	(revisión: 1)
+++ mp3gain.c	(revisión: 2)
@@ -2126,9 +2126,19 @@
 #ifdef AACGAIN
             if (!aacH)
 #endif
-    			mh = mpg123_new(NULL, NULL);
-				mpg123_param(mh, MPG123_ADD_FLAGS, MPG123_FORCE_FLOAT, 0.);
-				mpg123_open_feed(mh);
+				/* We want mpg123 decoding to float only, and without fancy sample cutting, */
+				/* so that mp3gain and mpg123 agree on sample offsets. The padding should   */
+				/* have no big effect on gain anyway. */
+				if(
+					!(mh = mpg123_new(NULL, &decodeSuccess)) ||
+					MPG123_OK != mpg123_param(mh, MPG123_ADD_FLAGS, MPG123_FORCE_FLOAT, 0.) ||
+					MPG123_OK != mpg123_param(mh, MPG123_REMOVE_FLAGS, MPG123_GAPLESS, 0.)  ||
+					MPG123_OK != mpg123_open_feed(mh)
+				) {
+					fprintf( stderr, "Failed to create/setup mpg123 handle:\n",
+						mh ? mpg123_strerror(mh) : mpg123_plain_strerror(decodeSuccess) );
+					exit(1);
+				}
 			if (tagInfo[mainloop].recalc == 0) {
 				maxsample = tagInfo[mainloop].trackPeak * 32767.0;
 				maxgain = tagInfo[mainloop].maxGain;
@@ -2319,20 +2329,43 @@
 											unsigned char *decout;
 											/* Get gain values directly, not from decoder. */
 											scanFrameGain();
-											mpg123_feed(mh, curframe, bytesinframe);
+											/* Feed the frames to libmpg123. As they are pre-parsed already, */
+											/* we tolerate no deviations. Calls for more data mean that the  */
+											/* mpg123 parser disagrees with us here, which is fatal.         */
+											/* It could also mean free format streams where you need to seek */
+											/* ahead. But: mp3gain will not work on those anyway. A rework   */
+											/* might just rely on libmpg123's parser and pull the raw data   */
+											/* from there. */
+											if(MPG123_OK != mpg123_feed(mh, curframe, bytesinframe)) {
+												fprintf(stderr, "Feeding mpg123 failed:\n", mpg123_strerror(mh));
+												exit(1);
+											}
+											/* We will check output format for each frame, as there is the */
+											/* case where mp3gain may think the channel count changed, but */
+											/* mpg123 does not (CVE-2017-14407). */
 											decodeSuccess = mpg123_decode_frame(mh, NULL, &decout, &decbytes);
 											if(decodeSuccess == MPG123_NEW_FORMAT)
-											{
+												decodeSuccess = mpg123_decode_frame(mh, NULL, &decout, &decbytes);
+											if(decodeSuccess == MPG123_OK) {
 												int enc = 0, channels = 0;
 												mpg123_getformat(mh, NULL, &channels, &enc);
+												/* Could also check that sampling freq matches ... */
 												if(enc != MPG123_ENC_FLOAT_32 || channels != nchan)
 												{
 													fprintf(stderr, "Unexpected format returned by libmpg123.\n");
 													exit(1);
 												}
-												decodeSuccess = mpg123_decode_frame(mh, NULL, &decout, &decbytes);
+											} else {
+												fprintf(stderr, "Failed to decode MPEG frame: %s\n", mpg123_strerror(mh));
+												exit(1);
 											}
 											nprocsamp = decbytes / sizeof(float) / nchan;
+											/* Paranoia triggered by CVE-2017-14407. */
+											if(nprocsamp > sizeof(lsamples)/sizeof(Float_t))
+											{
+												fprintf(stderr, "Too many samples in libmpg123 output.\n");
+												exit(1);
+											}
 											convert_decout((float*)decout, nprocsamp, nchan, lsamples, rsamples);
 											maxsample = find_maxsample((float*)decout, nprocsamp*nchan, maxsample);
 #ifdef WIN32

------------------------------------------------------------------------
r3 | thomas | 2018-02-04 10:52:33 +0100 (dom 04 de feb de 2018) | 1 línea

Add missing %s to libmpg123 error printouts.

Index: mp3gain.c
===================================================================
--- mp3gain.c	(revisión: 2)
+++ mp3gain.c	(revisión: 3)
@@ -2135,7 +2135,7 @@
 					MPG123_OK != mpg123_param(mh, MPG123_REMOVE_FLAGS, MPG123_GAPLESS, 0.)  ||
 					MPG123_OK != mpg123_open_feed(mh)
 				) {
-					fprintf( stderr, "Failed to create/setup mpg123 handle:\n",
+					fprintf( stderr, "Failed to create/setup mpg123 handle: %s\n",
 						mh ? mpg123_strerror(mh) : mpg123_plain_strerror(decodeSuccess) );
 					exit(1);
 				}
@@ -2337,7 +2337,7 @@
 											/* might just rely on libmpg123's parser and pull the raw data   */
 											/* from there. */
 											if(MPG123_OK != mpg123_feed(mh, curframe, bytesinframe)) {
-												fprintf(stderr, "Feeding mpg123 failed:\n", mpg123_strerror(mh));
+												fprintf(stderr, "Feeding mpg123 failed: %s\n", mpg123_strerror(mh));
 												exit(1);
 											}
 											/* We will check output format for each frame, as there is the */

------------------------------------------------------------------------
r7 | thomas | 2018-02-04 13:25:16 +0100 (dom 04 de feb de 2018) | 14 líneas

Fix libmpg123 usage with respect to MPG123_NEED_MORE.

Current libmpg123 always wants to check if there is a valid
header after the first frame and thus always returns MPG123_NEED_MORE
after feeding one and only one frame of MPEG data. Since
API version 45 (mpg123 1.26.0, to be released), this can be avoided.

This change makes mp3gain work both with new and old libmpg123, only
giving a one-line note about the fact with the latter. The computation
will miss the last frame with old libmpg123, but the effect for the
overall gain is small, if measurable at all. The disabling of gapless
decoding helps a bit.

Index: mp3gain.c
===================================================================
--- mp3gain.c	(revisión: 6)
+++ mp3gain.c	(revisión: 7)
@@ -2129,11 +2129,16 @@
 #endif
 				/* We want mpg123 decoding to float only, and without fancy sample cutting, */
 				/* so that mp3gain and mpg123 agree on sample offsets. The padding should   */
-				/* have no big effect on gain anyway. */
+				/* have no big effect on gain anyway. Seek buffer needs to be disabled to   */
+				/* avoid MPG123_NEED_MORE after feeding the first frame.                    */
 				if(
 					!(mh = mpg123_new(NULL, &decodeSuccess)) ||
 					MPG123_OK != mpg123_param(mh, MPG123_ADD_FLAGS, MPG123_FORCE_FLOAT, 0.) ||
-					MPG123_OK != mpg123_param(mh, MPG123_REMOVE_FLAGS, MPG123_GAPLESS, 0.)  ||
+					MPG123_OK != mpg123_param(mh, MPG123_REMOVE_FLAGS, MPG123_GAPLESS|MPG123_SEEKBUFFER, 0.)  ||
+					MPG123_OK != mpg123_param(mh, MPG123_VERBOSE, 10, 0.) ||
+#if (MPG123_API_VERSION >= 45)
+					MPG123_OK != mpg123_param(mh, MPG123_ADD_FLAGS, MPG123_NO_READAHEAD, 0.) ||
+#endif
 					MPG123_OK != mpg123_open_feed(mh)
 				) {
 					fprintf( stderr, "Failed to create/setup mpg123 handle: %s\n",
@@ -2357,8 +2362,18 @@
 													exit(1);
 												}
 											} else {
+#if (MPG123_API_VERSION < 45)
+												if(decodeSuccess == MPG123_NEED_MORE)
+												{
+													fprintf(stderr, "Delaying a frame in decoding with old libmpg123.\n");
+													decbytes = 0;
+												} else {
+#endif
 												fprintf(stderr, "Failed to decode MPEG frame: %s\n", mpg123_strerror(mh));
 												exit(1);
+#if (MPG123_API_VERSION < 45)
+												}
+#endif
 											}
 											nprocsamp = decbytes / sizeof(float) / nchan;
 											/* Paranoia triggered by CVE-2017-14407. */

------------------------------------------------------------------------

Places

File mp3gain-1.6.1-libmpg123_fixup.patch of Package mp3gain (Revision c2f8e32aad8c64b5d3c86df605d62467)

Places