gh-110481: Fix Py_SET_REFCNT() integer overflow #112174

vstinner · 2023-11-16T18:15:18Z

If Py_NOGIL is defined and Py_SET_REFCNT() is called with a reference count larger than UINT32_MAX, make the object immortal.

Set _Py_IMMORTAL_REFCNT constant type to Py_ssize_t to fix the following compiler warning:

Include/internal/pycore_global_objects_fini_generated.h:14:24: warning: comparison of integers of different signs: 'Py_ssize_t' (aka 'long') and 'unsigned int' [-Wsign-compare]

if (Py_REFCNT(obj) < _Py_IMMORTAL_REFCNT) {
    ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~

Issue: Implement biased reference counting in --disable-gil builds #110481

vstinner · 2023-11-16T18:34:14Z

@colesbury @eduardo-elizondo @corona10: Would you mind to review these changes?

eduardo-elizondo · 2023-11-16T18:47:01Z

@vstinner I can do a thorough review this Saturday if no one gets to it before then!

colesbury

The biased reference counting changes lgtm.

This adds some C-style casts. What's the "rule" for using C-style casts vs. _Py_STATIC_CAST/_Py_CAST in this file?

vstinner · 2023-11-16T20:10:37Z

Oh, I forgot about _Py_CAST(). i introduced it to avoid C++ compiler warnings with the strict "no old C cast" warning. Sadly, my complicated C++ _Py_CAST() implementation had to be reverted. There are now some basic C++ tests in test_cppext, but it doesn't catch forgotten _Py_CAST().

vstinner · 2023-11-16T20:36:25Z

This adds some C-style casts. What's the "rule" for using C-style casts vs. _Py_STATIC_CAST/_Py_CAST in this file?

I added _Py_CAST() calls.

vstinner · 2023-11-17T14:18:27Z

I modified the PR that Py_SET_REFCNT() makes an object immortal if refcnt is larger than UINT32_MAX. PR based on my previous PR which adds "immortal" to the documentation glossary: #112180

I also added comments in Py_INCREF() to describe the behavior when the reference count in UINT32_MAX in Py_INCREF() (not in the 3rd code path which checks explicitly for _Py_IsImmortal()).

vstinner · 2023-11-17T14:19:29Z

@eduardo-elizondo:

@vstinner I can do a thorough review this Saturday if no one gets to it before then!

Sure, I will wait for your review :-)

Doc/using/configure.rst

If Py_NOGIL is defined and Py_SET_REFCNT() is called with a reference count larger than UINT32_MAX, make the object immortal. Set _Py_IMMORTAL_REFCNT constant type to Py_ssize_t to fix the following compiler warning: Include/internal/pycore_global_objects_fini_generated.h:14:24: warning: comparison of integers of different signs: 'Py_ssize_t' (aka 'long') and 'unsigned int' [-Wsign-compare] if (Py_REFCNT(obj) < _Py_IMMORTAL_REFCNT) { ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~

vstinner · 2023-11-27T11:20:28Z

I prepared the documentation change of this PR by adding the "immortal" term to the documentation glossary: #112180

bedevere-bot · 2023-12-01T14:52:14Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu NoGIL 3.x has failed when building commit 5f6ac2d.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1225/builds/780) and take a look at the build logs.
Check if the failure is related to this commit (5f6ac2d) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1225/builds/780

Summary of the results of the build (if available):

Click to see traceback logs

