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.

5. max(b'\x00\x00\x00\x80', 4) returns 0 (on little-endian).

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.

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.

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.

Can anyone look at the patch? I want fix this issue before 2.7.4 RC released.

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.

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)?

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).

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

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.

> *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.

stage: patch review -> resolved

messages: + msg177935