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 — bpo-33632: Avoid signed integer overflow in the _thread module by ZackerySpytz · Pull Request #12729 · python/cpython

Conversation

@ZackerySpytz

@ZackerySpytz ZackerySpytz commented Apr 8, 2019 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Co-Authored-By: Martin Panter [email protected]

https://bugs.python.org/issue33632

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ZackerySpytz @vadmium
bpo-33632: Avoid signed integer overflow in the _thread module
e1eb0b4 
Co-Authored-By: Martin Panter <[email protected]>
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 8, 2019
@bedevere-bot bedevere-bot added the awaiting review label Apr 8, 2019
@pganssle

pganssle commented Apr 8, 2019

Copy link
Copy Markdown
Member

@ZackerySpytz The approach looks good to me, but I think it would be good to add a test here.

Is timeout something a user can specify? If timeout + now overflows, does it actually raise an error, or does it just set endtime to a negative number, causing this to time out immediately?

I also think it might be worth adding some of the details of exactly what user-facing behaviors will change with this patch to the changelog. "Avoid signed integer overflow" doesn't mean too much to me, out of context, and the BPO report is not much more detailed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Apr 15, 2019
View reviewed changes
Comment thread Modules/_threadmodule.c
}
else {
timeout -= elapsed;
starttime += elapsed;

@vstinner vstinner Apr 15, 2019

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'm not sure that this part is really needed. I would prefer to leave starttime and timeout unchanged.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pganssle and hongweipeng reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@ZackerySpytz ZackerySpytz Apr 15, 2019

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.

Thank you, @vstinner, for your review.

Sorry, something went wrong.

Uh oh!

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

@vadmium vadmium Apr 20, 2019

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 original code also updates timeout every time it is interrupted. If you don’t adjust the timeout, I think the overall timeout could be too long. I expect you would be able to test this by installing a Python signal handler and sending signals while acquire or whatever method is running.

Perhaps you would prefer to calculate (overall) timeout − (overall) elapsed and pass that to PyThread_acquire_lock_timed.

Sorry, something went wrong.

Uh oh!

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

@hongweipeng hongweipeng Sep 16, 2019

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.

starttime += elapsed will cause starttime overflow. Because starttime is closer to _PyTime_GetMonotonicClock() + timeout by accumulating, and _PyTime_GetMonotonicClock() + timeout may overflow.

I think the else block is not needed.

Sorry, something went wrong.

Uh oh!

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

vadmium commented Apr 20, 2019

Copy link
Copy Markdown
Member

To clarify the changelog, you could say it is a timeout value that causes the overflow, and figure out what APIs are affected. I suspect some acquire method(s) of lock object(s), such as threading.Lock.acquire, but someone with an up-to-date copy of the code should confirm.
pganssle 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.
@csabella

csabella commented May 29, 2019

Copy link
Copy Markdown
Contributor

@ZackerySpytz, please take a look at the code review comments. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

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

hongweipeng commented Sep 16, 2019

Copy link
Copy Markdown
Contributor

@ZackerySpytz Are you interested in continuing to work on this PR?
All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel commented Mar 24, 2022

Copy link
Copy Markdown
Member

This was fixed in #28674, as mentioned on the bpo.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel iritkatriel closed this Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@vadmium vadmium vadmium left review comments
+1 more reviewer
@hongweipeng hongweipeng hongweipeng 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

awaiting 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.

9 participants

@ZackerySpytz @pganssle @vadmium @csabella @hongweipeng @iritkatriel @vstinner @the-knights-who-say-ni @bedevere-bot
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.