[PATCH] frontend: handle mpg123 length and getstate errors in decode
"Maya R. Odinezenko via Lame-dev" <[email protected]> Fri, 17 Jul 2026 15:18:00 -0400
| Newsgroups | gmane.comp.audio.mp3.lame |
|---|---|
| Message-ID | <CAFCgL3n_kRiULFacq9hMLvoERZc7pGxXb4R+48kfqdT3JqwqpA@mail.gmail.com> |
Hello, In the libmpg123 decode path, lame123_decode_initfile() (frontend/get_audio.c) does not check the error returns from mpg123_framelength() and mpg123_length(). Both return MPG123_ERR (-1) when no length is known, but the guards (len <= (unsigned long)-1 and len <= UINT_MAX/2) admit that -1 before it is stored into the destination fields, and the first cannot reject anything at all where unsigned long is 64-bit. With a piped, Info-tag-less MP3 I saw nsamp become 2^64-1, which also masks the MAX_U_32_NUM "unknown" sentinel and disables the file-size fallback. The two mpg123_getstate() calls just below are likewise unchecked and read val uninitialized if the query fails. The patch stores 0 (unknown) for an unusable frame count, MAX_U_32_NUM (unknown) for a failed length query so the file-size fallback re-engages, keeps rejecting counts above INT_MAX as before, and on a failed getstate() uses the documented "unknown" value (-1) instead of reading val uninitialized. Tested on Apple Silicon (LP64): the piped decode now stores the sentinel instead of 2^64-1; seekable and LAME-tagged inputs are unchanged (delay/padding read identically); a forged oversized-Xing file is still rejected; decoded and encoded outputs are byte-identical. The patch is against current trunk (r6597). Maya -- >8 -- From: Maya <[email protected]> Date: Fri, 17 Jul 2026 14:30:00 -0400 Subject: [PATCH] frontend: handle mpg123 length and getstate errors in decode init mpg123_framelength() and mpg123_length() return MPG123_ERR (-1) when no length guess is possible, but both guards admitted negative values before conversion into the destination fields, and the framelength guard (len <= (unsigned long)-1) cannot reject anything where unsigned long is 64-bit. Reproduced with a piped, Info-tag-less MP3: nsamp became 2^64-1, which also masked the MAX_U_32_NUM sentinel and disabled the file-size fallback. Store 0 (unknown) for unusable frame counts, MAX_U_32_NUM (unknown) for failed length queries so the file-size fallback re-engages, and keep rejecting sample counts above INT_MAX as before. When mpg123_getstate() does not return MPG123_OK, use the documented "unknown" value (-1) instead of reading val uninitialized. Tested on Apple Silicon (LP64): the piped decode now stores the sentinel instead of 2^64-1; seekable and LAME-tagged inputs are unchanged (delay/padding 576/1216 read identically); a forged oversized-Xing file is still rejected; decoded and encoded outputs are byte-identical. --- frontend/get_audio.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/frontend/get_audio.c b/frontend/get_audio.c index 2898cb3..f492efe 100644 --- a/frontend/get_audio.c +++ b/frontend/get_audio.c @@ -2171,22 +2171,30 @@ int lame123_decode_initfile(FILE *fd, mp3data_struct *mp3data, int *enc_delay, i /* Guessing seems to be OK, so we do not have to insist on knowing if libmpg123 got that info from Info tag or not. */ /* I am paranoid about off_t being larger than long or int. */ + /* mpg123_framelength()/mpg123_length() return MPG123_ERR (-1) when no + length is known; the old guards (len <= (unsigned long)-1 and + len <= UINT_MAX/2) let that -1 through into the destination fields, + and the first cannot reject anything where unsigned long is 64-bit */ len = mpg123_framelength(global.hip->mh); - if(len <= (unsigned long)-1) - mp3data->totalframes = len; + if(len >= 0 && len <= INT_MAX) + mp3data->totalframes = (int)len; else - return -1; + mp3data->totalframes = 0; /* unknown; the mpglib path uses 0 here too */ len = mpg123_length(global.hip->mh); - if(len <= ((unsigned int)-1)/2) + if(len >= 0 && len <= ((unsigned int)-1)/2) mp3data->nsamp = len; + else if(len < 0) + mp3data->nsamp = MAX_U_32_NUM; /* unknown: let the file-size fallback guess */ else - return -1; + return -1; /* implausibly long: keep rejecting, as before */ /* Encoder delay and padding are not needed when libmpg123 handles gapless decoding itself. So let's see if we get away with that. */ - mpg123_getstate(global.hip->mh, MPG123_ENC_DELAY, &val, NULL); - *enc_delay = val; - mpg123_getstate(global.hip->mh, MPG123_ENC_PADDING, &val, NULL); - *enc_padding = val; + if(MPG123_OK != mpg123_getstate(global.hip->mh, MPG123_ENC_DELAY, &val, NULL)) + val = -1; /* documented "unknown" value */ + *enc_delay = (int)val; + if(MPG123_OK != mpg123_getstate(global.hip->mh, MPG123_ENC_PADDING, &val, NULL)) + val = -1; + *enc_padding = (int)val; if(global.in_id3v2_tag) free(global.in_id3v2_tag); global.in_id3v2_size = 0;