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-145376: Fix null pointer deref in md5module.c by eendebakpt · Pull Request #145422 · python/cpython

Conversation

@eendebakpt

@eendebakpt eendebakpt commented Mar 2, 2026 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Avoid a null pointer deref in the case of an error path in the constructors (e.g. MD5Type_copy_impl)

Issue found using Claude.

  • Issue: Various reference leaks in the core #145376

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
eendebakpt added 2 commits March 2, 2026 12:02
@eendebakpt
Fix null pointer deref in md5module.c
d5b9513
@eendebakpt
pep7
6cc205c
@eendebakpt
eendebakpt requested review from gpshead and picnixz as code owners March 2, 2026 14:47
@bedevere-app bedevere-app Bot mentioned this pull request Mar 2, 2026
Various reference leaks in the core #145376
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Mar 2, 2026
picnixz
picnixz reviewed Mar 2, 2026
View reviewed changes

@picnixz picnixz left a comment

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.

I think this pattern is also present in SHA-* and other modules as it's essentially C/C a b it everywhere.

Sorry, something went wrong.

Uh oh!

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

eendebakpt commented Mar 2, 2026

Copy link
Copy Markdown
Contributor Author

I think this pattern is also present in SHA-* and other modules as it's essentially C/C a b it everywhere.

Not sure what C/C a b is, but the pattern is indeed the same. E.g.

cpython/Modules/sha1module.c

Line 89 in 02288bf

