bpo-33632: Avoid signed integer overflow in the _thread module

[ZackerySpytz]

Co-Authored-By: Martin Panter vadmium+py@gmail.com

https://bugs.python.org/issue33632

[pganssle]

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

[vstinner]

I'm not sure that this part is really needed. I would prefer to leave starttime and timeout unchanged.

[ZackerySpytz]

Thank you, @vstinner, for your review.

[vadmium]

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.

[hongweipeng]

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.

[vadmium]

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.

[csabella]

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

[hongweipeng]

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

[iritkatriel]

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