From 4013627670e5ecd35bb5830aa3bf1600491255fa Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Wed, 24 Apr 2024 13:00:41 +0800 Subject: [PATCH 01/35] add structured exception handling to mmap_read_method --- Modules/mmapmodule.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 0cce7c27f9b16a..78eeae6a420647 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -291,6 +291,17 @@ mmap_read_line_method(mmap_object *self, return result; } +#if defined(MS_WIN32) && !defined(DONT_USE_SEH) +static DWORD HandlePageException(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) +{ + *record = *ptrs->ExceptionRecord; + if (ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { + return EXCEPTION_EXECUTE_HANDLER; + } + return EXCEPTION_CONTINUE_SEARCH; +} +#endif + static PyObject * mmap_read_method(mmap_object *self, PyObject *args) @@ -307,8 +318,23 @@ mmap_read_method(mmap_object *self, remaining = (self->pos < self->size) ? self->size - self->pos : 0; if (num_bytes < 0 || num_bytes > remaining) num_bytes = remaining; + + #if defined(MS_WIN32) && !defined(DONT_USE_SEH) + EXCEPTION_RECORD record; + __try { + result = PyBytes_FromStringAndSize(&self->data[self->pos], num_bytes); + self->pos += num_bytes; + } + __except (HandlePageException(GetExceptionInformation(), &record)) { + NTSTATUS code = record.ExceptionInformation[2]; + PyErr_SetFromWindowsErr(code); + result = NULL; + } + #else result = PyBytes_FromStringAndSize(&self->data[self->pos], num_bytes); self->pos += num_bytes; + #endif + return result; } From 64841345ad4e7d33030d6f5109f98c57e3175952 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 24 Apr 2024 05:16:33 +0000 Subject: [PATCH 02/35] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst diff --git a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst new file mode 100644 index 00000000000000..cef73500cc202d --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst @@ -0,0 +1 @@ +Fix crash in :func:`mmap.read` on Windows when the underlying file operation raises errors. From 71218d2db9744cbb67a5269122c479022bca44c0 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Wed, 24 Apr 2024 13:17:06 +0800 Subject: [PATCH 03/35] adjust indentation --- Modules/mmapmodule.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 78eeae6a420647..492c0bdd21bee2 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -319,7 +319,7 @@ mmap_read_method(mmap_object *self, if (num_bytes < 0 || num_bytes > remaining) num_bytes = remaining; - #if defined(MS_WIN32) && !defined(DONT_USE_SEH) +#if defined(MS_WIN32) && !defined(DONT_USE_SEH) EXCEPTION_RECORD record; __try { result = PyBytes_FromStringAndSize(&self->data[self->pos], num_bytes); @@ -330,10 +330,10 @@ mmap_read_method(mmap_object *self, PyErr_SetFromWindowsErr(code); result = NULL; } - #else +#else result = PyBytes_FromStringAndSize(&self->data[self->pos], num_bytes); self->pos += num_bytes; - #endif +#endif return result; } From cd4b1fcbdd2dfdc7fd0f417216fa1534a51e4164 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Thu, 25 Apr 2024 12:23:58 +0800 Subject: [PATCH 04/35] restructure to use safe_memcpy to avoid memory leak --- Modules/mmapmodule.c | 80 ++++++++++++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 26 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 492c0bdd21bee2..b48653c85cc4c0 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -255,6 +255,44 @@ do { \ } while (0) #endif /* UNIX */ +#if defined(MS_WIN32) && !defined(DONT_USE_SEH) +static DWORD +HandlePageException(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) +{ + *record = *ptrs->ExceptionRecord; + if (ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { + return EXCEPTION_EXECUTE_HANDLER; + } + return EXCEPTION_CONTINUE_SEARCH; +} +#endif + +int +safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { +#if defined(MS_WIN32) && !defined(DONT_USE_SEH) + + // never fail for count 0 + if (count == 0) { + return 0; + } + + EXCEPTION_RECORD record; + __try { + memcpy(dest, src, count); + return 0; + } + __except (HandlePageException(GetExceptionInformation(), &record)) { + NTSTATUS status = record.ExceptionInformation[2]; + ULONG code = LsaNtStatusToWinError(status); + PyErr_SetFromWindowsErr(code); + return -1; + } +#else + memcpy(dest, src, count); + return 0; +#endif +} + static PyObject * mmap_read_byte_method(mmap_object *self, PyObject *Py_UNUSED(ignored)) @@ -264,7 +302,14 @@ mmap_read_byte_method(mmap_object *self, PyErr_SetString(PyExc_ValueError, "read byte out of range"); return NULL; } - return PyLong_FromLong((unsigned char)self->data[self->pos++]); + unsigned char dest; + if (safe_memcpy(dest, self->data + self->pos, 1) < 0) { + return NULL; + } + else { + self->pos++; + return PyLong_FromLong(dest); + } } static PyObject * @@ -291,17 +336,6 @@ mmap_read_line_method(mmap_object *self, return result; } -#if defined(MS_WIN32) && !defined(DONT_USE_SEH) -static DWORD HandlePageException(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) -{ - *record = *ptrs->ExceptionRecord; - if (ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { - return EXCEPTION_EXECUTE_HANDLER; - } - return EXCEPTION_CONTINUE_SEARCH; -} -#endif - static PyObject * mmap_read_method(mmap_object *self, PyObject *args) @@ -319,22 +353,16 @@ mmap_read_method(mmap_object *self, if (num_bytes < 0 || num_bytes > remaining) num_bytes = remaining; -#if defined(MS_WIN32) && !defined(DONT_USE_SEH) - EXCEPTION_RECORD record; - __try { - result = PyBytes_FromStringAndSize(&self->data[self->pos], num_bytes); - self->pos += num_bytes; + result = PyBytes_FromStringAndSize(NULL, num_bytes); + if (result == NULL) { + return NULL; } - __except (HandlePageException(GetExceptionInformation(), &record)) { - NTSTATUS code = record.ExceptionInformation[2]; - PyErr_SetFromWindowsErr(code); - result = NULL; + if (safe_memcpy(((PyBytesObject *) result)->ob_sval, self->data + self->pos, num_bytes) < 0) { + Py_CLEAR(result); + } + else { + self->pos += num_bytes; } -#else - result = PyBytes_FromStringAndSize(&self->data[self->pos], num_bytes); - self->pos += num_bytes; -#endif - return result; } From cb1ef197f07b582b378e149c447017e9ff9f8239 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Thu, 25 Apr 2024 13:01:32 +0800 Subject: [PATCH 05/35] Apply suggestions from code review rename HandlePageException to filter_page_exception Co-authored-by: Eryk Sun --- Modules/mmapmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index b48653c85cc4c0..72eb770e2720a6 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -257,7 +257,7 @@ do { \ #if defined(MS_WIN32) && !defined(DONT_USE_SEH) static DWORD -HandlePageException(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) +filter_page_exception(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) { *record = *ptrs->ExceptionRecord; if (ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { @@ -281,7 +281,7 @@ safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { memcpy(dest, src, count); return 0; } - __except (HandlePageException(GetExceptionInformation(), &record)) { + __except (filter_page_exception(GetExceptionInformation(), &record)) { NTSTATUS status = record.ExceptionInformation[2]; ULONG code = LsaNtStatusToWinError(status); PyErr_SetFromWindowsErr(code); From 448fa900362d2b789214ba6c72bf9f2a39a49830 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Fri, 26 Apr 2024 12:27:30 +0800 Subject: [PATCH 06/35] fix passing dest as pointer in mmap_read_byte_method Co-authored-by: Steve Dower --- Modules/mmapmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 72eb770e2720a6..5f59b4f49e16ca 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -303,7 +303,7 @@ mmap_read_byte_method(mmap_object *self, return NULL; } unsigned char dest; - if (safe_memcpy(dest, self->data + self->pos, 1) < 0) { + if (safe_memcpy(&dest, self->data + self->pos, 1) < 0) { return NULL; } else { From ba96163c03ac3fd2f754262aca0d56e159bf801e Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Fri, 26 Apr 2024 12:30:02 +0800 Subject: [PATCH 07/35] use PyBytes_AS_STRING instead of raw access Co-authored-by: Steve Dower --- Modules/mmapmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 5f59b4f49e16ca..0133dc1f53903f 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -357,7 +357,7 @@ mmap_read_method(mmap_object *self, if (result == NULL) { return NULL; } - if (safe_memcpy(((PyBytesObject *) result)->ob_sval, self->data + self->pos, num_bytes) < 0) { + if (safe_memcpy(PyBytes_AS_STRING(result), self->data + self->pos, num_bytes) < 0) { Py_CLEAR(result); } else { From f97d2131253cb2e92813efcec1357a877f79d91d Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Fri, 26 Apr 2024 12:38:18 +0800 Subject: [PATCH 08/35] handle mmap_write_method --- .../2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst | 2 +- Modules/mmapmodule.c | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst index cef73500cc202d..ce807e7ca938d1 100644 --- a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst +++ b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst @@ -1 +1 @@ -Fix crash in :func:`mmap.read` on Windows when the underlying file operation raises errors. +Fix crashes in :func:`mmap.read`, :func:`mmap.read_byte` and :func:`mmap.write` on Windows when the underlying file operation raises errors. diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 0133dc1f53903f..781e5c2af134ce 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -486,10 +486,15 @@ mmap_write_method(mmap_object *self, } CHECK_VALID_OR_RELEASE(NULL, data); - memcpy(&self->data[self->pos], data.buf, data.len); - self->pos += data.len; - PyBuffer_Release(&data); - return PyLong_FromSsize_t(data.len); + if (safe_memcpy(self->data + self->pos, data.buf, data.len) < 0) { + PyBuffer_Release(&data); + return NULL; + } + else { + self->pos += data.len; + PyBuffer_Release(&data); + return PyLong_FromSsize_t(data.len); + } } static PyObject * From 79838690dcf98ebaf78afd976a33ab0ca74d6e4f Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Fri, 26 Apr 2024 12:46:30 +0800 Subject: [PATCH 09/35] handle mmap_write_byte_method --- .../2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst | 3 ++- Modules/mmapmodule.c | 14 +++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst index ce807e7ca938d1..ebc56e36123e29 100644 --- a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst +++ b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst @@ -1 +1,2 @@ -Fix crashes in :func:`mmap.read`, :func:`mmap.read_byte` and :func:`mmap.write` on Windows when the underlying file operation raises errors. +Fix crashes in :func:`mmap.read`, :func:`mmap.read_byte`, :func:`mmap.write` +and :func:`mmap.write_byte` on Windows when the underlying file operation raises errors. diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 781e5c2af134ce..cefe76a5ec0fd9 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -511,14 +511,18 @@ mmap_write_byte_method(mmap_object *self, return NULL; CHECK_VALID(NULL); - if (self->pos < self->size) { - self->data[self->pos++] = value; - Py_RETURN_NONE; - } - else { + if (self->pos >= self->size) { PyErr_SetString(PyExc_ValueError, "write byte out of range"); return NULL; } + + if (safe_memcpy(self->data + self->pos, value, 1) < 0) { + return NULL; + } + else { + self->pos++; + Py_RETURN_NONE; + } } static PyObject * From c0bd57be5a935018ef1f0c1d51e3dbbf1bdce60c Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 30 Apr 2024 18:36:59 +0800 Subject: [PATCH 10/35] add safe_byte_copy, safe_memmove, safe_memchr, safe_copy_from_slice and safe_copy_to_slice handle SEH in mmap_read_line_method, mmap_item, mmap_ass_item and mmap_subscript, mmap_ass_subscript, mmap_move_method --- Modules/mmapmodule.c | 196 +++++++++++++++++++++++++++++++++---------- 1 file changed, 153 insertions(+), 43 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index cefe76a5ec0fd9..f352c015426fbe 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -26,6 +26,8 @@ #include "pycore_abstract.h" // _Py_convert_optional_to_ssize_t() #include "pycore_bytesobject.h" // _PyBytes_Find() #include "pycore_fileutils.h" // _Py_stat_struct +#include "pycore_global_objects.h"// _Py_SINGLETON +#include "pycore_runtime.h" // _PyRuntime #include // offsetof() #ifndef MS_WINDOWS @@ -41,6 +43,7 @@ #ifdef MS_WINDOWS #include +#include // LsaNtStatusToWinError static int my_getpagesize(void) { @@ -255,6 +258,11 @@ do { \ } while (0) #endif /* UNIX */ +// copied from bytesobject.c +#define CHARACTERS _Py_SINGLETON(bytes_characters) +#define CHARACTER(ch) \ + ((PyBytesObject *)&(CHARACTERS[ch])); + #if defined(MS_WIN32) && !defined(DONT_USE_SEH) static DWORD filter_page_exception(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) @@ -272,6 +280,8 @@ safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { #if defined(MS_WIN32) && !defined(DONT_USE_SEH) // never fail for count 0 + // according to https://en.cppreference.com/w/cpp/string/byte/memcpy + // If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero. if (count == 0) { return 0; } @@ -282,7 +292,7 @@ safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { return 0; } __except (filter_page_exception(GetExceptionInformation(), &record)) { - NTSTATUS status = record.ExceptionInformation[2]; + NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; ULONG code = LsaNtStatusToWinError(status); PyErr_SetFromWindowsErr(code); return -1; @@ -293,6 +303,69 @@ safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { #endif } +#if defined(MS_WIN32) && !defined(DONT_USE_SEH) +#define handle_invalid_mem(sourcecode) \ + EXCEPTION_RECORD record; \ + __try { \ + sourcecode \ + return 0; \ + } \ + __except (filter_page_exception(GetExceptionInformation(), &record)) { \ + NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ + ULONG code = LsaNtStatusToWinError(status); \ + PyErr_SetFromWindowsErr(code); \ + return -1; \ + } +#else +#define handle_invalid_mem(sourcecode) \ +sourcecode \ +return 0; +#endif + +int +safe_byte_copy(char *dest, const char *src) { + handle_invalid_mem( + *dest = *src; + ) +} + +int +safe_memchr(void **out, const void *ptr, int ch, size_t count) { + handle_invalid_mem( + *out = memchr(ptr, ch, count); + ) +} + +int +safe_memmove(void *dest, const void *src, size_t count) { + handle_invalid_mem( + memmove(dest, src, count); + ) +} + +int +safe_copy_from_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t step, Py_ssize_t slicelen) { + handle_invalid_mem( + size_t cur; + Py_ssize_t i; + for (cur = start, i = 0; i < slicelen; cur += step, i++) { + dest[cur] = src[i]; + } + ) +} + +int +safe_copy_to_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t step, Py_ssize_t slicelen) { + handle_invalid_mem( + size_t cur; + Py_ssize_t i; + for (cur = start, i = 0; i < slicelen; cur += step, i++) { + dest[i] = src[cur]; + } + ) +} + + static PyObject * mmap_read_byte_method(mmap_object *self, PyObject *Py_UNUSED(ignored)) @@ -302,8 +375,8 @@ mmap_read_byte_method(mmap_object *self, PyErr_SetString(PyExc_ValueError, "read byte out of range"); return NULL; } - unsigned char dest; - if (safe_memcpy(&dest, self->data + self->pos, 1) < 0) { + char dest; + if (safe_byte_copy(&dest, self->data + self->pos) < 0) { return NULL; } else { @@ -312,13 +385,42 @@ mmap_read_byte_method(mmap_object *self, } } +PyObject * +_read_mmap_mem(mmap_object *self, char *start, size_t num_bytes) { + if (num_bytes == 1) { + char dest; + if (safe_byte_copy(&dest, start) < 0) { + return NULL; + } + else { + PyBytesObject *op = CHARACTER(dest & 255); + assert(_Py_IsImmortal(op)); + self->pos += 1; + return (PyObject *)op; + } + } + else { + PyObject *result = PyBytes_FromStringAndSize(NULL, num_bytes); + // this check was not done previously in mmap_read_line_method, which means pos was increased even in case of error + if (result == NULL) { + return NULL; + } + if (safe_memcpy(PyBytes_AS_STRING(result), start, num_bytes) < 0) { + Py_CLEAR(result); + } + else { + self->pos += num_bytes; + } + return result; + } +} + static PyObject * mmap_read_line_method(mmap_object *self, PyObject *Py_UNUSED(ignored)) { Py_ssize_t remaining; char *start, *eol; - PyObject *result; CHECK_VALID(NULL); @@ -326,14 +428,17 @@ mmap_read_line_method(mmap_object *self, if (!remaining) return PyBytes_FromString(""); start = self->data + self->pos; - eol = memchr(start, '\n', remaining); + + if (safe_memchr(&eol, start, '\n', remaining) < 0) { + return NULL; + } + if (!eol) eol = self->data + self->size; else ++eol; /* advance past newline */ - result = PyBytes_FromStringAndSize(start, (eol - start)); - self->pos += (eol - start); - return result; + + return _read_mmap_mem(self, start, eol - start); } static PyObject * @@ -341,7 +446,6 @@ mmap_read_method(mmap_object *self, PyObject *args) { Py_ssize_t num_bytes = PY_SSIZE_T_MAX, remaining; - PyObject *result; CHECK_VALID(NULL); if (!PyArg_ParseTuple(args, "|O&:read", _Py_convert_optional_to_ssize_t, &num_bytes)) @@ -353,17 +457,7 @@ mmap_read_method(mmap_object *self, if (num_bytes < 0 || num_bytes > remaining) num_bytes = remaining; - result = PyBytes_FromStringAndSize(NULL, num_bytes); - if (result == NULL) { - return NULL; - } - if (safe_memcpy(PyBytes_AS_STRING(result), self->data + self->pos, num_bytes) < 0) { - Py_CLEAR(result); - } - else { - self->pos += num_bytes; - } - return result; + return _read_mmap_mem(self, self->data + self->pos, num_bytes); } static PyObject * @@ -516,7 +610,7 @@ mmap_write_byte_method(mmap_object *self, return NULL; } - if (safe_memcpy(self->data + self->pos, value, 1) < 0) { + if (safe_byte_copy(self->data + self->pos, &value) < 0) { return NULL; } else { @@ -826,8 +920,9 @@ mmap_move_method(mmap_object *self, PyObject *args) goto bounds; CHECK_VALID(NULL); - memmove(&self->data[dest], &self->data[src], cnt); - + if (safe_memmove(self->data + dest, self->data + src, cnt) < 0) { + return NULL; + }; Py_RETURN_NONE; bounds: @@ -1031,7 +1126,16 @@ mmap_item(mmap_object *self, Py_ssize_t i) PyErr_SetString(PyExc_IndexError, "mmap index out of range"); return NULL; } - return PyBytes_FromStringAndSize(self->data + i, 1); + + char dest; + if (safe_byte_copy(&dest, self->data + i) < 0) { + return NULL; + } + else { + PyBytesObject *op = CHARACTER(dest & 255); + assert(_Py_IsImmortal(op)); + return (PyObject *)op; + } } static PyObject * @@ -1068,17 +1172,16 @@ mmap_subscript(mmap_object *self, PyObject *item) slicelen); else { char *result_buf = (char *)PyMem_Malloc(slicelen); - size_t cur; - Py_ssize_t i; PyObject *result; if (result_buf == NULL) return PyErr_NoMemory(); - for (cur = start, i = 0; i < slicelen; - cur += step, i++) { - result_buf[i] = self->data[cur]; + if (safe_copy_to_slice(result_buf, self->data, start, step, slicelen) < 0) { + PyMem_Free(result_buf); + return NULL; } + result = PyBytes_FromStringAndSize(result_buf, slicelen); PyMem_Free(result_buf); @@ -1115,8 +1218,13 @@ mmap_ass_item(mmap_object *self, Py_ssize_t i, PyObject *v) if (!is_writable(self)) return -1; buf = PyBytes_AsString(v); - self->data[i] = buf[0]; - return 0; + + if (safe_byte_copy(self->data + i, buf) < 0) { + return -1; + } + else { + return 0; + } } static int @@ -1160,8 +1268,13 @@ mmap_ass_subscript(mmap_object *self, PyObject *item, PyObject *value) return -1; } CHECK_VALID(-1); - self->data[i] = (char) v; - return 0; + + if (safe_byte_copy(self->data + i, (char *) &v) < 0) { + return -1; + } + else { + return 0; + } } else if (PySlice_Check(item)) { Py_ssize_t start, stop, step, slicelen; @@ -1186,24 +1299,21 @@ mmap_ass_subscript(mmap_object *self, PyObject *item, PyObject *value) } CHECK_VALID_OR_RELEASE(-1, vbuf); + int result = 0; if (slicelen == 0) { } else if (step == 1) { - memcpy(self->data + start, vbuf.buf, slicelen); + if (safe_memcpy(self->data + start, vbuf.buf, slicelen) < 0) { + result = -1; + } } else { - size_t cur; - Py_ssize_t i; - - for (cur = start, i = 0; - i < slicelen; - cur += step, i++) - { - self->data[cur] = ((char *)vbuf.buf)[i]; + if (safe_copy_from_slice(self->data, (char *)vbuf.buf, start, step, slicelen) < 0) { + result = -1; } } PyBuffer_Release(&vbuf); - return 0; + return result; } else { PyErr_SetString(PyExc_TypeError, From 92f4bf02778513e08952988c36b5d446d6eee846 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Wed, 1 May 2024 13:34:21 +0800 Subject: [PATCH 11/35] improve macro fix some type issues --- Modules/mmapmodule.c | 62 ++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index f352c015426fbe..9caa2c93ade631 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -304,68 +304,74 @@ safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { } #if defined(MS_WIN32) && !defined(DONT_USE_SEH) -#define handle_invalid_mem(sourcecode) \ - EXCEPTION_RECORD record; \ - __try { \ - sourcecode \ - return 0; \ - } \ +#define HANDLE_INVALID_MEM(sourcecode) \ +do { \ + EXCEPTION_RECORD record; \ + __try { \ + sourcecode \ + } \ __except (filter_page_exception(GetExceptionInformation(), &record)) { \ - NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ - ULONG code = LsaNtStatusToWinError(status); \ - PyErr_SetFromWindowsErr(code); \ - return -1; \ - } + NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ + ULONG code = LsaNtStatusToWinError(status); \ + PyErr_SetFromWindowsErr(code); \ + return -1; \ + } \ +} while (0) #else -#define handle_invalid_mem(sourcecode) \ -sourcecode \ -return 0; +#define HANDLE_INVALID_MEM(sourcecode) \ +do { \ + sourcecode \ +} while (0) #endif int safe_byte_copy(char *dest, const char *src) { - handle_invalid_mem( + HANDLE_INVALID_MEM( *dest = *src; - ) + ); + return 0; } int -safe_memchr(void **out, const void *ptr, int ch, size_t count) { - handle_invalid_mem( - *out = memchr(ptr, ch, count); - ) +safe_memchr(char **out, const void *ptr, int ch, size_t count) { + HANDLE_INVALID_MEM( + *out = (char *) memchr(ptr, ch, count); + ); + return 0; } int safe_memmove(void *dest, const void *src, size_t count) { - handle_invalid_mem( + HANDLE_INVALID_MEM( memmove(dest, src, count); - ) + ); + return 0; } int safe_copy_from_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t step, Py_ssize_t slicelen) { - handle_invalid_mem( + HANDLE_INVALID_MEM( size_t cur; Py_ssize_t i; for (cur = start, i = 0; i < slicelen; cur += step, i++) { dest[cur] = src[i]; } - ) + ); + return 0; } int safe_copy_to_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t step, Py_ssize_t slicelen) { - handle_invalid_mem( + HANDLE_INVALID_MEM( size_t cur; Py_ssize_t i; for (cur = start, i = 0; i < slicelen; cur += step, i++) { dest[i] = src[cur]; } - ) + ); + return 0; } - static PyObject * mmap_read_byte_method(mmap_object *self, PyObject *Py_UNUSED(ignored)) @@ -381,7 +387,7 @@ mmap_read_byte_method(mmap_object *self, } else { self->pos++; - return PyLong_FromLong(dest); + return PyLong_FromLong((unsigned char) dest); } } From 364d8462f9d049f80188f563926b69313785c309 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Wed, 1 May 2024 13:37:30 +0800 Subject: [PATCH 12/35] Update Modules/mmapmodule.c cast to char and then deref instead of casting (char *) Co-authored-by: Steve Dower --- Modules/mmapmodule.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 9caa2c93ade631..9830894fbbf3fb 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -1275,7 +1275,8 @@ mmap_ass_subscript(mmap_object *self, PyObject *item, PyObject *value) } CHECK_VALID(-1); - if (safe_byte_copy(self->data + i, (char *) &v) < 0) { + char v_char = (char) v; + if (safe_byte_copy(self->data + i, &v_char) < 0) { return -1; } else { From 04dc2d900de64aca70c266fd62085f90c154c66e Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Thu, 2 May 2024 18:18:38 +0800 Subject: [PATCH 13/35] handle EXCEPTION_ACCESS_VIOLATION define safe_memcpy in terms of HANDLE_INVALID_MEM --- Modules/mmapmodule.c | 50 ++++++++++++++++---------------------------- 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 9830894fbbf3fb..b26bdc6a11be08 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -268,41 +268,14 @@ static DWORD filter_page_exception(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) { *record = *ptrs->ExceptionRecord; - if (ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { + if (ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_IN_PAGE_ERROR + || ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { return EXCEPTION_EXECUTE_HANDLER; } return EXCEPTION_CONTINUE_SEARCH; } #endif -int -safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { -#if defined(MS_WIN32) && !defined(DONT_USE_SEH) - - // never fail for count 0 - // according to https://en.cppreference.com/w/cpp/string/byte/memcpy - // If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero. - if (count == 0) { - return 0; - } - - EXCEPTION_RECORD record; - __try { - memcpy(dest, src, count); - return 0; - } - __except (filter_page_exception(GetExceptionInformation(), &record)) { - NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; - ULONG code = LsaNtStatusToWinError(status); - PyErr_SetFromWindowsErr(code); - return -1; - } -#else - memcpy(dest, src, count); - return 0; -#endif -} - #if defined(MS_WIN32) && !defined(DONT_USE_SEH) #define HANDLE_INVALID_MEM(sourcecode) \ do { \ @@ -311,9 +284,14 @@ do { \ sourcecode \ } \ __except (filter_page_exception(GetExceptionInformation(), &record)) { \ - NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ - ULONG code = LsaNtStatusToWinError(status); \ - PyErr_SetFromWindowsErr(code); \ + if (record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { \ + NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ + ULONG code = LsaNtStatusToWinError(status); \ + PyErr_SetFromWindowsErr(code); \ + } \ + else if (record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { \ + PyErr_SetFromWindowsErr(ERROR_NOACCESS); \ + } \ return -1; \ } \ } while (0) @@ -324,6 +302,14 @@ do { \ } while (0) #endif +int +safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { + HANDLE_INVALID_MEM( + memcpy(dest, src, count); + ); + return 0; +} + int safe_byte_copy(char *dest, const char *src) { HANDLE_INVALID_MEM( From 0654f9b99d0f5b7f53ab08a59c0fdf77aacc8a4a Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Thu, 2 May 2024 18:20:15 +0800 Subject: [PATCH 14/35] fix formatting --- Modules/mmapmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index b26bdc6a11be08..69e8673a30df5e 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -277,7 +277,7 @@ filter_page_exception(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) #endif #if defined(MS_WIN32) && !defined(DONT_USE_SEH) -#define HANDLE_INVALID_MEM(sourcecode) \ +#define HANDLE_INVALID_MEM(sourcecode) \ do { \ EXCEPTION_RECORD record; \ __try { \ @@ -290,7 +290,7 @@ do { \ PyErr_SetFromWindowsErr(code); \ } \ else if (record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { \ - PyErr_SetFromWindowsErr(ERROR_NOACCESS); \ + PyErr_SetFromWindowsErr(ERROR_NOACCESS); \ } \ return -1; \ } \ From d3661ff3836ff92907971207b524bb0d4d8e11c9 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Thu, 2 May 2024 18:27:22 +0800 Subject: [PATCH 15/35] use PyBytes_FromStringAndSize instead of extracting the optization --- Modules/mmapmodule.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 69e8673a30df5e..3cae038a326f28 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -26,8 +26,6 @@ #include "pycore_abstract.h" // _Py_convert_optional_to_ssize_t() #include "pycore_bytesobject.h" // _PyBytes_Find() #include "pycore_fileutils.h" // _Py_stat_struct -#include "pycore_global_objects.h"// _Py_SINGLETON -#include "pycore_runtime.h" // _PyRuntime #include // offsetof() #ifndef MS_WINDOWS @@ -258,11 +256,6 @@ do { \ } while (0) #endif /* UNIX */ -// copied from bytesobject.c -#define CHARACTERS _Py_SINGLETON(bytes_characters) -#define CHARACTER(ch) \ - ((PyBytesObject *)&(CHARACTERS[ch])); - #if defined(MS_WIN32) && !defined(DONT_USE_SEH) static DWORD filter_page_exception(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) @@ -385,10 +378,9 @@ _read_mmap_mem(mmap_object *self, char *start, size_t num_bytes) { return NULL; } else { - PyBytesObject *op = CHARACTER(dest & 255); - assert(_Py_IsImmortal(op)); + PyObject *result = PyBytes_FromStringAndSize(&dest, 1); self->pos += 1; - return (PyObject *)op; + return result; } } else { @@ -1124,9 +1116,7 @@ mmap_item(mmap_object *self, Py_ssize_t i) return NULL; } else { - PyBytesObject *op = CHARACTER(dest & 255); - assert(_Py_IsImmortal(op)); - return (PyObject *)op; + return PyBytes_FromStringAndSize(&dest, 1); } } From 30aa64b954fea2143775914d28cbc7170979073f Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Fri, 3 May 2024 17:28:39 +0800 Subject: [PATCH 16/35] update news entry --- .../2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst index ebc56e36123e29..e430b9afd6dd01 100644 --- a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst +++ b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst @@ -1,2 +1,12 @@ -Fix crashes in :func:`mmap.read`, :func:`mmap.read_byte`, :func:`mmap.write` -and :func:`mmap.write_byte` on Windows when the underlying file operation raises errors. +Fix crashes in + +* :func:`mmap.move` +* :func:`mmap.read` +* :func:`mmap.read_byte` +* :func:`mmap.readline` +* :func:`mmap.write` +* :func:`mmap.write_byte` +* :func:`mmap.__getitem__` +* :func:`mmap.__setitem__` + +on Windows when the underlying file operation raises errors or an access violation occurs. From b809ce997153d8f2ef7b55cf637665b9cfc95bd9 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Mon, 6 May 2024 21:55:37 +0800 Subject: [PATCH 17/35] Update Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst Co-authored-by: Steve Dower --- .../2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst index e430b9afd6dd01..da70b2528919e1 100644 --- a/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst +++ b/Misc/NEWS.d/next/Windows/2024-04-24-05-16-32.gh-issue-118209.Ryyzlz.rst @@ -1,12 +1,2 @@ -Fix crashes in - -* :func:`mmap.move` -* :func:`mmap.read` -* :func:`mmap.read_byte` -* :func:`mmap.readline` -* :func:`mmap.write` -* :func:`mmap.write_byte` -* :func:`mmap.__getitem__` -* :func:`mmap.__setitem__` - -on Windows when the underlying file operation raises errors or an access violation occurs. +Avoid crashing in :mod:`mmap` on Windows when the mapped memory is inaccessible +due to file system errors or access violations. From e907e6d323d13c5cd678151c3927fcc66da9bb8e Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Mon, 6 May 2024 22:00:56 +0800 Subject: [PATCH 18/35] updated whatsnew entry --- Doc/whatsnew/3.13.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index 89694afdfa3fec..022fe12de0112a 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -568,6 +568,9 @@ mmap * :class:`mmap.mmap` now has a *trackfd* parameter on Unix; if it is ``False``, the file descriptor specified by *fileno* will not be duplicated. (Contributed by Zackery Spytz and Petr Viktorin in :gh:`78502`.) +:class:`mmap.mmap` is now protected from crashing on Windows when the mapped memory + is inaccessible due to file system errors or access violations. + (Contributed by Jannis Weigend in :gh:`118209`.) opcode ------ From d1ad0ab496e55004fb08126026e0a63c27ac9260 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Mon, 6 May 2024 23:00:58 +0800 Subject: [PATCH 19/35] fix typo --- Doc/whatsnew/3.13.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index 022fe12de0112a..f74476ca35d337 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -568,7 +568,7 @@ mmap * :class:`mmap.mmap` now has a *trackfd* parameter on Unix; if it is ``False``, the file descriptor specified by *fileno* will not be duplicated. (Contributed by Zackery Spytz and Petr Viktorin in :gh:`78502`.) -:class:`mmap.mmap` is now protected from crashing on Windows when the mapped memory +* :class:`mmap.mmap` is now protected from crashing on Windows when the mapped memory is inaccessible due to file system errors or access violations. (Contributed by Jannis Weigend in :gh:`118209`.) From 2ca440d29c71e1aab1f992b7619719a860a28916 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 01:14:06 +0800 Subject: [PATCH 20/35] fix handling some cases in mmap_subscript --- Modules/mmapmodule.c | 69 +++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 3cae038a326f28..bad94ac6372d4e 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -351,27 +351,8 @@ safe_copy_to_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t ste return 0; } -static PyObject * -mmap_read_byte_method(mmap_object *self, - PyObject *Py_UNUSED(ignored)) -{ - CHECK_VALID(NULL); - if (self->pos >= self->size) { - PyErr_SetString(PyExc_ValueError, "read byte out of range"); - return NULL; - } - char dest; - if (safe_byte_copy(&dest, self->data + self->pos) < 0) { - return NULL; - } - else { - self->pos++; - return PyLong_FromLong((unsigned char) dest); - } -} - PyObject * -_read_mmap_mem(mmap_object *self, char *start, size_t num_bytes) { +_get_mmap_mem(char *start, size_t num_bytes) { if (num_bytes == 1) { char dest; if (safe_byte_copy(&dest, start) < 0) { @@ -379,26 +360,40 @@ _read_mmap_mem(mmap_object *self, char *start, size_t num_bytes) { } else { PyObject *result = PyBytes_FromStringAndSize(&dest, 1); - self->pos += 1; return result; } } else { PyObject *result = PyBytes_FromStringAndSize(NULL, num_bytes); - // this check was not done previously in mmap_read_line_method, which means pos was increased even in case of error if (result == NULL) { return NULL; } if (safe_memcpy(PyBytes_AS_STRING(result), start, num_bytes) < 0) { Py_CLEAR(result); } - else { - self->pos += num_bytes; - } return result; } } +static PyObject * +mmap_read_byte_method(mmap_object *self, + PyObject *Py_UNUSED(ignored)) +{ + CHECK_VALID(NULL); + if (self->pos >= self->size) { + PyErr_SetString(PyExc_ValueError, "read byte out of range"); + return NULL; + } + char dest; + if (safe_byte_copy(&dest, self->data + self->pos) < 0) { + return NULL; + } + else { + self->pos++; + return PyLong_FromLong((unsigned char) dest); + } +} + static PyObject * mmap_read_line_method(mmap_object *self, PyObject *Py_UNUSED(ignored)) @@ -422,7 +417,11 @@ mmap_read_line_method(mmap_object *self, else ++eol; /* advance past newline */ - return _read_mmap_mem(self, start, eol - start); + PyObject *result = _get_mmap_mem(start, eol - start); + if (result != NULL) { + self->pos += (eol - start); + } + return result; } static PyObject * @@ -441,7 +440,11 @@ mmap_read_method(mmap_object *self, if (num_bytes < 0 || num_bytes > remaining) num_bytes = remaining; - return _read_mmap_mem(self, self->data + self->pos, num_bytes); + PyObject *result = _get_mmap_mem(self->data + self->pos, num_bytes); + if (result != NULL) { + self->pos += num_bytes; + } + return result; } static PyObject * @@ -1136,7 +1139,14 @@ mmap_subscript(mmap_object *self, PyObject *item) return NULL; } CHECK_VALID(NULL); - return PyLong_FromLong(Py_CHARMASK(self->data[i])); + + char dest; + if (safe_byte_copy(&dest, self->data + i) < 0) { + return NULL; + } + else { + return PyLong_FromLong(Py_CHARMASK(dest)); + } } else if (PySlice_Check(item)) { Py_ssize_t start, stop, step, slicelen; @@ -1150,8 +1160,7 @@ mmap_subscript(mmap_object *self, PyObject *item) if (slicelen <= 0) return PyBytes_FromStringAndSize("", 0); else if (step == 1) - return PyBytes_FromStringAndSize(self->data + start, - slicelen); + return _get_mmap_mem(self->data + start, slicelen); else { char *result_buf = (char *)PyMem_Malloc(slicelen); PyObject *result; From f7c356c75ace5a7448ac6d121de628124d47ad7c Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 01:19:28 +0800 Subject: [PATCH 21/35] add test using private function --- Lib/test/test_mmap.py | 38 ++++++++++++++++++++++++++++++++++++++ Modules/mmapmodule.c | 26 +++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index ee86227e026b67..26ecbd8778c696 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1058,6 +1058,44 @@ def __exit__(self, exc_type, exc_value, traceback): with self.assertRaisesRegex(ValueError, "mmap closed or invalid"): m.write_byte(X()) + @unittest.skipUnless(os.name == 'nt', 'requires Windows') + def test_access_violations(self): + PAGE_NOACCESS = 0x01 + + with open(TESTFN, 'bw+') as f: + f.write(b'\0'* PAGESIZE) + f.flush() + + m = mmap.mmap(f.fileno(), PAGESIZE) + m._protect(PAGE_NOACCESS, 0, PAGESIZE) + with self.assertRaises(OSError): + m.read(PAGESIZE) + with self.assertRaises(OSError): + m.read_byte() + with self.assertRaises(OSError): + m.readline() + with self.assertRaises(OSError): + m.write(b'\0'* PAGESIZE) + with self.assertRaises(OSError): + m.write_byte(0) + with self.assertRaises(OSError): + m[0] # test mmap_subscript + with self.assertRaises(OSError): + m[0:10] # test mmap_subscript + with self.assertRaises(OSError): + m[0:10:2] # test mmap_subscript + with self.assertRaises(OSError): + m[0] = 1 + with self.assertRaises(OSError): + m[0:10] = b'\0'* 10 + with self.assertRaises(OSError): + m[0:10:2] = b'\0'* 5 + with self.assertRaises(OSError): + m.move(0, 10, 1) + with self.assertRaises(OSError): + list(m) # test mmap_item + + class LargeMmapTests(unittest.TestCase): def setUp(self): diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index bad94ac6372d4e..d758a2e1670710 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -1000,6 +1000,27 @@ mmap__sizeof__method(mmap_object *self, void *Py_UNUSED(ignored)) } #endif +#if defined(MS_WINDOWS) && defined(Py_DEBUG) +static PyObject * +mmap_protect_method(mmap_object *self, PyObject *args) { + DWORD flNewProtect, flOldProtect; + Py_ssize_t start, length; + + CHECK_VALID(NULL); + + if (!PyArg_ParseTuple(args, "Inn:protect", &flNewProtect, &start, &length)) { + return NULL; + } + + if (VirtualProtect((void *) (self->data + start), length, flNewProtect, &flOldProtect) == 0) { + PyErr_SetFromWindowsErr(GetLastError()); + return NULL; + } + + Py_RETURN_NONE; +} +#endif + #ifdef HAVE_MADVISE static PyObject * mmap_madvise_method(mmap_object *self, PyObject *args) @@ -1069,7 +1090,10 @@ static struct PyMethodDef mmap_object_methods[] = { {"__exit__", (PyCFunction) mmap__exit__method, METH_VARARGS}, #ifdef MS_WINDOWS {"__sizeof__", (PyCFunction) mmap__sizeof__method, METH_NOARGS}, -#endif +#ifdef Py_DEBUG + {"_protect", (PyCFunction) mmap_protect_method, METH_VARARGS}, +#endif // Py_DEBUG +#endif // MS_WINDOWS {NULL, NULL} /* sentinel */ }; From 0d701086d2884e4cfe36616e90810d23dbc41d79 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 02:02:26 +0800 Subject: [PATCH 22/35] Update Lib/test/test_mmap.py Co-authored-by: Eryk Sun --- Lib/test/test_mmap.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 26ecbd8778c696..9f2be0cfc770dc 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1059,6 +1059,7 @@ def __exit__(self, exc_type, exc_value, traceback): m.write_byte(X()) @unittest.skipUnless(os.name == 'nt', 'requires Windows') + @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') def test_access_violations(self): PAGE_NOACCESS = 0x01 From 6f1f7266a83a03d1ff1d4c68df9cd1c849934479 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 02:25:07 +0800 Subject: [PATCH 23/35] run test in subprocess --- Lib/test/test_mmap.py | 84 +++++++++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 34 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 9f2be0cfc770dc..f39dc2f01a9e69 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -3,6 +3,7 @@ ) from test.support.import_helper import import_module from test.support.os_helper import TESTFN, unlink +from test.support.script_helper import assert_python_ok import unittest import errno import os @@ -12,6 +13,7 @@ import socket import string import sys +import textwrap import weakref # Skip test if we can't import mmap. @@ -1061,41 +1063,55 @@ def __exit__(self, exc_type, exc_value, traceback): @unittest.skipUnless(os.name == 'nt', 'requires Windows') @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') def test_access_violations(self): - PAGE_NOACCESS = 0x01 - - with open(TESTFN, 'bw+') as f: - f.write(b'\0'* PAGESIZE) - f.flush() - - m = mmap.mmap(f.fileno(), PAGESIZE) - m._protect(PAGE_NOACCESS, 0, PAGESIZE) - with self.assertRaises(OSError): - m.read(PAGESIZE) - with self.assertRaises(OSError): - m.read_byte() - with self.assertRaises(OSError): - m.readline() - with self.assertRaises(OSError): - m.write(b'\0'* PAGESIZE) - with self.assertRaises(OSError): - m.write_byte(0) - with self.assertRaises(OSError): - m[0] # test mmap_subscript - with self.assertRaises(OSError): - m[0:10] # test mmap_subscript - with self.assertRaises(OSError): - m[0:10:2] # test mmap_subscript - with self.assertRaises(OSError): - m[0] = 1 - with self.assertRaises(OSError): - m[0:10] = b'\0'* 10 - with self.assertRaises(OSError): - m[0:10:2] = b'\0'* 5 - with self.assertRaises(OSError): - m.move(0, 10, 1) - with self.assertRaises(OSError): - list(m) # test mmap_item + + code = textwrap.dedent(""" + from test.support.os_helper import TESTFN + import mmap + import os + from contextlib import suppress + + if os.path.exists(TESTFN): + os.unlink(TESTFN) + PAGESIZE = mmap.PAGESIZE + PAGE_NOACCESS = 0x01 + + with open(TESTFN, 'bw+') as f: + f.write(b'A'* PAGESIZE) + f.flush() + + m = mmap.mmap(f.fileno(), PAGESIZE) + m._protect(PAGE_NOACCESS, 0, PAGESIZE) + with suppress(OSError): + m.read(PAGESIZE) + with suppress(OSError): + m.read_byte() + with suppress(OSError): + m.readline() + with suppress(OSError): + m.write(b'A'* PAGESIZE) + with suppress(OSError): + m.write_byte(0) + with suppress(OSError): + m[0] # test mmap_subscript + with suppress(OSError): + m[0:10] # test mmap_subscript + with suppress(OSError): + m[0:10:2] # test mmap_subscript + with suppress(OSError): + m[0] = 1 + with suppress(OSError): + m[0:10] = b'A'* 10 + with suppress(OSError): + m[0:10:2] = b'A'* 5 + with suppress(OSError): + m.move(0, 10, 1) + with suppress(OSError): + list(m) # test mmap_item + """) + # stderr includes "Windows fatal exception: access violation" logs, + # even when the access violations are correctly transformed into exceptions + assert_python_ok("-c", code) class LargeMmapTests(unittest.TestCase): From 05e8ca0be42563830a50097ce5610d9f2398e8b0 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 02:31:55 +0800 Subject: [PATCH 24/35] add expected failure for find method --- Lib/test/test_mmap.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index f39dc2f01a9e69..c7794e4ff6a8c6 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1113,6 +1113,34 @@ def test_access_violations(self): # even when the access violations are correctly transformed into exceptions assert_python_ok("-c", code) + @unittest.skipUnless(os.name == 'nt', 'requires Windows') + @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') + @unittest.expectedFailure + def test_access_violations_fail(self): + + code = textwrap.dedent(""" + from test.support.os_helper import TESTFN + import mmap + import os + from contextlib import suppress + + if os.path.exists(TESTFN): + os.unlink(TESTFN) + + PAGESIZE = mmap.PAGESIZE + PAGE_NOACCESS = 0x01 + + with open(TESTFN, 'bw+') as f: + f.write(b'A'* PAGESIZE) + f.flush() + + m = mmap.mmap(f.fileno(), PAGESIZE) + m._protect(PAGE_NOACCESS, 0, PAGESIZE) + with suppress(OSError): + m.find(b'A') + """) + assert_python_ok("-c", code) + class LargeMmapTests(unittest.TestCase): def setUp(self): From 29b12bf05901b0b682cf6dad3018101d22f5a9cc Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 02:34:30 +0800 Subject: [PATCH 25/35] disable faulthandler --- Lib/test/test_mmap.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index c7794e4ff6a8c6..8963102e14803b 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1066,10 +1066,13 @@ def test_access_violations(self): code = textwrap.dedent(""" from test.support.os_helper import TESTFN + import faulthandler import mmap import os from contextlib import suppress + faulthandler.disable() + if os.path.exists(TESTFN): os.unlink(TESTFN) @@ -1109,9 +1112,9 @@ def test_access_violations(self): with suppress(OSError): list(m) # test mmap_item """) - # stderr includes "Windows fatal exception: access violation" logs, - # even when the access violations are correctly transformed into exceptions - assert_python_ok("-c", code) + rt, stdout, stderr = assert_python_ok("-c", code) + self.assertEqual(stdout.rstrip(), b'') + self.assertEqual(stderr.rstrip(), b'') @unittest.skipUnless(os.name == 'nt', 'requires Windows') @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') From 59e8c4e902b591b72f8e8cd71a86b20d62c2fba1 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 02:36:18 +0800 Subject: [PATCH 26/35] typo --- Lib/test/test_mmap.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 8963102e14803b..1c0f25e3c30c43 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1113,8 +1113,8 @@ def test_access_violations(self): list(m) # test mmap_item """) rt, stdout, stderr = assert_python_ok("-c", code) - self.assertEqual(stdout.rstrip(), b'') - self.assertEqual(stderr.rstrip(), b'') + self.assertEqual(stdout.strip(), b'') + self.assertEqual(stderr.strip(), b'') @unittest.skipUnless(os.name == 'nt', 'requires Windows') @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') From 4f610a540b379e1b41b531988b4606c292c31014 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 02:38:55 +0800 Subject: [PATCH 27/35] trailing blanks --- Lib/test/test_mmap.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 1c0f25e3c30c43..ce9872f23b5439 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1063,7 +1063,7 @@ def __exit__(self, exc_type, exc_value, traceback): @unittest.skipUnless(os.name == 'nt', 'requires Windows') @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') def test_access_violations(self): - + code = textwrap.dedent(""" from test.support.os_helper import TESTFN import faulthandler @@ -1120,7 +1120,7 @@ def test_access_violations(self): @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') @unittest.expectedFailure def test_access_violations_fail(self): - + code = textwrap.dedent(""" from test.support.os_helper import TESTFN import mmap From aa2b41d16f030109945b00c9a6624984116c6ae6 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 02:50:34 +0800 Subject: [PATCH 28/35] Update Lib/test/test_mmap.py Co-authored-by: Eryk Sun --- Lib/test/test_mmap.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index ce9872f23b5439..a33a63e9f793b7 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1071,6 +1071,7 @@ def test_access_violations(self): import os from contextlib import suppress + # Prevent logging access violations to stderr. faulthandler.disable() if os.path.exists(TESTFN): From 11fa04ad2258331f3a79b5f6263b4a639c15b3f5 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 03:16:23 +0800 Subject: [PATCH 29/35] add asserts to tests --- Lib/test/test_mmap.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index a33a63e9f793b7..2fee6308243000 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1088,30 +1088,43 @@ def test_access_violations(self): m._protect(PAGE_NOACCESS, 0, PAGESIZE) with suppress(OSError): m.read(PAGESIZE) + assert False, 'mmap.read() did not raise' with suppress(OSError): m.read_byte() + assert False, 'mmap.read_byte() did not raise' with suppress(OSError): m.readline() + assert False, 'mmap.readline() did not raise' with suppress(OSError): m.write(b'A'* PAGESIZE) + assert False, 'mmap.write() did not raise' with suppress(OSError): m.write_byte(0) + assert False, 'mmap.write_byte() did not raise' with suppress(OSError): m[0] # test mmap_subscript + assert False, 'mmap.__getitem__() did not raise' with suppress(OSError): m[0:10] # test mmap_subscript + assert False, 'mmap.__getitem__() did not raise' with suppress(OSError): m[0:10:2] # test mmap_subscript + assert False, 'mmap.__getitem__() did not raise' with suppress(OSError): m[0] = 1 + assert False, 'mmap.__setitem__() did not raise' with suppress(OSError): m[0:10] = b'A'* 10 + assert False, 'mmap.__setitem__() did not raise' with suppress(OSError): m[0:10:2] = b'A'* 5 + assert False, 'mmap.__setitem__() did not raise' with suppress(OSError): m.move(0, 10, 1) + assert False, 'mmap.move() did not raise' with suppress(OSError): list(m) # test mmap_item + assert False, 'mmap.__getitem__() did not raise' """) rt, stdout, stderr = assert_python_ok("-c", code) self.assertEqual(stdout.strip(), b'') @@ -1142,6 +1155,7 @@ def test_access_violations_fail(self): m._protect(PAGE_NOACCESS, 0, PAGESIZE) with suppress(OSError): m.find(b'A') + assert False, 'mmap.find() did not raise' """) assert_python_ok("-c", code) From f02c83a815f9626b211ad1696e8e17afe481850d Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 10:51:31 +0800 Subject: [PATCH 30/35] Update Lib/test/test_mmap.py Co-authored-by: Eryk Sun --- Lib/test/test_mmap.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index 2fee6308243000..bd74af7d753f59 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1141,8 +1141,6 @@ def test_access_violations_fail(self): import os from contextlib import suppress - if os.path.exists(TESTFN): - os.unlink(TESTFN) PAGESIZE = mmap.PAGESIZE PAGE_NOACCESS = 0x01 From 8533af58de4fa7d6c37b5934dcb04e6ebb6864c2 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 18:08:05 +0800 Subject: [PATCH 31/35] support mmap_gfind fix test cleanup --- Lib/test/test_mmap.py | 42 ++++------------- Modules/mmapmodule.c | 105 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 102 insertions(+), 45 deletions(-) diff --git a/Lib/test/test_mmap.py b/Lib/test/test_mmap.py index bd74af7d753f59..a1cf5384ada5b5 100644 --- a/Lib/test/test_mmap.py +++ b/Lib/test/test_mmap.py @@ -1063,24 +1063,22 @@ def __exit__(self, exc_type, exc_value, traceback): @unittest.skipUnless(os.name == 'nt', 'requires Windows') @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') def test_access_violations(self): + from test.support.os_helper import TESTFN code = textwrap.dedent(""" - from test.support.os_helper import TESTFN import faulthandler import mmap import os + import sys from contextlib import suppress # Prevent logging access violations to stderr. faulthandler.disable() - if os.path.exists(TESTFN): - os.unlink(TESTFN) - PAGESIZE = mmap.PAGESIZE PAGE_NOACCESS = 0x01 - with open(TESTFN, 'bw+') as f: + with open(sys.argv[1], 'bw+') as f: f.write(b'A'* PAGESIZE) f.flush() @@ -1125,37 +1123,17 @@ def test_access_violations(self): with suppress(OSError): list(m) # test mmap_item assert False, 'mmap.__getitem__() did not raise' - """) - rt, stdout, stderr = assert_python_ok("-c", code) - self.assertEqual(stdout.strip(), b'') - self.assertEqual(stderr.strip(), b'') - - @unittest.skipUnless(os.name == 'nt', 'requires Windows') - @unittest.skipUnless(hasattr(mmap.mmap, '_protect'), 'test needs debug build') - @unittest.expectedFailure - def test_access_violations_fail(self): - - code = textwrap.dedent(""" - from test.support.os_helper import TESTFN - import mmap - import os - from contextlib import suppress - - - PAGESIZE = mmap.PAGESIZE - PAGE_NOACCESS = 0x01 - - with open(TESTFN, 'bw+') as f: - f.write(b'A'* PAGESIZE) - f.flush() - - m = mmap.mmap(f.fileno(), PAGESIZE) - m._protect(PAGE_NOACCESS, 0, PAGESIZE) with suppress(OSError): m.find(b'A') assert False, 'mmap.find() did not raise' + with suppress(OSError): + m.rfind(b'A') + assert False, 'mmap.rfind() did not raise' """) - assert_python_ok("-c", code) + rt, stdout, stderr = assert_python_ok("-c", code, TESTFN) + self.assertEqual(stdout.strip(), b'') + self.assertEqual(stderr.strip(), b'') + class LargeMmapTests(unittest.TestCase): diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index d758a2e1670710..b38205b3bd290e 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -261,12 +261,29 @@ static DWORD filter_page_exception(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) { *record = *ptrs->ExceptionRecord; - if (ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_IN_PAGE_ERROR - || ptrs->ExceptionRecord->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { + if (record->ExceptionCode == EXCEPTION_IN_PAGE_ERROR + || record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { return EXCEPTION_EXECUTE_HANDLER; } return EXCEPTION_CONTINUE_SEARCH; } + +static DWORD +filter_page_exception_method(mmap_object *self, EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) +{ + *record = *ptrs->ExceptionRecord; + if (record->ExceptionCode == EXCEPTION_IN_PAGE_ERROR + || record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { + + ULONG_PTR address = record->ExceptionInformation[1]; + if (address >= (ULONG_PTR) self->data + && address < (ULONG_PTR) self->data + (ULONG_PTR) self->size) + { + return EXCEPTION_EXECUTE_HANDLER; + } + } + return EXCEPTION_CONTINUE_SEARCH; +} #endif #if defined(MS_WIN32) && !defined(DONT_USE_SEH) @@ -277,6 +294,8 @@ do { \ sourcecode \ } \ __except (filter_page_exception(GetExceptionInformation(), &record)) { \ + assert(record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR \ + || record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION); \ if (record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { \ NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ ULONG code = LsaNtStatusToWinError(status); \ @@ -295,6 +314,34 @@ do { \ } while (0) #endif +#if defined(MS_WIN32) && !defined(DONT_USE_SEH) +#define HANDLE_INVALID_MEM_METHOD(self, sourcecode) \ +do { \ + EXCEPTION_RECORD record; \ + __try { \ + sourcecode \ + } \ + __except (filter_page_exception_method(self, GetExceptionInformation(), &record)) { \ + assert(record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR \ + || record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION); \ + if (record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { \ + NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ + ULONG code = LsaNtStatusToWinError(status); \ + PyErr_SetFromWindowsErr(code); \ + } \ + else if (record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { \ + PyErr_SetFromWindowsErr(ERROR_NOACCESS); \ + } \ + return -1; \ + } \ +} while (0) +#else +#define HANDLE_INVALID_MEM_METHOD(self, sourcecode) \ +do { \ + sourcecode \ +} while (0) +#endif + int safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { HANDLE_INVALID_MEM( @@ -351,8 +398,29 @@ safe_copy_to_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t ste return 0; } + +int +_safe_PyBytes_Find(Py_ssize_t *out, mmap_object *self, const char *haystack, + Py_ssize_t len_haystack, const char *needle, Py_ssize_t len_needle, + Py_ssize_t offset) { + HANDLE_INVALID_MEM_METHOD(self, + *out = _PyBytes_Find(haystack, len_haystack, needle, len_needle, offset); + ); + return 0; +} + +int +_safe_PyBytes_ReverseFind(Py_ssize_t *out, mmap_object *self, const char *haystack, + Py_ssize_t len_haystack, const char *needle, Py_ssize_t len_needle, + Py_ssize_t offset) { + HANDLE_INVALID_MEM_METHOD(self, + *out = _PyBytes_ReverseFind(haystack, len_haystack, needle, len_needle, offset); + ); + return 0; +} + PyObject * -_get_mmap_mem(char *start, size_t num_bytes) { +_safe_PyBytes_FromStringAndSize(char *start, size_t num_bytes) { if (num_bytes == 1) { char dest; if (safe_byte_copy(&dest, start) < 0) { @@ -417,7 +485,7 @@ mmap_read_line_method(mmap_object *self, else ++eol; /* advance past newline */ - PyObject *result = _get_mmap_mem(start, eol - start); + PyObject *result = _safe_PyBytes_FromStringAndSize(start, eol - start); if (result != NULL) { self->pos += (eol - start); } @@ -440,7 +508,7 @@ mmap_read_method(mmap_object *self, if (num_bytes < 0 || num_bytes > remaining) num_bytes = remaining; - PyObject *result = _get_mmap_mem(self->data + self->pos, num_bytes); + PyObject *result = _safe_PyBytes_FromStringAndSize(self->data + self->pos, num_bytes); if (result != NULL) { self->pos += num_bytes; } @@ -476,25 +544,36 @@ mmap_gfind(mmap_object *self, else if (end > self->size) end = self->size; - Py_ssize_t res; + Py_ssize_t index; + PyObject *result; CHECK_VALID_OR_RELEASE(NULL, view); if (end < start) { - res = -1; + result = PyLong_FromSsize_t(-1); } else if (reverse) { assert(0 <= start && start <= end && end <= self->size); - res = _PyBytes_ReverseFind( + if (_safe_PyBytes_ReverseFind(&index, self, self->data + start, end - start, - view.buf, view.len, start); + view.buf, view.len, start) < 0) { + result = NULL; + } + else { + result = PyLong_FromSsize_t(index); + } } else { assert(0 <= start && start <= end && end <= self->size); - res = _PyBytes_Find( + if (_safe_PyBytes_Find(&index, self, self->data + start, end - start, - view.buf, view.len, start); + view.buf, view.len, start) < 0) { + result = NULL; + } + else { + result = PyLong_FromSsize_t(index); + } } PyBuffer_Release(&view); - return PyLong_FromSsize_t(res); + return result; } } @@ -1184,7 +1263,7 @@ mmap_subscript(mmap_object *self, PyObject *item) if (slicelen <= 0) return PyBytes_FromStringAndSize("", 0); else if (step == 1) - return _get_mmap_mem(self->data + start, slicelen); + return _safe_PyBytes_FromStringAndSize(self->data + start, slicelen); else { char *result_buf = (char *)PyMem_Malloc(slicelen); PyObject *result; From 6aeaa32c26fc96acbf88188052f76a38b4a26536 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Tue, 7 May 2024 18:41:21 +0800 Subject: [PATCH 32/35] aesthetics --- Modules/mmapmodule.c | 49 ++++++++++++++++---------------------------- 1 file changed, 18 insertions(+), 31 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index b38205b3bd290e..269c6b821a1a8f 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -427,8 +427,7 @@ _safe_PyBytes_FromStringAndSize(char *start, size_t num_bytes) { return NULL; } else { - PyObject *result = PyBytes_FromStringAndSize(&dest, 1); - return result; + return PyBytes_FromStringAndSize(&dest, 1); } } else { @@ -456,10 +455,8 @@ mmap_read_byte_method(mmap_object *self, if (safe_byte_copy(&dest, self->data + self->pos) < 0) { return NULL; } - else { - self->pos++; - return PyLong_FromLong((unsigned char) dest); - } + self->pos++; + return PyLong_FromLong((unsigned char) dest); } static PyObject * @@ -646,15 +643,16 @@ mmap_write_method(mmap_object *self, } CHECK_VALID_OR_RELEASE(NULL, data); + PyObject *result; if (safe_memcpy(self->data + self->pos, data.buf, data.len) < 0) { - PyBuffer_Release(&data); - return NULL; + result = NULL; } else { self->pos += data.len; - PyBuffer_Release(&data); - return PyLong_FromSsize_t(data.len); + result = PyLong_FromSsize_t(data.len); } + PyBuffer_Release(&data); + return result; } static PyObject * @@ -679,10 +677,8 @@ mmap_write_byte_method(mmap_object *self, if (safe_byte_copy(self->data + self->pos, &value) < 0) { return NULL; } - else { - self->pos++; - Py_RETURN_NONE; - } + self->pos++; + Py_RETURN_NONE; } static PyObject * @@ -1221,9 +1217,7 @@ mmap_item(mmap_object *self, Py_ssize_t i) if (safe_byte_copy(&dest, self->data + i) < 0) { return NULL; } - else { - return PyBytes_FromStringAndSize(&dest, 1); - } + return PyBytes_FromStringAndSize(&dest, 1); } static PyObject * @@ -1247,9 +1241,7 @@ mmap_subscript(mmap_object *self, PyObject *item) if (safe_byte_copy(&dest, self->data + i) < 0) { return NULL; } - else { - return PyLong_FromLong(Py_CHARMASK(dest)); - } + return PyLong_FromLong(Py_CHARMASK(dest)); } else if (PySlice_Check(item)) { Py_ssize_t start, stop, step, slicelen; @@ -1272,12 +1264,11 @@ mmap_subscript(mmap_object *self, PyObject *item) return PyErr_NoMemory(); if (safe_copy_to_slice(result_buf, self->data, start, step, slicelen) < 0) { - PyMem_Free(result_buf); - return NULL; + result = NULL; + } + else { + result = PyBytes_FromStringAndSize(result_buf, slicelen); } - - result = PyBytes_FromStringAndSize(result_buf, - slicelen); PyMem_Free(result_buf); return result; } @@ -1316,9 +1307,7 @@ mmap_ass_item(mmap_object *self, Py_ssize_t i, PyObject *v) if (safe_byte_copy(self->data + i, buf) < 0) { return -1; } - else { - return 0; - } + return 0; } static int @@ -1367,9 +1356,7 @@ mmap_ass_subscript(mmap_object *self, PyObject *item, PyObject *value) if (safe_byte_copy(self->data + i, &v_char) < 0) { return -1; } - else { - return 0; - } + return 0; } else if (PySlice_Check(item)) { Py_ssize_t start, stop, step, slicelen; From 9737f222b33f7d253c38acd97b5b5a76adad6331 Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Wed, 8 May 2024 00:42:56 +0800 Subject: [PATCH 33/35] Apply suggestions from code review Co-authored-by: Eryk Sun --- Modules/mmapmodule.c | 88 ++++++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 32 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 269c6b821a1a8f..27b16bc82a06ca 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -256,28 +256,31 @@ do { \ } while (0) #endif /* UNIX */ -#if defined(MS_WIN32) && !defined(DONT_USE_SEH) +#if defined(MS_WINDOWS) && !defined(DONT_USE_SEH) static DWORD filter_page_exception(EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) { *record = *ptrs->ExceptionRecord; - if (record->ExceptionCode == EXCEPTION_IN_PAGE_ERROR - || record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { + if (record->ExceptionCode == EXCEPTION_IN_PAGE_ERROR || + record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) + { return EXCEPTION_EXECUTE_HANDLER; } return EXCEPTION_CONTINUE_SEARCH; } static DWORD -filter_page_exception_method(mmap_object *self, EXCEPTION_POINTERS *ptrs, EXCEPTION_RECORD *record) +filter_page_exception_method(mmap_object *self, EXCEPTION_POINTERS *ptrs, + EXCEPTION_RECORD *record) { *record = *ptrs->ExceptionRecord; - if (record->ExceptionCode == EXCEPTION_IN_PAGE_ERROR - || record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { + if (record->ExceptionCode == EXCEPTION_IN_PAGE_ERROR || + record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) + { ULONG_PTR address = record->ExceptionInformation[1]; - if (address >= (ULONG_PTR) self->data - && address < (ULONG_PTR) self->data + (ULONG_PTR) self->size) + if (address >= (ULONG_PTR) self->data && + address < (ULONG_PTR) self->data + (ULONG_PTR) self->size) { return EXCEPTION_EXECUTE_HANDLER; } @@ -286,7 +289,7 @@ filter_page_exception_method(mmap_object *self, EXCEPTION_POINTERS *ptrs, EXCEPT } #endif -#if defined(MS_WIN32) && !defined(DONT_USE_SEH) +#if defined(MS_WINDOWS) && !defined(DONT_USE_SEH) #define HANDLE_INVALID_MEM(sourcecode) \ do { \ EXCEPTION_RECORD record; \ @@ -294,8 +297,8 @@ do { \ sourcecode \ } \ __except (filter_page_exception(GetExceptionInformation(), &record)) { \ - assert(record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR \ - || record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION); \ + assert(record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR || \ + record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION); \ if (record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { \ NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ ULONG code = LsaNtStatusToWinError(status); \ @@ -314,7 +317,7 @@ do { \ } while (0) #endif -#if defined(MS_WIN32) && !defined(DONT_USE_SEH) +#if defined(MS_WINDOWS) && !defined(DONT_USE_SEH) #define HANDLE_INVALID_MEM_METHOD(self, sourcecode) \ do { \ EXCEPTION_RECORD record; \ @@ -322,8 +325,8 @@ do { sourcecode \ } \ __except (filter_page_exception_method(self, GetExceptionInformation(), &record)) { \ - assert(record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR \ - || record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION); \ + assert(record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR || \ + record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION); \ if (record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { \ NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ ULONG code = LsaNtStatusToWinError(status); \ @@ -343,7 +346,8 @@ do { #endif int -safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { +safe_memcpy(void *restrict dest, const void *restrict src, size_t count) +{ HANDLE_INVALID_MEM( memcpy(dest, src, count); ); @@ -351,7 +355,8 @@ safe_memcpy(void *restrict dest, const void *restrict src, size_t count) { } int -safe_byte_copy(char *dest, const char *src) { +safe_byte_copy(char *dest, const char *src) +{ HANDLE_INVALID_MEM( *dest = *src; ); @@ -359,7 +364,8 @@ safe_byte_copy(char *dest, const char *src) { } int -safe_memchr(char **out, const void *ptr, int ch, size_t count) { +safe_memchr(char **out, const void *ptr, int ch, size_t count) +{ HANDLE_INVALID_MEM( *out = (char *) memchr(ptr, ch, count); ); @@ -367,7 +373,8 @@ safe_memchr(char **out, const void *ptr, int ch, size_t count) { } int -safe_memmove(void *dest, const void *src, size_t count) { +safe_memmove(void *dest, const void *src, size_t count) +{ HANDLE_INVALID_MEM( memmove(dest, src, count); ); @@ -375,7 +382,9 @@ safe_memmove(void *dest, const void *src, size_t count) { } int -safe_copy_from_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t step, Py_ssize_t slicelen) { +safe_copy_from_slice(char *dest, const char *src, Py_ssize_t start, + Py_ssize_t step, Py_ssize_t slicelen) +{ HANDLE_INVALID_MEM( size_t cur; Py_ssize_t i; @@ -387,7 +396,9 @@ safe_copy_from_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t s } int -safe_copy_to_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t step, Py_ssize_t slicelen) { +safe_copy_to_slice(char *dest, const char *src, Py_ssize_t start, + Py_ssize_t step, Py_ssize_t slicelen) +{ HANDLE_INVALID_MEM( size_t cur; Py_ssize_t i; @@ -401,8 +412,9 @@ safe_copy_to_slice(char *dest, const char *src, Py_ssize_t start, Py_ssize_t ste int _safe_PyBytes_Find(Py_ssize_t *out, mmap_object *self, const char *haystack, - Py_ssize_t len_haystack, const char *needle, Py_ssize_t len_needle, - Py_ssize_t offset) { + Py_ssize_t len_haystack, const char *needle, + Py_ssize_t len_needle, Py_ssize_t offset) +{ HANDLE_INVALID_MEM_METHOD(self, *out = _PyBytes_Find(haystack, len_haystack, needle, len_needle, offset); ); @@ -410,11 +422,14 @@ _safe_PyBytes_Find(Py_ssize_t *out, mmap_object *self, const char *haystack, } int -_safe_PyBytes_ReverseFind(Py_ssize_t *out, mmap_object *self, const char *haystack, - Py_ssize_t len_haystack, const char *needle, Py_ssize_t len_needle, - Py_ssize_t offset) { +_safe_PyBytes_ReverseFind(Py_ssize_t *out, mmap_object *self, + const char *haystack, Py_ssize_t len_haystack, + const char *needle, Py_ssize_t len_needle, + Py_ssize_t offset) +{ HANDLE_INVALID_MEM_METHOD(self, - *out = _PyBytes_ReverseFind(haystack, len_haystack, needle, len_needle, offset); + *out = _PyBytes_ReverseFind(haystack, len_haystack, needle, len_needle, + offset); ); return 0; } @@ -505,7 +520,8 @@ mmap_read_method(mmap_object *self, if (num_bytes < 0 || num_bytes > remaining) num_bytes = remaining; - PyObject *result = _safe_PyBytes_FromStringAndSize(self->data + self->pos, num_bytes); + PyObject *result = _safe_PyBytes_FromStringAndSize(self->data + self->pos, + num_bytes); if (result != NULL) { self->pos += num_bytes; } @@ -551,7 +567,8 @@ mmap_gfind(mmap_object *self, assert(0 <= start && start <= end && end <= self->size); if (_safe_PyBytes_ReverseFind(&index, self, self->data + start, end - start, - view.buf, view.len, start) < 0) { + view.buf, view.len, start) < 0) + { result = NULL; } else { @@ -562,7 +579,8 @@ mmap_gfind(mmap_object *self, assert(0 <= start && start <= end && end <= self->size); if (_safe_PyBytes_Find(&index, self, self->data + start, end - start, - view.buf, view.len, start) < 0) { + view.buf, view.len, start) < 0) + { result = NULL; } else { @@ -1087,7 +1105,9 @@ mmap_protect_method(mmap_object *self, PyObject *args) { return NULL; } - if (VirtualProtect((void *) (self->data + start), length, flNewProtect, &flOldProtect) == 0) { + if (!VirtualProtect((void *) (self->data + start), length, flNewProtect, + &flOldProtect)) + { PyErr_SetFromWindowsErr(GetLastError()); return NULL; } @@ -1263,7 +1283,9 @@ mmap_subscript(mmap_object *self, PyObject *item) if (result_buf == NULL) return PyErr_NoMemory(); - if (safe_copy_to_slice(result_buf, self->data, start, step, slicelen) < 0) { + if (safe_copy_to_slice(result_buf, self->data, start, step, + slicelen) < 0) + { result = NULL; } else { @@ -1390,7 +1412,9 @@ mmap_ass_subscript(mmap_object *self, PyObject *item, PyObject *value) } } else { - if (safe_copy_from_slice(self->data, (char *)vbuf.buf, start, step, slicelen) < 0) { + if (safe_copy_from_slice(self->data, (char *)vbuf.buf, start, step, + slicelen) < 0) + { result = -1; } } From 12fb5610489f3d47fd257af83309d34e8d11750d Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Wed, 8 May 2024 08:30:29 +0800 Subject: [PATCH 34/35] remove restrict for compliance with the python C dialect --- Modules/mmapmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index 27b16bc82a06ca..ea239305442d5e 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -346,7 +346,7 @@ do { #endif int -safe_memcpy(void *restrict dest, const void *restrict src, size_t count) +safe_memcpy(void *dest, const void *src, size_t count) { HANDLE_INVALID_MEM( memcpy(dest, src, count); From 880a9c058e968a8d96a4a6df5739bc822f7b659f Mon Sep 17 00:00:00 2001 From: Dobatymo Date: Wed, 8 May 2024 08:30:59 +0800 Subject: [PATCH 35/35] Update Modules/mmapmodule.c Co-authored-by: Eryk Sun --- Modules/mmapmodule.c | 45 ++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/Modules/mmapmodule.c b/Modules/mmapmodule.c index ea239305442d5e..d0402e2a6bcbb4 100644 --- a/Modules/mmapmodule.c +++ b/Modules/mmapmodule.c @@ -318,30 +318,31 @@ do { \ #endif #if defined(MS_WINDOWS) && !defined(DONT_USE_SEH) -#define HANDLE_INVALID_MEM_METHOD(self, sourcecode) \ -do { \ - EXCEPTION_RECORD record; \ - __try { \ - sourcecode \ - } \ - __except (filter_page_exception_method(self, GetExceptionInformation(), &record)) { \ - assert(record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR || \ - record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION); \ - if (record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { \ - NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ - ULONG code = LsaNtStatusToWinError(status); \ - PyErr_SetFromWindowsErr(code); \ - } \ - else if (record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { \ - PyErr_SetFromWindowsErr(ERROR_NOACCESS); \ - } \ - return -1; \ - } \ +#define HANDLE_INVALID_MEM_METHOD(self, sourcecode) \ +do { \ + EXCEPTION_RECORD record; \ + __try { \ + sourcecode \ + } \ + __except (filter_page_exception_method(self, GetExceptionInformation(), \ + &record)) { \ + assert(record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR || \ + record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION); \ + if (record.ExceptionCode == EXCEPTION_IN_PAGE_ERROR) { \ + NTSTATUS status = (NTSTATUS) record.ExceptionInformation[2]; \ + ULONG code = LsaNtStatusToWinError(status); \ + PyErr_SetFromWindowsErr(code); \ + } \ + else if (record.ExceptionCode == EXCEPTION_ACCESS_VIOLATION) { \ + PyErr_SetFromWindowsErr(ERROR_NOACCESS); \ + } \ + return -1; \ + } \ } while (0) #else -#define HANDLE_INVALID_MEM_METHOD(self, sourcecode) \ -do { \ - sourcecode \ +#define HANDLE_INVALID_MEM_METHOD(self, sourcecode) \ +do { \ + sourcecode \ } while (0) #endif