This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Incorrect behavior of LOAD_ATTR due to overflow in tp_version
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.10, Python 3.9
process
Status: closed Resolution: duplicate
Dependencies: Superseder: tp_version_tag is not unique when test runs with -R :
View: 44914
Assigned To: Nosy List: Mark.Shannon, pablogsal
Priority: normal Keywords:

Created on 2021-05-26 16:03 by Mark.Shannon, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Messages (6)
msg394442 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2021-05-26 16:03
Should the tp_version overflow, and wrap around to a value previously used in the opcache for LOAD_ATTR, then LOAD_ATTR could produce the wrong value.

It will take a fair bit of contrivance and a long time to do this, but it is possible:
Run some code ~2000 times to get the version cached.
Change an attibute of the type about 4 billion times.
Rerun the original code.



Invalidating all the opcaches is going to be a huge pain, so I propose not allowing the version to overflow but partitioning the 32 bit space something like this:

Top 20 bits: Unique per-class ID, 0 and 0xFFFFF are reserved.
Low 12 bits: Per-class version.

tp_version == 0 that no version has been assigned to this class, as yet.
(tp_version & 0xFFF) == 0 means that the version tag is temporarily invalid
tp_version == 0xFFFFFFFF means that the version tag is permanently invalid

If (tp_version & 0xFFF) != 0 and tp_version != 0xFFFFFFFF, then the combined 32 bits represents a unique tag of the class's state as it does now.

Should the low bits of a class hit 0xFFF then all 32 bits are set to 0xFFFFFFFF, and we can't cache its version any more.
If a class has been changed a 1000 times, then there is unlikely to be much value in caching it anyway.
msg394443 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-05-26 16:19
But wait, in the case of an overflow, the values of the tags would be different than the one stored, triggering a cache miss. What I am missing?
msg394444 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-05-26 16:20
Oh, right, it will load and old value from the cache.
msg394445 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-05-26 16:21
I mean, the possibilities are even more remote because not only this needs to overflow but the lookup has to be done at exactly the old value.
msg394447 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2021-05-26 16:48
It is extremely unlikely, I agree. But not impossible.

I plan to fix it for 3.11. Once I've done that we can decide if backports are worth it.
msg394448 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-05-26 17:08
I wonder what other stuff can break due to this. I agree we should fix it, but also we need to consider how crazy is the solution because I'm not sure if this justifies a very complex one.

As an alternative idea, we could use th max allowed value as an "overflowed" status, which invalidates anything that is using the tag.
History
Date User Action Args
2022-04-11 14:59:46adminsetgithub: 88406
2021-08-16 11:02:20Mark.Shannonsetstatus: open -> closed
superseder: tp_version_tag is not unique when test runs with -R :
resolution: duplicate
stage: needs patch -> resolved
2021-05-26 17:08:46pablogsalsetmessages: + msg394448
2021-05-26 16:48:45Mark.Shannonsetmessages: + msg394447
2021-05-26 16:21:15pablogsalsetmessages: + msg394445
2021-05-26 16:20:03pablogsalsetmessages: + msg394444
2021-05-26 16:19:29pablogsalsetmessages: + msg394443
2021-05-26 16:03:18Mark.Shannoncreate