← 返回首页
gh-103987: fix crash in mmap module by Agent-Hellboy · Pull Request #103990 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

gh-103987: fix crash in mmap module#103990

Merged
JelleZijlstra merged 39 commits into
python:mainfrom
Agent-Hellboy:fix-issue-103987
May 20, 2023
Merged

gh-103987: fix crash in mmap module#103990
JelleZijlstra merged 39 commits into
python:mainfrom
Agent-Hellboy:fix-issue-103987

Conversation

Agent-Hellboy commented Apr 29, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

bedevere-bot mentioned this pull request Apr 29, 2023
Closed
arhadthedev added the extension-modules C modules in the Modules dir label Apr 29, 2023
arhadthedev added the needs backport to 3.11 only security fixes label Apr 29, 2023

Agent-Hellboy commented Apr 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I am closing the mmap object, still the test is failing

Eclips4 reviewed Apr 29, 2023
View reviewed changes
Comment thread Lib/test/test_mmap.py Outdated Show resolved Hide resolved
Eclips4 reviewed Apr 29, 2023
View reviewed changes
Comment thread Lib/test/test_mmap.py Outdated Show resolved Hide resolved

Eclips4 commented Apr 29, 2023

Copy link
Copy Markdown
Member

Let's wait CI/CD results. Also, can you add a few lines about this in NEWS entry?

Eclips4 commented Apr 29, 2023

Copy link
Copy Markdown
Member

Great work @Agent-Hellboy !
Let's see what core devs will say.

cfbolz commented Apr 29, 2023

Copy link
Copy Markdown
Contributor

still some cases not covered properly. Now that you moved the CHECK_VALID(NULL); into the if-branch, you don't check whether the mmap is closed any more if the index is a slice. try adding a test like:

m = mmap(...) m.close() m[0:5] # should complain about a closed file

also you should add a test that uses the weird X class as part of a slice

m[X():5]

and you need to do the same thing again for assignment, both with X as an index and a slice:

m[X()] = 1 ... m[X():5] = b'abcde'

or something like that.

Also there needs to be a test for the special case I mentioned at the end of the issue:

m = mmap(...) m.close() with self.assertRaises(ValueError): m["abc"] # not TypeError!

Agent-Hellboy commented Apr 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

hi @cfbolz, Now I am checking at the start as well which denies throwing TypeError for closed mmap

cfbolz commented Apr 29, 2023

Copy link
Copy Markdown
Contributor

now you need to do the same things for item assignments still.

sunmy2019 requested changes Apr 29, 2023
View reviewed changes
Comment thread Modules/mmapmodule.c Outdated Show resolved Hide resolved

sunmy2019 commented Apr 29, 2023

Copy link
Copy Markdown
Member

We should CHECK_VALID(NULL); before each self->data access, unless we have strong proof that our handle is not closed.

sunmy2019 commented Apr 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

@Agent-Hellboy

I am saying you should add these

