From 13b585037cda30e450facd5e292dd04e4738e78c Mon Sep 17 00:00:00 2001 From: Matthias Goergens Date: Mon, 19 Sep 2022 11:26:41 +0800 Subject: [PATCH 1/4] gh-96821: Fix undefined behaviour in `audioop.c` Left-shifting negative numbers is undefined behaviour. Fortunately, multiplication works just as well, is defined behaviour, and gets compiled to the same machine code as before by optimizing compilers. --- Modules/audioop.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/Modules/audioop.c b/Modules/audioop.c index d74e634ac44889..8ea2cbbc34ffbf 100644 --- a/Modules/audioop.c +++ b/Modules/audioop.c @@ -65,6 +65,8 @@ search(int16_t val, const int16_t *table, int size) if (val <= *table++) return (i); } + assert(0 <= size); + assert(size < INT16_MAX); return (size); } #define st_ulaw2linear16(uc) (_st_ulaw2linear16[uc]) @@ -170,7 +172,7 @@ st_14linear2ulaw(int16_t pcm_val) /* 2's complement (14-bit range) */ if (seg >= 8) /* out of range, return maximum value. */ return (unsigned char) (0x7F ^ mask); else { - uval = (unsigned char) (seg << 4) | ((pcm_val >> (seg + 1)) & 0xF); + uval = (unsigned char) (seg * (1 << 4)) | ((pcm_val >> (seg + 1)) & 0xF); return (uval ^ mask); } @@ -259,7 +261,7 @@ st_linear2alaw(int16_t pcm_val) /* 2's complement (13-bit range) */ if (seg >= 8) /* out of range, return maximum value. */ return (unsigned char) (0x7F ^ mask); else { - aval = (unsigned char) seg << SEG_SHIFT; + aval = (unsigned char) seg * (1 << SEG_SHIFT); if (seg < 2) aval |= (pcm_val >> 1) & QUANT_MASK; else @@ -300,13 +302,13 @@ static const int stepsizeTable[89] = { #ifdef WORDS_BIGENDIAN #define GETINT24(cp, i) ( \ ((unsigned char *)(cp) + (i))[2] + \ - (((unsigned char *)(cp) + (i))[1] << 8) + \ - (((signed char *)(cp) + (i))[0] << 16) ) + (((unsigned char *)(cp) + (i))[1] * (1 << 8)) + \ + (((signed char *)(cp) + (i))[0] * (1 << 16)) ) #else #define GETINT24(cp, i) ( \ ((unsigned char *)(cp) + (i))[0] + \ - (((unsigned char *)(cp) + (i))[1] << 8) + \ - (((signed char *)(cp) + (i))[2] << 16) ) + (((unsigned char *)(cp) + (i))[1] * (1 << 8)) + \ + (((signed char *)(cp) + (i))[2] * (1 << 16)) ) #endif @@ -347,10 +349,10 @@ static const int stepsizeTable[89] = { } while(0) -#define GETSAMPLE32(size, cp, i) ( \ - (size == 1) ? (int)GETINT8((cp), (i)) << 24 : \ - (size == 2) ? (int)GETINT16((cp), (i)) << 16 : \ - (size == 3) ? (int)GETINT24((cp), (i)) << 8 : \ +#define GETSAMPLE32(size, cp, i) ( \ + (size == 1) ? (int)GETINT8((cp), (i)) * (1 << 24) : \ + (size == 2) ? (int)GETINT16((cp), (i)) * (1 << 16) : \ + (size == 3) ? (int)GETINT24((cp), (i)) * (1 << 8) : \ (int)GETINT32((cp), (i))) #define SETSAMPLE32(size, cp, i, val) do { \ @@ -1558,7 +1560,7 @@ audioop_ulaw2lin_impl(PyObject *module, Py_buffer *fragment, int width) cp = fragment->buf; for (i = 0; i < fragment->len*width; i += width) { - int val = st_ulaw2linear16(*cp++) << 16; + int val = st_ulaw2linear16(*cp++) * (1 << 16); SETSAMPLE32(width, ncp, i, val); } return rv; @@ -1632,7 +1634,7 @@ audioop_alaw2lin_impl(PyObject *module, Py_buffer *fragment, int width) cp = fragment->buf; for (i = 0; i < fragment->len*width; i += width) { - val = st_alaw2linear16(*cp++) << 16; + val = st_alaw2linear16(*cp++) * (1 << 16); SETSAMPLE32(width, ncp, i, val); } return rv; @@ -1757,7 +1759,7 @@ audioop_lin2adpcm_impl(PyObject *module, Py_buffer *fragment, int width, /* Step 6 - Output value */ if ( bufferstep ) { - outputbuffer = (delta << 4) & 0xf0; + outputbuffer = (delta * (1 << 4)) & 0xf0; } else { *ncp++ = (delta & 0x0f) | outputbuffer; } @@ -1875,7 +1877,7 @@ audioop_adpcm2lin_impl(PyObject *module, Py_buffer *fragment, int width, step = stepsizeTable[index]; /* Step 6 - Output value */ - SETSAMPLE32(width, ncp, i, valpred << 16); + SETSAMPLE32(width, ncp, i, valpred * (1 << 16)); } rv = Py_BuildValue("(O(ii))", str, valpred, index); From 07d709a5d526d09e81ecb8aaa3a12cd9211847bd Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Mon, 19 Sep 2022 03:35:02 +0000 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2022-09-19-03-35-01.gh-issue-96821.izK6JA.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-09-19-03-35-01.gh-issue-96821.izK6JA.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-09-19-03-35-01.gh-issue-96821.izK6JA.rst b/Misc/NEWS.d/next/Core and Builtins/2022-09-19-03-35-01.gh-issue-96821.izK6JA.rst new file mode 100644 index 00000000000000..73d0c76f0297ca --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-09-19-03-35-01.gh-issue-96821.izK6JA.rst @@ -0,0 +1 @@ +Fix undefined behaviour in ``audioop.c``. From 15e43a6288d74022b2332697aa6575c7a83f1c4d Mon Sep 17 00:00:00 2001 From: Matthias Goergens Date: Wed, 5 Oct 2022 09:20:57 +0800 Subject: [PATCH 3/4] Move asserts for more coverage --- Modules/audioop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/audioop.c b/Modules/audioop.c index 8ea2cbbc34ffbf..e7429eeb70c1d7 100644 --- a/Modules/audioop.c +++ b/Modules/audioop.c @@ -59,14 +59,14 @@ static const int16_t seg_uend[8] = { static int16_t search(int16_t val, const int16_t *table, int size) { + assert(0 <= size); + assert(size < INT16_MAX); int i; for (i = 0; i < size; i++) { if (val <= *table++) return (i); } - assert(0 <= size); - assert(size < INT16_MAX); return (size); } #define st_ulaw2linear16(uc) (_st_ulaw2linear16[uc]) From ba806114d9810571f9aad81d2e3094e9f445e9c6 Mon Sep 17 00:00:00 2001 From: Matthias Goergens Date: Sun, 9 Oct 2022 20:33:38 +0800 Subject: [PATCH 4/4] Revert unneeded changes --- Modules/audioop.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Modules/audioop.c b/Modules/audioop.c index e7429eeb70c1d7..c29a3e8df4099d 100644 --- a/Modules/audioop.c +++ b/Modules/audioop.c @@ -172,7 +172,8 @@ st_14linear2ulaw(int16_t pcm_val) /* 2's complement (14-bit range) */ if (seg >= 8) /* out of range, return maximum value. */ return (unsigned char) (0x7F ^ mask); else { - uval = (unsigned char) (seg * (1 << 4)) | ((pcm_val >> (seg + 1)) & 0xF); + assert(seg >= 0); + uval = (unsigned char) (seg << 4) | ((pcm_val >> (seg + 1)) & 0xF); return (uval ^ mask); } @@ -261,7 +262,7 @@ st_linear2alaw(int16_t pcm_val) /* 2's complement (13-bit range) */ if (seg >= 8) /* out of range, return maximum value. */ return (unsigned char) (0x7F ^ mask); else { - aval = (unsigned char) seg * (1 << SEG_SHIFT); + aval = (unsigned char) seg << SEG_SHIFT; if (seg < 2) aval |= (pcm_val >> 1) & QUANT_MASK; else