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: Unsigned type in mmap_move_method
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, ZackerySpytz, amaury.forgeotdarc, neologix, rmib, zanella
Priority: normal Keywords: easy, patch

Created on 2011-03-27 21:13 by rmib, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
mmap.move.patch zanella, 2011-04-24 13:35 bounds check modified, var types still wrong review
Messages (5)
msg132364 - (view) Author: rmib (rmib) Date: 2011-03-27 21:13
In mmapmodule.c a function mmap_move_method, use unsigned variables dest, src, cnt, as signed:
unsigned long dest, src, cnt;
...
if (cnt <0 | | (cnt + dest) <cnt | | (cnt + src) <cnt | |
            src <0 | | src> self-> size | | (src + cnt)> self-> size | |
            dest <0 | | dest> self-> size | | (dest + cnt)> self-> size)
msg132417 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-03-28 20:17
Did you see a compiler warning? Indeed, this function (mmap.move) decodes its arguments with:
   PyArg_ParseTuple(args, "kkk:move", &dest, &src, &cnt)
This looks wrong to me: these three numbers should be Py_ssize_t, and decoded with "nnn".

An example of wrong behavior (on win32):
>>> m = mmap.mmap(-1, 500)
>>> m.move(2**32, 10, 4)   # Should throw a ValueError
msg134331 - (view) Author: Rafael Zanella (zanella) Date: 2011-04-24 13:35
Seems like it should use size_t since it deals with memory location/obj size, but Python doesn't have size_t only ssize_t, and ssize_t is signed...

"m.move(2**32, 10, 4)   # Should throw a ValueError" <- Won't it wrap around and become 0 once truncated ?

I've attached a patch that I believe fixes the bounds check, though it's still wrong on the types, since it's not portable.
msg221715 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-27 20:00
The if conditional referenced in msg132364 was changed in r79606 but dest, src and cnt are still unsigned long and the call to PyArg_ParseTuple is unchanged.
msg320078 - (view) Author: Zackery Spytz (ZackerySpytz) * (Python triager) Date: 2018-06-20 14:13
This was fixed in commit cd04db03debaead0abd1bff149389445284f88e2 (the commit didn't have an associated BPO issue).
History
Date User Action Args
2022-04-11 14:57:15adminsetgithub: 55906
2018-06-20 14:27:29berker.peksagsetstatus: open -> closed
resolution: out of date
stage: needs patch -> resolved
2018-06-20 14:13:27ZackerySpytzsetnosy: + ZackerySpytz
messages: + msg320078
2014-06-27 21:07:08ned.deilysetnosy: + neologix
2014-06-27 20:00:04BreamoreBoysetnosy: + BreamoreBoy
messages: + msg221715
2011-04-24 13:35:19zanellasetfiles: + mmap.move.patch

nosy: + zanella
messages: + msg134331

keywords: + patch
2011-03-28 20:17:58amaury.forgeotdarcsetkeywords: + easy
2011-03-28 20:17:36amaury.forgeotdarcsetnosy: + amaury.forgeotdarc

messages: + msg132417
stage: needs patch
2011-03-27 21:13:16rmibcreate