else if (PySlice_Check(item)) { Py_ssize_t start, stop, step, slicelen; if (PySlice_Unpack(item, &start, &stop, &step) < 0) { return NULL; } CHECK_VALID(NULL); ///////////////////////////////////////////////////////////////// slicelen = PySlice_AdjustIndices(self->size, &start, &stop, step); if (slicelen <= 0) return PyBytes_FromStringAndSize("", 0); else if (step == 1) return PyBytes_FromStringAndSize(self->data + start, 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(); CHECK_VALID(NULL); ///////////////////////////////////////////////////////////////// for (cur = start, i = 0; i < slicelen; cur += step, i++) { result_buf[i] = self->data[cur]; } result = PyBytes_FromStringAndSize(result_buf, slicelen); PyMem_Free(result_buf); return result; } } else { PyErr_SetString(PyExc_TypeError, "mmap indices must be integers"); return NULL; } }

Agent-Hellboy commented Apr 30, 2023

Copy link
Copy Markdown
Contributor Author

Hi @sunmy2019, please share with me a scenario where it's bypassing the first CHECK_VALID(NULL); at the start for a slice

sunmy2019 commented Apr 30, 2023

Copy link
Copy Markdown
Member

Hi @sunmy2019, please share with me a scenario where it's bypassing the first CHECK_VALID(NULL); at the start for a slice

m[X():5]

Agent-Hellboy commented Apr 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Currently, it's throwing expected Value error, i have added a test for the same

sunmy2019 requested changes Apr 30, 2023
View reviewed changes
Comment thread Lib/test/test_mmap.py Outdated Show resolved Hide resolved

sunmy2019 commented Apr 30, 2023

Copy link
Copy Markdown
Member

Currently, it's throwing expected Value error

This is wrong. And you are not creating a new m, which covered your mistake.

33 hidden items Load more…
sunmy2019 reviewed May 12, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-04-29-18-23-16.gh-issue-103987.sRgALL.rst Outdated Show resolved Hide resolved
sunmy2019 reviewed May 12, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-04-29-18-23-16.gh-issue-103987.sRgALL.rst Outdated Show resolved Hide resolved
JelleZijlstra reviewed May 19, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-04-29-18-23-16.gh-issue-103987.sRgALL.rst Outdated Show resolved Hide resolved

sunmy2019 commented May 19, 2023

Copy link
Copy Markdown
Member

then we access self->fd

I think checking self->fd != -1 is enough here.

sunmy2019 commented May 19, 2023

Copy link
Copy Markdown
Member

I think it's now ready for review. @JelleZijlstra @cfbolz

Sorry for the delay!

JelleZijlstra self-requested a review May 19, 2023 20:48
JelleZijlstra reviewed May 19, 2023
View reviewed changes
Comment thread Lib/test/test_mmap.py Outdated Show resolved Hide resolved
Comment thread Modules/mmapmodule.c

if (result_buf == NULL)
return PyErr_NoMemory();

JelleZijlstra May 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Could the buffer have been invalidated here as a side effect of the PyMem_Malloc call above triggering GC? I seem to recall that's possible, but not 100% sure.

sunmy2019 May 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I cannot find any GC-related code in Object/obmalloc.c

Update Lib/test/test_mmap.py …
320feac
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra merged commit ceaa4c3 into python:main May 20, 2023

miss-islington commented May 20, 2023

Copy link
Copy Markdown
Contributor

Thanks @Agent-Hellboy for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot commented May 20, 2023

Copy link
Copy Markdown

GH-104677 is a backport of this pull request to the 3.11 branch.

bedevere-bot removed the needs backport to 3.11 only security fixes label May 20, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 20, 2023
pythongh-103987: fix several crashes in mmap module (pythonGH-103990) …
7f358a0
(cherry picked from commit ceaa4c3) Co-authored-by: Prince Roshan <princekrroshan01@gmail.com> Co-authored-by: sunmy2019 <59365878+sunmy2019@users.noreply.github.com> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra added a commit that referenced this pull request May 20, 2023
[3.11] gh-103987: fix several crashes in mmap module (GH-103990) (#10… …
dbb011a
…4677) gh-103987: fix several crashes in mmap module (GH-103990) (cherry picked from commit ceaa4c3) Co-authored-by: Prince Roshan <princekrroshan01@gmail.com> Co-authored-by: sunmy2019 <59365878+sunmy2019@users.noreply.github.com> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
carljm added a commit to gsallam/cpython_with_perfmap_apii that referenced this pull request May 20, 2023
Merge branch 'main' into gsallam/main …
ea5965a
* main: (30 commits) pythongh-103987: fix several crashes in mmap module (python#103990) docs: fix wrong indentation causing rendering error in dis page (python#104661) pythongh-94906: Support multiple steps in math.nextafter (python#103881) pythongh-104472: Skip `test_subprocess.ProcessTestCase.test_empty_env` if ASAN is enabled (python#104667) pythongh-103839: Allow building Tkinter against Tcl 8.7 without external libtommath (pythonGH-103842) pythongh-85984: New additions and improvements to the tty library. (python#101832) pythongh-104659: Consolidate python examples in enum documentation (python#104665) pythongh-92248: Deprecate `type`, `choices`, `metavar` parameters of `argparse.BooleanOptionalAction` (python#103678) pythongh-104645: fix error handling in marshal tests (python#104646) pythongh-104600: Make type.__type_params__ writable (python#104634) pythongh-104602: Add additional test for listcomp with lambda (python#104639) pythongh-104640: Disallow walrus in comprehension within type scopes (python#104641) pythongh-103921: Rename "type" header in argparse docs (python#104654) Improve readability of `typing._ProtocolMeta.__instancecheck__` (python#104649) pythongh-96522: Fix deadlock in pty.spawn (python#96639) pythonGH-102818: Do not call `PyTraceBack_Here` in sys.settrace trampoline. (pythonGH-104579) pythonGH-103545: Add macOS specific constants for ``os.setpriority`` to ``os`` (python#104606) pythongh-104623: Update macOS installer to SQLite 3.42.0 (pythonGH-104624) pythongh-104619: never leak comprehension locals to outer locals() (python#104637) pythongh-104602: ensure all cellvars are known up front (python#104603) ...
Eclips4 mentioned this pull request May 20, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JelleZijlstra JelleZijlstra left review comments
sunmy2019 sunmy2019 requested changes
Eclips4 Eclips4 left review comments
markshannon Awaiting requested review from markshannon

Assignees

No one assigned

Labels

extension-modules C modules in the Modules dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Footer

© 2026 GitHub, Inc.