classification
Title: audioop overflow issues
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.4, Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, jcea, mark.dickinson, python-dev, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2012-12-14 18:15 by serhiy.storchaka, last changed 2013-02-09 09:35 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
audioop_2.patch serhiy.storchaka, 2012-12-22 10:54 review
audioop_2.py serhiy.storchaka, 2012-12-22 10:56 A sample Python implementation
Messages (12)
msg177482 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-14 18:15
The audioop module has some issues with an overflow.

1. It uses post-checks for an integer overflow. This means using an undefined behavior.

2. When the result truncated in case of overflow, -maxval used as minimal value. But real minimum value is less (-maxval - 1). This means not using full possible range and causes an odd result of some operations (for example add(b'\x80', '\x00', 1) returns b'\x81').

3. Some operations (for example bias()) does not truncating and just overflow.

4. lin2lin() conversion from 4 to 4 (should do nothing) loses 16 lowest bits.
msg177531 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-15 08:19
5. max(b'\x00\x00\x00\x80', 4) returns 0 (on little-endian).
msg177771 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-19 18:51
6. reverse() and ratecv() lose 16 lowest bits for 4-bytes data.

7. rms() can returns negative value (-0x80000000 instead 0x80000000).

8. maxpp() and avgpp() overflow and return absolutely wrong result for large peaks.

9. ratecv() crashes Python on empty input.
msg177772 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-19 19:24
Because audioop module is such buggy, I propose a large patch which
fixes mentioned above bugs (and some other). Tests have greatly
extended. There is also a reference Python implementation (later I will
open a separated issue for this).

There are also other, algorithmic bugs. I will submit patches for them
later, after fixing C implementation bugs.
msg177935 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-22 10:54
I found that the documentation contains a receipt which depends on the fact that bias() wraps around samples. Here is an updated patch. Also some docs changes included.
msg180564 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-25 10:31
Can anyone look at the patch? I want fix this issue before 2.7.4 RC released.
msg181192 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-02 18:28
Since there is no one who want to review the patch for this dirty buggy module, I will test and review it myself yet once and then commit.
msg181226 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-02-02 22:58
I do not have the knowledge needed to review the code, but I took a brief look. The three doc patches need a verb to be proper English. "Samples truncated in case of overflow." should be "Samples are truncated in case of overflow." in both places. I think "Samples wrapped around in case of overflow." could be rewritten as "Samples wrap around in case of overflow.", though adding 'are' works too.

In the C code comment:

     /* Passing a short** for an 's' argument is correct only
        if the string contents is aligned for interpretation
        as short[]. Due to the definition of PyBytesObject,
-       this is currently (Python 2.6) the case. */
+       this is currently (Python 2.6) the case.
+       XXX: It's not true for memoryview. */

Is whatever the case in 2.6 only (if so, drop obsolete) or since 2.6. If the latter, I would rewrite as "this is the case since Python 2.6.".
The addition is not clear to me. Are you implying that something should be made true for memory view (in a future patch)?
msg181263 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-03 11:54
Thank you Terry.

> Is whatever the case in 2.6 only (if so, drop obsolete) or since 2.6. If the latter, I would rewrite as "this is the case since Python 2.6.".
> The addition is not clear to me. Are you implying that something should be made true for memory view (in a future patch)?

In 2.6 the sentence possible was true and the code was correct. But it is wrong for currently supported versions when we pass memoryview as an argument. This is a bug (possible even a crash) and should be fixed. But on x86 or when we don't use unaligned memoryview (most peoples don't do this) it is never occurred. Due to this I left this bug unfixed yet. There are other minor bugs which I left for different issues. This patch fixes most critical bugs and creates a solid basis for testing.

I see unstable AIX buildbots failed on test_aifc. Perhaps this patch will fix it or expose an issue in audioop module (current audioop testing too poor).
msg181723 - (view) Author: Roundup Robot (python-dev) Date: 2013-02-09 09:17
New changeset 6add6ac6a802 by Serhiy Storchaka in branch '2.7':
Issue #16686: Fixed a lot of bugs in audioop module.
http://hg.python.org/cpython/rev/6add6ac6a802

New changeset 104b17f8316b by Serhiy Storchaka in branch '3.2':
Issue #16686: Fixed a lot of bugs in audioop module.
http://hg.python.org/cpython/rev/104b17f8316b

New changeset 63b164708e60 by Serhiy Storchaka in branch '3.3':
Issue #16686: Fixed a lot of bugs in audioop module.
http://hg.python.org/cpython/rev/63b164708e60

New changeset 48747ef5f65b by Serhiy Storchaka in branch 'default':
Issue #16686: Fixed a lot of bugs in audioop module.
http://hg.python.org/cpython/rev/48747ef5f65b
msg181724 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-09 09:25
I fixed yet one bug in avgpp() and remove my XXX comment. *All* audioop functions are unsafe regarding unaligned access. I'll open a new issue for this.
msg181725 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-09 09:35
> *All* audioop functions are unsafe regarding unaligned access.

Actually this is not true because currently audioop functions work only with bytes (and str, see issue16685) and not with arbitrary memoryview.
History
Date User Action Args
2013-02-09 09:35:31serhiy.storchakasetmessages: + msg181725
2013-02-09 09:25:26serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg181724

stage: patch review -> resolved
2013-02-09 09:17:44python-devsetnosy: + python-dev
messages: + msg181723
2013-02-03 11:54:20serhiy.storchakasetmessages: + msg181263
2013-02-02 22:58:46terry.reedysetmessages: + msg181226
2013-02-02 18:28:24serhiy.storchakasetmessages: + msg181192
2013-01-25 10:31:12serhiy.storchakasetmessages: + msg180564
2012-12-29 21:53:35serhiy.storchakasetassignee: serhiy.storchaka
2012-12-22 19:34:29Arfreversetnosy: + Arfrever
2012-12-22 10:58:16serhiy.storchakasetstage: needs patch -> patch review
2012-12-22 10:57:50serhiy.storchakasetfiles: - audioop.py
2012-12-22 10:57:34serhiy.storchakasetfiles: - audioop_tests.patch
2012-12-22 10:57:18serhiy.storchakasetfiles: - audioop.patch
2012-12-22 10:56:38serhiy.storchakasetfiles: + audioop_2.py
2012-12-22 10:54:05serhiy.storchakasetfiles: + audioop_2.patch

messages: + msg177935
2012-12-22 00:43:07terry.reedysetnosy: + terry.reedy
2012-12-19 19:24:51serhiy.storchakasetfiles: + audioop.patch, audioop_tests.patch, audioop.py
keywords: + patch
messages: + msg177772
2012-12-19 18:51:55serhiy.storchakasettype: behavior -> crash
messages: + msg177771
2012-12-15 08:19:26serhiy.storchakasetmessages: + msg177531
2012-12-14 18:31:05jceasetnosy: + jcea
2012-12-14 18:15:07serhiy.storchakacreate