remote: Enumerating objects: 9, done.        
remote: Counting objects:  16% (1/6)        
remote: Counting objects:  33% (2/6)        
remote: Counting objects:  50% (3/6)        
remote: Counting objects:  66% (4/6)        
remote: Counting objects:  83% (5/6)        
remote: Counting objects: 100% (6/6)        
remote: Counting objects: 100% (6/6), done.        
remote: Compressing objects:  25% (1/4)        
remote: Compressing objects:  50% (2/4)        
remote: Compressing objects:  75% (3/4)        
remote: Compressing objects: 100% (4/4)        
remote: Compressing objects: 100% (4/4), done.        
remote: Total 9 (delta 2), reused 2 (delta 2), pack-reused 3        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '5f6ac2d88a49b8a7c764691365cd41ee6226a8d0'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 5f6ac2d88a gh-110481: Fix Py_SET_REFCNT() integer overflow (#112174)
Switched to and reset branch 'main'

In file included from ./Include/Python.h:58,
                 from ./Programs/python.c:3:
./Include/object.h: In function ‘Py_SET_REFCNT’:
In file included from ./Include/Python.h:58,
                 from Parser/pegen.c:1:
./Include/object.h: In function ‘Py_SET_REFCNT’:
In file included from ./Include/Python.h:58,
                 from Parser/pegen_errors.c:1:
./Include/object.h: In function ‘Py_SET_REFCNT’:
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
In file included from ./Include/Python.h:58,
                 from Parser/pegen.h:4,
                 from Parser/parser.c:2:
./Include/object.h: In function ‘Py_SET_REFCNT’:
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
In file included from ./Include/Python.h:58,
                 from Parser/action_helpers.c:1:
./Include/object.h: In function ‘Py_SET_REFCNT’:
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
In file included from ./Include/Python.h:58,
                 from Parser/lexer/buffer.c:1:
./Include/object.h: In function ‘Py_SET_REFCNT’:
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
make: *** [Makefile:1541: Programs/python.o] Error 1
make: *** Waiting for unfinished jobs....
make: *** [Makefile:2767: Parser/lexer/buffer.o] Error 1
make: *** [Makefile:2767: Parser/pegen_errors.o] Error 1
In file included from ./Include/Python.h:58,
                 from Parser/string_parser.c:3:
./Include/object.h: In function ‘Py_SET_REFCNT’:
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
In file included from ./Include/Python.h:58,
                 from Parser/token.c:3:
./Include/object.h: In function ‘Py_SET_REFCNT’:
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
In file included from ./Include/Python.h:58,
                 from Parser/peg_api.c:1:
./Include/object.h: In function ‘Py_SET_REFCNT’:
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
In file included from ./Include/Python.h:58,
                 from Parser/lexer/lexer.c:1:
./Include/object.h: In function ‘Py_SET_REFCNT’:
./Include/object.h:360:13: error: ‘op’ undeclared (first use in this function); did you mean ‘ob’?
  360 |             op->ob_tid = _Py_UNOWNED_TID;
      |             ^~
      |             ob
./Include/object.h:360:13: note: each undeclared identifier is reported only once for each function it appears in
make: *** [Makefile:2767: Parser/pegen.o] Error 1
make: *** [Makefile:2767: Parser/token.o] Error 1
make: *** [Makefile:2767: Parser/action_helpers.o] Error 1
make: *** [Makefile:2767: Parser/string_parser.o] Error 1
make: *** [Makefile:2767: Parser/peg_api.o] Error 1
make: *** [Makefile:2767: Parser/lexer/lexer.o] Error 1
make: *** [Makefile:2767: Parser/parser.o] Error 1

find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make: [Makefile:2853: clean-retain-profile] Error 1 (ignored)

vstinner · 2023-12-01T14:56:38Z

Oh, I missed an obvious typo :-( Sadly, the NoGIL jobs didn't run in the pre-commit CI. Maybe because I created the PR 2 weeks ago?

Anyway, PR #112595 should fix the buildbot.

bedevere-bot · 2023-12-01T15:03:59Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86-64 MacOS Intel NoGIL 3.x has failed when building commit 5f6ac2d.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1258/builds/292) and take a look at the build logs.
Check if the failure is related to this commit (5f6ac2d) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1258/builds/292

Summary of the results of the build (if available):

Click to see traceback logs

remote: Enumerating objects: 18, done.        
remote: Counting objects:   7% (1/14)        
remote: Counting objects:  14% (2/14)        
remote: Counting objects:  21% (3/14)        
remote: Counting objects:  28% (4/14)        
remote: Counting objects:  35% (5/14)        
remote: Counting objects:  42% (6/14)        
remote: Counting objects:  50% (7/14)        
remote: Counting objects:  57% (8/14)        
remote: Counting objects:  64% (9/14)        
remote: Counting objects:  71% (10/14)        
remote: Counting objects:  78% (11/14)        
remote: Counting objects:  85% (12/14)        
remote: Counting objects:  92% (13/14)        
remote: Counting objects: 100% (14/14)        
remote: Counting objects: 100% (14/14), done.        
remote: Compressing objects:   9% (1/11)        
remote: Compressing objects:  18% (2/11)        
remote: Compressing objects:  27% (3/11)        
remote: Compressing objects:  36% (4/11)        
remote: Compressing objects:  45% (5/11)        
remote: Compressing objects:  54% (6/11)        
remote: Compressing objects:  63% (7/11)        
remote: Compressing objects:  72% (8/11)        
remote: Compressing objects:  81% (9/11)        
remote: Compressing objects:  90% (10/11)        
remote: Compressing objects: 100% (11/11)        
remote: Compressing objects: 100% (11/11), done.        
remote: Total 18 (delta 4), reused 7 (delta 3), pack-reused 4        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '5f6ac2d88a49b8a7c764691365cd41ee6226a8d0'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 5f6ac2d88a gh-110481: Fix Py_SET_REFCNT() integer overflow (#112174)
Switched to and reset branch 'main'

In file included from Parser/action_helpers.c:1:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
In file included from Parser/token.c:3:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
In file included from Parser/lexer/buffer.c:1:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
In file included from Parser/pegen.c:1:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: ./Include/object.h:error: use of undeclared identifier 'op'
362:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;            op->ob_ref_shared = 0;

            ^            ^

./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
In file included from Parser/parser.c:2:
In file included from Parser/pegen.h:4:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
In file included from ./Programs/python.c:3:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
In file included from Parser/string_parser.c:3:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
In file included from Parser/peg_api.c:1:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
In file included from Parser/pegen_errors.c:1:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
In file included from Parser/lexer/lexer.c:1:
In file included from ./Include/Python.h:58:
./Include/object.h:360:13: error: use of undeclared identifier 'op'
            op->ob_tid = _Py_UNOWNED_TID;
            ^
./Include/object.h:361:13: error: use of undeclared identifier 'op'
            op->ob_ref_local = _Py_IMMORTAL_REFCNT_LOCAL;
            ^
./Include/object.h:362:13: error: use of undeclared identifier 'op'
            op->ob_ref_shared = 0;
            ^
3 errors generated.
make: *** [Programs/python.o] Error 1
make: *** Waiting for unfinished jobs....
3 errors generated.
3 errors generated.
3 errors generated.
make: *** [Parser/lexer/buffer.o] Error 1
make: *** [Parser/token.o] Error 1
make: *** [Parser/string_parser.o] Error 1
3 errors generated.
3 errors generated.
make: *** [Parser/peg_api.o] Error 1
make: *** [Parser/pegen_errors.o] Error 1
3 errors generated.
make: *** [Parser/lexer/lexer.o] Error 1
3 errors generated.
3 errors generated.
make: *** [Parser/pegen.o] Error 1
make: *** [Parser/action_helpers.o] Error 1
3 errors generated.
make: *** [Parser/parser.o] Error 1

find: build: No such file or directory
find: build: No such file or directory
find: build: No such file or directory
find: build: No such file or directory
make: [clean-retain-profile] Error 1 (ignored)

If Py_NOGIL is defined and Py_SET_REFCNT() is called with a reference count larger than UINT32_MAX, make the object immortal. Set _Py_IMMORTAL_REFCNT constant type to Py_ssize_t to fix the following compiler warning: Include/internal/pycore_global_objects_fini_generated.h:14:24: warning: comparison of integers of different signs: 'Py_ssize_t' (aka 'long') and 'unsigned int' [-Wsign-compare] if (Py_REFCNT(obj) < _Py_IMMORTAL_REFCNT) { ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~

vstinner added the skip news label Nov 16, 2023

bedevere-app bot added the awaiting core review label Nov 16, 2023

bedevere-app bot mentioned this pull request Nov 16, 2023

Implement biased reference counting in --disable-gil builds #110481

Closed

vstinner force-pushed the signed_refcnt branch from 1d1bc93 to 7d64093 Compare November 16, 2023 18:33

colesbury reviewed Nov 16, 2023

View reviewed changes

vstinner force-pushed the signed_refcnt branch from 4a34e88 to 56fc4e1 Compare November 17, 2023 14:16

vstinner requested review from erlend-aasland and corona10 as code owners November 17, 2023 14:16

Eclips4 reviewed Nov 18, 2023

View reviewed changes

Doc/using/configure.rst Outdated Show resolved Hide resolved

vstinner added 6 commits November 27, 2023 12:17

Use _Py_CAST()

3a53d9b

Document the behavior

53df2c1

Add comment about _Py_IMMORTAL_REFCNT in Py_INCREF()

3bb41e5

fix sphinx syntax in the doc

11f91f3

fix typo in the doc link

d873b98

vstinner force-pushed the signed_refcnt branch from fc4349e to d873b98 Compare November 27, 2023 11:18

vstinner merged commit 5f6ac2d into python:main Dec 1, 2023

vstinner deleted the signed_refcnt branch December 1, 2023 14:50

bedevere-app bot removed the awaiting core review label Dec 1, 2023

vstinner mentioned this pull request Jul 13, 2024

[3.13] _PyObject_Init() assertion fails with the stable ABI on Python built with assertions (extension built with Python 3.11) #121528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-110481: Fix Py_SET_REFCNT() integer overflow #112174

gh-110481: Fix Py_SET_REFCNT() integer overflow #112174

vstinner commented Nov 16, 2023 •

edited

Loading

vstinner commented Nov 16, 2023

eduardo-elizondo commented Nov 16, 2023

colesbury left a comment

vstinner commented Nov 16, 2023

vstinner commented Nov 16, 2023

vstinner commented Nov 17, 2023

vstinner commented Nov 17, 2023

vstinner commented Nov 27, 2023

bedevere-bot commented Dec 1, 2023

vstinner commented Dec 1, 2023

bedevere-bot commented Dec 1, 2023

gh-110481: Fix Py_SET_REFCNT() integer overflow #112174

gh-110481: Fix Py_SET_REFCNT() integer overflow #112174

Conversation

vstinner commented Nov 16, 2023 • edited Loading

vstinner commented Nov 16, 2023

eduardo-elizondo commented Nov 16, 2023

colesbury left a comment

Choose a reason for hiding this comment

vstinner commented Nov 16, 2023

vstinner commented Nov 16, 2023

vstinner commented Nov 17, 2023

vstinner commented Nov 17, 2023

vstinner commented Nov 27, 2023

bedevere-bot commented Dec 1, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

vstinner commented Dec 1, 2023

bedevere-bot commented Dec 1, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

vstinner commented Nov 16, 2023 •

edited

Loading