Skip to content

gh-106524: Zero template items before initializing them in _sre.template#106525

Merged
serhiy-storchaka merged 4 commits intopython:mainfrom
chgnrdv:sre-template-invalid-group-index
Jul 8, 2023
Merged

gh-106524: Zero template items before initializing them in _sre.template#106525
serhiy-storchaka merged 4 commits intopython:mainfrom
chgnrdv:sre-template-invalid-group-index

Conversation

@chgnrdv
Copy link
Contributor

@chgnrdv chgnrdv commented Jul 7, 2023 •
edited by bedevere-bot
Loading

Fixes #106524

  • made _sre_template_impl set items of 'self->items' array to zero before initializing them with group indices and literals
  • added test

@chgnrdv
gh-106524: Zero template items before initializing them in _sre.template
816432c 
* made `_sre_template_impl` set items of 'self->items' array to zero before initializing them with group indices and literals
* added test
@bedevere-bot bedevere-bot mentioned this pull request Jul 7, 2023
Closed
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jul 7, 2023

Good catch. Other approach is to call Py_SET_SIZE(self, i) on an error during initialization. It may have less impact on performance. But how large this impact in the first place? If it is insignificant, memset() will do.

@chgnrdv
Copy link
Contributor Author

chgnrdv commented Jul 7, 2023

@serhiy-storchaka, benchmarks speak for themselves:

./python -m pyperf timeit -s 'import _sre' '_sre.template("", ["", 1, "", 1, "", 1, "", 1, ""])'

main:
Mean +- std dev: 480 ns +- 2 ns

with memset:
Mean +- std dev: 523 ns +- 7 ns

with Py_SET_SIZE:
Mean +- std dev: 474 ns +- 2 ns

@serhiy-storchaka serhiy-storchaka merged commit 2ef1dc3 into python:main Jul 8, 2023
@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jul 8, 2023
@miss-islington
Copy link
Contributor

miss-islington commented Jul 8, 2023

Thanks @chgnrdv for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Jul 8, 2023

Thanks @chgnrdv for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Jul 8, 2023

Sorry @chgnrdv and @serhiy-storchaka, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 2ef1dc37f02b08536b677dd23ec51541a60effd7 3.11

@bedevere-bot
Copy link

bedevere-bot commented Jul 8, 2023

GH-106544 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jul 8, 2023
@serhiy-storchaka serhiy-storchaka removed the needs backport to 3.11 only security fixes label Jul 8, 2023
serhiy-storchaka pushed a commit that referenced this pull request Jul 8, 2023
@miss-islington @chgnrdv
[3.12] gh-106524: Fix a crash in _sre.template() (GH-106525) (GH-106544)
8fdb058 
Some items remained uninitialized if _sre.template() was called with invalid
indices. Then attempt to clear them in the destructor led to dereferencing
of uninitialized pointer.
(cherry picked from commit 2ef1dc3)

Co-authored-by: Radislav Chugunov <52372310+chgnrdv@users.noreply.github.com>
@serhiy-storchaka serhiy-storchaka removed their assignment Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

_sre.template crashes in case of negative or non-integer group index

4 participants

@chgnrdv @serhiy-storchaka @miss-islington @bedevere-bot