if (ptr->hash_state != NULL) {

There the hash_state is also set to NULL, not sure why that is.
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Mar 2, 2026

Copy link
Copy Markdown
Member

C/C a b is

it was meant to be a "C/C a bit" and I used C/C for "carbon copy" (which is essentially to mean "copy-paste" for me)

There the hash_state is also set to NULL, not sure why that is.

Likely to prevent a double-free (I don't know if it was me or tiran/gpshead who added this)
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Mar 2, 2026

Copy link
Copy Markdown
Member

Actually it was me in 261633b. I just forgot about the MD5 one I think.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz previously approved these changes Mar 2, 2026
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Mar 2, 2026
@picnixz

picnixz commented Mar 2, 2026

Copy link
Copy Markdown
Member
  1. Please add a small NEWS entry.
  2. If you find other issues in MD5, you can include them here as well. Same for SHA-*, BLAKE2 and HMAC.
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Mar 2, 2026

Copy link
Copy Markdown
Member

Oh and please also put the state to NULL just to prevent a possible double-free
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
picnixz dismissed their stale review March 2, 2026 18:10

Hit the wrong button

@bedevere-app bedevere-app Bot added awaiting review and removed awaiting merge labels Mar 2, 2026
eendebakpt and others added 6 commits March 2, 2026 20:37
@eendebakpt
review comments
330128f
@blurb-it
📜🤖 Added by blurb_it.
aae8d22
@eendebakpt
Merge branch 'null_md5module' of github.com:eendebakpt/cpython into n…
c696867 
…ull_md5module
@eendebakpt
Merge branch 'main' into null_md5module
eafcc2d
@eendebakpt
Merge branch 'null_md5module' of github.com:eendebakpt/cpython into n…
6b4cbff 
…ull_md5module
@eendebakpt
fix
3aa0033
picnixz
picnixz reviewed Mar 2, 2026
View reviewed changes
Comment thread Modules/hmacmodule.c Outdated
int rc = py_hmac_hinfo_ht_add(table, KEY, value); \
if (rc < 0) { \
PyMem_Free(value); \
if (value->refcnt == 0) { \

@picnixz picnixz Mar 2, 2026

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 one is already part of an other PR actually.

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz Mar 2, 2026

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.

Namely: #145321.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Mar 4, 2026

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.

I would prefer merging PR gh-145321 first since it's older and more complete.

Sorry, something went wrong.

Uh oh!

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

@eendebakpt eendebakpt Mar 4, 2026

Copy link
Copy Markdown
Contributor 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.

Agreed. I am waiting for the other PR and will rebase accordingly.

Sorry, something went wrong.

Uh oh!

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

@gpshead gpshead Mar 5, 2026

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.

i just merged the above.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed Mar 4, 2026
View reviewed changes

@vstinner vstinner left a comment

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.

The md5 change LGTM. I'm not sure that it's worth it to add a NEWS entry for this change, since it's unlikely to hit this bug in practice.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/hmacmodule.c Outdated
int rc = py_hmac_hinfo_ht_add(table, KEY, value); \
if (rc < 0) { \
PyMem_Free(value); \
if (value->refcnt == 0) { \

@vstinner vstinner Mar 4, 2026

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.

I would prefer merging PR gh-145321 first since it's older and more complete.

Sorry, something went wrong.

Uh oh!

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

picnixz commented Mar 5, 2026

Copy link
Copy Markdown
Member

I prefer to always include a NEWS entry for crashes even if they are rare.
vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@eendebakpt
Merge branch 'main' into null_md5module
fe78d49
@eendebakpt
eendebakpt marked this pull request as draft March 5, 2026 09:57
@bedevere-app bedevere-app Bot removed the awaiting review label Mar 5, 2026
vstinner
vstinner approved these changes Mar 5, 2026
View reviewed changes

@vstinner vstinner left a comment

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.

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2026-03-02-19-41-39.gh-issue-145376.OOzSOh.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting merge label Mar 5, 2026
@eendebakpt @vstinner
Apply suggestion from @vstinner
4ae295b 
Co-authored-by: Victor Stinner <[email protected]>
picnixz
picnixz reviewed Mar 5, 2026
View reviewed changes
Comment thread Modules/hmacmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@eendebakpt @picnixz
Apply suggestion from @picnixz
3e79541 
Co-authored-by: Bénédikt Tran <[email protected]>
eendebakpt
eendebakpt commented Mar 5, 2026
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2026-03-02-19-41-39.gh-issue-145376.OOzSOh.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
eendebakpt added 2 commits March 5, 2026 12:53
@eendebakpt
Apply suggestion from @eendebakpt
1fa2e83
@eendebakpt
Merge branch 'main' into null_md5module
46b2511
@eendebakpt
eendebakpt marked this pull request as ready for review March 5, 2026 12:22
@bedevere-app bedevere-app Bot added awaiting review and removed awaiting merge labels Mar 5, 2026
picnixz
picnixz approved these changes Mar 5, 2026
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Mar 5, 2026
@picnixz
Merge branch 'main' into null_md5module
93f1de5
@picnixz
picnixz enabled auto-merge (squash) March 6, 2026 19:38
@picnixz

picnixz commented Mar 6, 2026

Copy link
Copy Markdown
Member

@eendebakpt Thanks for the PR! Once it's merged, could you do a manual backport for 3.13 and an automatic backport for 3.14 (though I'm not entirely sure about conflicts in md5module.c)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
picnixz merged commit c1d7768 into python:main Mar 6, 2026
50 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 6, 2026
@eendebakpt eendebakpt added the needs backport to 3.14 bugs and security fixes label Mar 6, 2026
@miss-islington-app

miss-islington-app Bot commented Mar 6, 2026

Copy link
Copy Markdown

Thanks @eendebakpt for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 6, 2026
@eendebakpt @miss-islington
pythongh-145376: Fix crashes in md5module.c and hmacmodule.c (pyt…
bbc0db7 
…honGH-145422)

Fix a possible NULL pointer dereference in `md5module.c` and a double-free in `hmacmodule.c`.
Those crashes only occur in error paths taken when the interpreter fails to allocate memory.
(cherry picked from commit c1d7768)

Co-authored-by: Pieter Eendebak <[email protected]>
@bedevere-app

bedevere-app Bot commented Mar 6, 2026

Copy link
Copy Markdown

GH-145610 is a backport of this pull request to the 3.14 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.14 bugs and security fixes label Mar 6, 2026
@bedevere-app

bedevere-app Bot commented Mar 6, 2026

Copy link
Copy Markdown

GH-145611 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.
vstinner pushed a commit that referenced this pull request Mar 6, 2026
@miss-islington @eendebakpt
[3.14] gh-145376: Fix crashes in md5module.c and hmacmodule.c (GH…
c23dd52 
…-145422) (#145610)

gh-145376: Fix crashes in `md5module.c` and `hmacmodule.c` (GH-145422)

Fix a possible NULL pointer dereference in `md5module.c` and a double-free in `hmacmodule.c`.
Those crashes only occur in error paths taken when the interpreter fails to allocate memory.
(cherry picked from commit c1d7768)

Co-authored-by: Pieter Eendebak <[email protected]>
@eendebakpt eendebakpt mentioned this pull request Mar 6, 2026
[3.13] gh-145376: Fix crashes in md5module.c (GH-145422) #145611
Merged
vstinner pushed a commit that referenced this pull request Mar 6, 2026
@eendebakpt @blurb-it
[3.13] gh-145376: Fix crashes in md5module.c (GH-145422) (#145611)
ae7206e 
* gh-145376: Fix crashes in md5module.c

Fix a possible NULL pointer dereference in `md5module.c`.
This can only occur in error paths taken when the interpreter fails to allocate memory.

(cherry-picked from c1d7768)

* 📜🤖 Added by blurb_it.

* Update Modules/md5module.c

---------

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
@eendebakpt
eendebakpt deleted the null_md5module branch March 19, 2026 21:39
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
@eendebakpt @ljfp
pythongh-145376: Fix crashes in md5module.c and hmacmodule.c (pyt…
fb9693d 
…hon#145422)

Fix a possible NULL pointer dereference in `md5module.c` and a double-free in `hmacmodule.c`.
Those crashes only occur in error paths taken when the interpreter fails to allocate memory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@picnixz picnixz picnixz approved these changes
@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

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.

4 participants

@eendebakpt @picnixz @gpshead @vstinner
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 01402503 — 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.