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-67877: Fix memory leaks in terminated RE matching by serhiy-storchaka · Pull Request #126840 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Nov 14, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

If SRE(match) function terminates abruptly, either because of a signal or because memory allocation fails, allocated SRE_REPEAT blocks might be never released.

  • Issue: Memory leak in Modules/sre_lib.h #67877

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
pythongh-67877: Fix memory leaks in terminated RE matching
541de35 
If SRE(match) function terminates abruptly, either because of a signal
or because memory allocation fails, allocated SRE_REPEAT blocks might
be never released.
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Nov 14, 2024
@bedevere-app bedevere-app Bot added the awaiting core review label Nov 14, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Nov 14, 2024
Memory leak in Modules/sre_lib.h #67877
Closed
@serhiy-storchaka

serhiy-storchaka commented Nov 14, 2024

Copy link
Copy Markdown
Member Author

See also #126843.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka serhiy-storchaka mentioned this pull request Nov 14, 2024
gh-67877: Fix memory leaks in terminated RE matching #126843
Closed
@serhiy-storchaka
Use a pool for SRE_REPEAT.
41e60ff
wjssz
wjssz reviewed Nov 16, 2024
View reviewed changes
Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sre/sre.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
Clean up.
c76a917
@wjssz

wjssz commented Nov 16, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Looks good to me.
You may also remove the outdated SRE_ERROR_RECURSION_LIMIT at the same time, or remove it later.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka added 2 commits November 16, 2024 11:54
@serhiy-storchaka
Set state->fail_after_count = -1 after triggering an exception.
3b4e81a
@serhiy-storchaka
Merge branch 'main' into re-memleaks
7b4797a
serhiy-storchaka
serhiy-storchaka commented Nov 16, 2024
View reviewed changes
Comment thread Modules/_sre/sre_lib.h
do { \
_MAYBE_CHECK_SIGNALS; \
if (state->fail_after_count >= 0) { \
if (state->fail_after_count-- == 0) { \

@serhiy-storchaka serhiy-storchaka Nov 16, 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.

Setting state->fail_after_count does not affect subsequent calls of the Pattern methods, but it still affects the finditer() iterator which reuses state between iterations.

Sorry, something went wrong.

Uh oh!

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

@wjssz wjssz Nov 16, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

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.

I deleted my previous reply.
Maybe setting state->fail_after_count = -1 doesn't make sense, it's better to make the finditer() iterator stop the subsequent matchings.

Sorry, something went wrong.

Uh oh!

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

wjssz commented Nov 16, 2024

Copy link
Copy Markdown

I remember another thing, if this patch is going to be backported, you may remove the changes related to this:

-    PyObject* pattern;	
+    PatternObject* pattern;

The patch will be smaller and clearer.
Polishing work can be done only on the main branch, such as change the type, remove the outdated SRE_ERROR_RECURSION_LIMIT.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka merged commit 7538e7f into python:main Nov 18, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Nov 18, 2024
@miss-islington-app

miss-islington-app Bot commented Nov 18, 2024

Copy link
Copy Markdown

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka deleted the re-memleaks branch November 18, 2024 09:53
@miss-islington-app

miss-islington-app Bot commented Nov 18, 2024

Copy link
Copy Markdown

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 7538e7f5696408fa0aa02fce8a413a7dfac76a04 3.13
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned serhiy-storchaka Nov 18, 2024
@miss-islington-app

miss-islington-app Bot commented Nov 18, 2024

Copy link
Copy Markdown

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 7538e7f5696408fa0aa02fce8a413a7dfac76a04 3.12
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Nov 18, 2024
@serhiy-storchaka
pythongh-67877: Fix memory leaks in terminated RE matching (pythonGH-…
9320415 
…126840)

If SRE(match) function terminates abruptly, either because of a signal
or because memory allocation fails, allocated SRE_REPEAT blocks might
be never released.

Co-authored-by: <[email protected]>
(cherry picked from commit 7538e7f)
@bedevere-app

bedevere-app Bot commented Nov 18, 2024

Copy link
Copy Markdown

GH-126960 is a backport of this pull request to the 3.13 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Nov 18, 2024
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Nov 18, 2024
@serhiy-storchaka
[3.12] pythongh-67877: Fix memory leaks in terminated RE matching (py…
5b1b787 
…thonGH-126840)

If SRE(match) function terminates abruptly, either because of a signal
or because memory allocation fails, allocated SRE_REPEAT blocks might
be never released.

(cherry picked from commit 7538e7f)

Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: <[email protected]>
@bedevere-app

bedevere-app Bot commented Nov 18, 2024

Copy link
Copy Markdown

GH-126961 is a backport of this pull request to the 3.12 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Nov 18, 2024
@serhiy-storchaka serhiy-storchaka removed their assignment Nov 18, 2024
@serhiy-storchaka

serhiy-storchaka commented Nov 18, 2024

Copy link
Copy Markdown
Member Author

I remember another thing, if this patch is going to be backported, you may remove the changes related to this:

It is easier to not do. I can introduce new bugs when trying to remove these changes in backports.

Initially it was needed to pass the second argument to state_fini, after reverting that change it is just a clean up, but it can still be helpful if in future we will need to pass the second argument to state_fini.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka added a commit that referenced this pull request Nov 18, 2024
@serhiy-storchaka
[3.12] gh-67877: Fix memory leaks in terminated RE matching (GH-126840)…
8eb25fa 
… (GH-126961)

If SRE(match) function terminates abruptly, either because of a signal
or because memory allocation fails, allocated SRE_REPEAT blocks might
be never released.

(cherry picked from commit 7538e7f)
serhiy-storchaka added a commit that referenced this pull request Nov 18, 2024
@serhiy-storchaka
[3.13] gh-67877: Fix memory leaks in terminated RE matching (GH-126840)…
885386b 
… (GH-126960)

If SRE(match) function terminates abruptly, either because of a signal
or because memory allocation fails, allocated SRE_REPEAT blocks might
be never released.

Co-authored-by: <[email protected]>
(cherry picked from commit 7538e7f)
@wjssz

wjssz commented Nov 19, 2024

Copy link
Copy Markdown

sre only has the “\B can't match empty string” bug as far as I know. It's easy to fix, if decide to fix it.
Maybe people won't touch sre's code for many years.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@serhiy-storchaka @ebonnal
pythongh-67877: Fix memory leaks in terminated RE matching (pythonGH-…
32e3afa 
…126840)

If SRE(match) function terminates abruptly, either because of a signal
or because memory allocation fails, allocated SRE_REPEAT blocks might
be never released.

Co-authored-by: <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@wjssz wjssz wjssz left review comments
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

None yet

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.

2 participants

@serhiy-storchaka @wjssz
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.