classification
Title: Coverity complains on audioop
Type: compile error Stage: resolved
Components: Build Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, christian.heimes, python-dev, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2014-01-26 07:35 by serhiy.storchaka, last changed 2015-02-10 08:47 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
audioop_ratecv_coverity_silence.patch serhiy.storchaka, 2014-01-26 07:35 review
Messages (9)
msg209284 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-26 07:35
Christian Heimes reported (http://permalink.gmane.org/gmane.comp.python.devel/145253) Coverity issue:

"""
** CID 1164423:  Division or modulo by zero  (DIVIDE_BY_ZERO)
/Modules/audioop.c: 1375 in audioop_ratecv_impl()


________________________________________________________________________________________________________
*** CID 1164423:  Division or modulo by zero  (DIVIDE_BY_ZERO)
/Modules/audioop.c: 1375 in audioop_ratecv_impl()
1369                without spurious overflow is the challenge; we can
1370                settle for a reasonable upper bound, though, in this
1371                case ceiling(len/inrate) * outrate. */
1372
1373             /* compute ceiling(len/inrate) without overflow */
1374             Py_ssize_t q = len > 0 ? 1 + (len - 1) / inrate : 0;
>>>     CID 1164423:  Division or modulo by zero  (DIVIDE_BY_ZERO)
>>>     In expression "9223372036854775807L / q", division by expression
"q" which may be zero has undefined behavior.
1375             if (outrate > PY_SSIZE_T_MAX / q / bytes_per_frame)
1376                 str = NULL;
1377             else
1378                 str = PyBytes_FromStringAndSize(NULL,
1379                                                 q * outrate *
bytes_per_frame);
1380         }
"""

This is false positive. len should be non-negative and a case of 0 already checked just several lines before.

Is Coverity aware asserts? Perhaps this patch will silence it.
msg209290 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-01-26 08:30
Coverity appears concerned about the division by q. It apparently knows inrate != 0. I do not see any division by len. If q is cleared up, I presume it will check bytes_per_frame.
msg209368 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2014-01-26 23:45
Coverity is concerned about the value of `q` when `len < 0`. The expression

   Py_ssize_t q = len > 0 ? 1 + (len - 1) / inrate : 0;

returns a positive, non-null value for len > 0. Another check ensures that len != 0 a couple of lines earlier. In theory it is possible that len < 0. After all it's a signed integer type.

Coverity tries very hard to guess the intention of code. Because there is a check for len > 0, Coverity thinks that the code has to handle len < 0. IMO a good fix should check len >= 0 very early and replace that line with

   Py_ssize_t q = 1 + (len - 1) / inrate;
msg209371 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-27 00:12
New changeset bf52f2dbfdde by Christian Heimes in branch 'default':
Issue #20394: Attempt to silence CID 1164423: Division or modulo by zero in audioop_ratecv_impl()
http://hg.python.org/cpython/rev/bf52f2dbfdde
msg209405 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-27 07:00
If Coverity is silenced, this patch should be backported to 2.7 and 3.3.
msg209411 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-27 08:07
New changeset 0a406b6fe61f by Christian Heimes in branch 'default':
I forgot to add a Misc/NEWS entry for issue #20394
http://hg.python.org/cpython/rev/0a406b6fe61f
msg209412 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2014-01-27 08:09
The commit has silenced coverity's warning.
msg223249 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-07-16 18:42
Christian, what about 2.7?
msg228517 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-10-05 00:43
Can we have a decision as to whether or not the patch should be back ported to 2.7.
History
Date User Action Args
2015-02-10 08:47:44serhiy.storchakasetstatus: pending -> closed
type: compile error
resolution: fixed
stage: patch review -> resolved
2014-11-11 10:58:30serhiy.storchakasetstatus: open -> pending
2014-10-05 00:43:21BreamoreBoysetstatus: pending -> open
nosy: + BreamoreBoy
messages: + msg228517

2014-07-16 18:42:35serhiy.storchakasetstatus: open -> pending

messages: + msg223249
versions: - Python 3.3, Python 3.4
2014-01-27 08:09:38christian.heimessetmessages: + msg209412
2014-01-27 08:07:53python-devsetmessages: + msg209411
2014-01-27 07:00:34serhiy.storchakasetmessages: + msg209405
2014-01-27 00:12:28python-devsetnosy: + python-dev
messages: + msg209371
2014-01-26 23:45:59christian.heimessetmessages: + msg209368
2014-01-26 08:30:07terry.reedysetnosy: + terry.reedy
messages: + msg209290
2014-01-26 07:35:21serhiy.storchakacreate