This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Undefined behavior in dtoa.c (rshift 32 of 32bit data type)
Type: behavior Stage: needs patch
Components: Versions: Python 3.3, Python 3.4, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, doko, eric.smith, mark.dickinson, serhiy.storchaka
Priority: normal Keywords:

Created on 2015-04-18 22:52 by christian.heimes, last changed 2022-04-11 14:58 by admin.

Messages (7)
msg241464 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2015-04-18 22:52
Coverity has found undefined behavior in dtoa.c:d2b(). lo0bits() can return 32 which z >>= 32, where z is an uint32. I've talked to doku at PyCon. He suggested to update dtoa.c to a more recent version. Our copy is based on a version from 2001. There are more modern versions available, e.g. https://searchcode.com/codesearch/view/52748288/ from 2006.

CID 1202735 (#1 of 1): Bad bit shift operation (BAD_SHIFT)
large_shift: In expression z >>= k, right shifting by more than 31 bits has undefined behavior. The shift amount, k, is 32.
msg241492 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-04-19 08:13
I'm pretty sure that our code was based on something rather more recent than 2001: it was the most recent version available at the time (around 2008?), and it incorporates subsequent fixes from David Gay.

Please don't replace our dtoa.c with a current version: ours has diverged from the original, and includes fixes that aren't available upstream.
msg241495 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-04-19 08:21
Looking more closely, the report doesn't make sense to me:  `k` is the return value from a call to `lo0bits`.  From the source of `lo0bits`, I don't see any way that `k` can be 32: it's always going to be in the range [0, 31].  Christian: do you have any more information from Coverity?  This looks like a false positive to me.
msg241496 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-04-19 08:25
Ah, sorry; I see it.  Fix on the way.
msg241497 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-04-19 08:41
Okay, so after looking more closely, this *still* looks like a false positive:  `lo0bits` *can* return 32, but only for an input of zero.  In the code in question, we're doing `k = lo0bits(&y)`, so the only way we can get a `k` of `32` is if `y = 0`.  But the whole thing is inside an "if" block that looks like `if ((y = word1(d))) { ... }` (yep, completely with the extra parentheses and the misleading equality-test-lookalike assignment), so that `if` block won't be executed if `y` is zero.

I edited the code to print out debugging information if `k` is ever 32 at that point, and saw no output.  So I don't think that line ever gets executed with `k = 32`.
msg241498 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-04-19 08:42
> saw no output

Bah; missed a bit.  I saw no output when running the Python test suite, that is.  That's not definitive, of course.
msg241515 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2015-04-19 13:03
You could be right. I didn't track all paths manually. All this bit shifting is making my head dizzy... :)

Anyways I have sent you an invite for Coverity, so you can check the result yourself. The Python test suite passes with assert(k < 32); inside the problematic block, too.
History
Date User Action Args
2022-04-11 14:58:15adminsetgithub: 68187
2015-04-19 13:03:53christian.heimessetmessages: + msg241515
2015-04-19 08:42:06mark.dickinsonsetmessages: + msg241498
2015-04-19 08:41:32mark.dickinsonsetmessages: + msg241497
2015-04-19 08:25:57mark.dickinsonsetmessages: + msg241496
2015-04-19 08:21:18mark.dickinsonsetmessages: + msg241495
2015-04-19 08:13:00mark.dickinsonsetmessages: + msg241492
2015-04-18 22:52:13christian.heimescreate