GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-118074: Executors in the COLD_EXITS array are not GC'able by gvanrossum · Pull Request #118117 · python/cpython

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Apr 20, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Implement a tp_is_gc slot that tests for this.

  • Issue: global-buffer-overflow in test_opt.py #118074

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gvanrossum
Executors in the COLD_EXITS array are not GC'able
f2e10eb 
Implement a `tp_is_gc` slot that tests for this.
@gvanrossum
gvanrossum requested a review from markshannon April 20, 2024 01:47
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 20, 2024
@gvanrossum

gvanrossum commented Apr 20, 2024

Copy link
Copy Markdown
Member Author

@adoxalim Can you verify that this fix works for you? (Thanks again for the bug report.)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
Fix address arithmetic bug
e748e8a
mdboom
mdboom reviewed Apr 22, 2024
View reviewed changes
Comment thread Python/optimizer.c Outdated
static int
executor_is_gc(PyObject *o)
{
if ((PyObject *)&COLD_EXITS[0] <= o && o < (PyObject *)&COLD_EXITS[UOP_MAX_TRACE_LENGTH]) {

@mdboom mdboom Apr 22, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The address sanitizer warning here seems plausibly correct:

array subscript 800 is above array bounds of ‘_PyExecutorObject[200]’ [-Warray-bounds]

COLD_EXITS has a length of UOP_MAX_TRACE_LENGTH/4. If there's some other reason this is correct, maybe add a comment / a way to suppress the warning?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Apr 22, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oh, you're right. That line was written by Copilot and it looked so plausible but was so wrong. :-(

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gvanrossum added 2 commits April 22, 2024 08:32
@gvanrossum
Use correct COLD_EXITS upper bound
bb71ae9
@gvanrossum
Add blurb
1653fe5
@gvanrossum

gvanrossum commented Apr 22, 2024

Copy link
Copy Markdown
Member Author

The emulated Linux (ARM64) test failures are known, caused by the the QEMU emulation. I'm merging now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
gvanrossum merged commit 1b85b34 into python:main Apr 22, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Apr 22, 2024
@gvanrossum
gvanrossum deleted the fix-cold-exit-crash branch April 22, 2024 23:20
markshannon
markshannon reviewed Apr 23, 2024
View reviewed changes
Comment thread Python/optimizer.c
static int
executor_is_gc(PyObject *o)
{
if ((PyObject *)&COLD_EXITS[0] <= o && o < (PyObject *)&COLD_EXITS[COLD_EXIT_COUNT]) {

@markshannon markshannon Apr 23, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is undefined behavior in C. Although it will mostly work in practice.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Apr 23, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

So I take it this should work?

Suggested change
if ((PyObject *)&COLD_EXITS[0] <= o && o < (PyObject *)&COLD_EXITS[COLD_EXIT_COUNT]) {
if (_Py_IsImmortal(o)) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Apr 23, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Apr 23, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe add a similar thing to the top of executor_traverse()? (Although, really, cold exits don't have exits, so their exit_count should always be zero, right? So why bother.

Anyway, see gh-118182 for a fix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon

markshannon commented Apr 23, 2024

Copy link
Copy Markdown
Member

Sorry for not reviewing this earlier.

I'd rather not use tp_is_gc, as it adds quite a bit of overhead to an already slow process.

As a short term fix, we could change tp_traverse to return immediately if the executor is immortal.
Longer term, we should fix this in the GC. The GC shouldn't be visiting immortal objects.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum gvanrossum mentioned this pull request Apr 23, 2024
gh-118074: Immortal executors are not GC-able #118182
Merged
gvanrossum added a commit that referenced this pull request Apr 23, 2024
@gvanrossum
gh-118074: Immortal executors are not GC-able (#118182)
7e87d30 
Better version of gh-118117.
Just check for immortality instead of an address range check.
szef1233
szef1233 approved these changes May 11, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added the awaiting core review label May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdboom mdboom mdboom left review comments
@markshannon markshannon markshannon left review comments
+1 more reviewer
@szef1233 szef1233 szef1233 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@gvanrossum @markshannon @mdboom @szef1233
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.