classification
Title: mmap.read() crashes when passed a negative argument
Type: crash Stage:
Components: Library (Lib) Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, ocean-city
Priority: high Keywords: patch

Created on 2009-06-25 21:22 by amaury.forgeotdarc, last changed 2009-06-29 15:10 by ocean-city. This issue is now closed.

Files
File name Uploaded Description Edit
fix_mmap_read.patch ocean-city, 2009-06-27 21:59
fix_mmap_read.patch ocean-city, 2009-06-29 11:04
Messages (8)
msg89714 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-06-25 21:22
mmap.read() crashes when passed a negative count:

    def test_read_negative(self):
        f = open(TESTFN, 'w+')
        f.write("ABCDE\0abcde")
        f.flush()

        mf = mmap.mmap(f.fileno(), 0)

        self.assertEqual(mf.read(2),  "AB")    # OK
        self.assertEqual(mf.read(-1), "CDE")   # ??
        self.assertEqual(mf.read(-1), "BCDE")  # ??
        self.assertEqual(mf.read(-1), "ABCDE") # ??
        mf.read(-1)                            # crash!!

I don't know what mf.read(-1) should do: raise a ValueError, return the empty 
string, or return everything from the current pos to len(mf)?
msg89717 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-06-26 01:00
> I don't know what mf.read(-1) should do
I'm not sure neither.

I think the problem is that mmap uses size_t as length, but uses
Py_ssize_t for PyArg_ParseTuple. (PyArg_ParseTuple doesn't support
size_t) I think this discrepancy should be fixed.

If mmap should use size_t because it should cover all possible memory
region which size_t can represent but Py_ssize_t cannot, maybe should
PyArg_ParseTuple support size_t?
msg89724 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-06-26 08:51
Well, I would not care that much: you can never read more than
PY_SSIZE_T_MAX, because the mapped area and the string would not fit in
the addressable space of the process.
msg89757 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-06-27 21:59
Hmm, I cannot reproduce the crash. I created the patch experimentally,
but I'm not confident with this patch. Especially here

+	if (n < 0)
+		n = PY_SSIZE_T_MAX;

because I don't have so much memory.
msg89771 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-06-28 09:47
> +	if (n < 0)

I suggest a comment like:

/* The difference can overflow, only if self->size is greater than
 * PY_SSIZE_T_MAX.  But then the operation cannot possibly succeed,
 * because the mapped area and the returned string each need more 
 * than half of the addressable memory.  So we clip the size, and let
 * the code below raise MemoryError.
 */
msg89824 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-06-29 11:04
OK, how about this patch?
msg89830 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-06-29 12:04
OK for me.
msg89849 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-06-29 15:10
Thanks, committed in r73677(trunk), r73682(release26-maint),
r73684(py3k), r73685(release31-maint).
History
Date User Action Args
2009-06-29 15:10:28ocean-citysetstatus: open -> closed
resolution: fixed
messages: + msg89849
2009-06-29 12:04:14amaury.forgeotdarcsetmessages: + msg89830
2009-06-29 11:04:24ocean-citysetfiles: + fix_mmap_read.patch

messages: + msg89824
2009-06-28 09:47:53amaury.forgeotdarcsetmessages: + msg89771
2009-06-27 21:59:05ocean-citysetfiles: + fix_mmap_read.patch
keywords: + patch
messages: + msg89757
2009-06-26 08:51:38amaury.forgeotdarcsetmessages: + msg89724
2009-06-26 01:00:40ocean-citysetmessages: + msg89717
2009-06-25 21:22:27amaury.forgeotdarccreate