gh-87804: Fix the refleak in error handling of `_pystatvfs_fromstructstatfs` by sobolevn · Pull Request #115335 · python/cpython

sobolevn · 2024-02-12T14:09:46Z

I messed up macro expansion rules, sorry!

It was:

do { if (PyLong_FromLong((long) st.f_iosize) == ((void*)0)) { Py_DECREF("./Modules/posixmodule.c", 12928, ((PyObject*)((v)))); return ((void*)0); } PyStructSequence_SetItem(v, 0, PyLong_FromLong((long) st.f_iosize)); } while (0);

It is now:

do { PyObject *obj = (PyLong_FromLong((long) st.f_iosize)); if (obj == ((void*)0) || PyErr_Occurred()) { Py_DECREF("./Modules/posixmodule.c", 12929, ((PyObject*)((v)))); return ((void*)0); } PyStructSequence_SetItem(v, (0), obj); } while (0);

Issue: MacOS os.statvfs() has rollover for >4TB disks at each 4TB (32bit counter overflow?) #87804

…structstatfs`

sobolevn · 2024-02-12T14:10:37Z

!buildbot refleak

bedevere-bot · 2024-02-12T14:10:39Z

🤖 New build scheduled with the buildbot fleet by @sobolevn for commit dd821c2 🤖

The command will test the builders whose names match following regular expression: refleak

The builders matched are:

s390x RHEL8 Refleaks PR
AMD64 Windows11 Refleaks PR
AMD64 Fedora Rawhide Refleaks PR
aarch64 Fedora Stable Refleaks PR
AMD64 Ubuntu NoGIL Refleaks PR
ARM64 MacOS M1 Refleaks NoGIL PR
aarch64 CentOS9 Refleaks PR
aarch64 RHEL8 Refleaks PR
s390x RHEL7 Refleaks PR
PPC64LE Fedora Rawhide Refleaks PR
PPC64LE RHEL7 Refleaks PR
PPC64LE Fedora Stable Refleaks PR
PPC64LE RHEL8 Refleaks PR
AMD64 RHEL7 Refleaks PR
s390x Fedora Refleaks PR
s390x Fedora Rawhide Refleaks PR
AMD64 Fedora Stable Refleaks PR
AMD64 CentOS9 Refleaks PR
AMD64 RHEL8 Refleaks PR
PPC64LE CentOS9 Refleaks PR
aarch64 Fedora Rawhide Refleaks PR

Modules/posixmodule.c

encukou · 2024-02-12T14:16:53Z

Good catch!
Could you also name the macro parameters in ALL_CAPS, see https://peps.python.org/pep-0007/#naming-conventions? This convention makes similar issues easier to spot.

Modules/posixmodule.c

encukou

Thanks for the quick fix!

Modules/posixmodule.c

vstinner

LGTM. It would be even better with argument name longer than 1 character :-)

sobolevn · 2024-02-12T15:11:10Z

Once again.

!buildbot refleak

vstinner · 2024-02-12T15:23:53Z

Once again.

I suggest you to give up on buildbots and just merge your PR if you can prove to yourself that your fix works as expected:

tests leak before
tests no longer leak after your change

For example, run ./python.exe -m test bisect test_posix -R 3:3 on macOS. I cannot, I don't have access to macOS.

sobolevn · 2024-02-12T15:36:30Z

I've done manual testing.

Before:

» ./python.exe -m test -R 3:3 test_os test_pickle test_pickletools test_posix test_shutil
Using random seed: 690889334
0:00:00 load avg: 4.23 Run 5 tests sequentially
0:00:00 load avg: 4.23 [1/5] test_os
beginning 6 repetitions
123456
/Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=8886) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=8886) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=8886) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=8886) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=8886) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=8886) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
.
test_os leaked [36, 36, 36] references, sum=108
test_os leaked [36, 36, 36] memory blocks, sum=108
0:00:08 load avg: 3.96 [2/5/1] test_pickle -- test_os failed (reference leak)
beginning 6 repetitions
123456
......
test_pickle leaked [272, 272, 272] references, sum=816
test_pickle leaked [270, 270, 270] memory blocks, sum=810
0:00:45 load avg: 3.27 [3/5/2] test_pickletools -- test_pickle failed (reference leak) in 37.4 sec
beginning 6 repetitions
123456
......
0:00:49 load avg: 3.25 [4/5/2] test_posix
beginning 6 repetitions
123456
......
test_posix leaked [27, 27, 27] references, sum=81
test_posix leaked [27, 27, 27] memory blocks, sum=81
0:00:57 load avg: 3.15 [5/5/3] test_shutil -- test_posix failed (reference leak)
beginning 6 repetitions
123456
......
test_shutil leaked [18, 18, 18] references, sum=54
test_shutil leaked [18, 18, 18] memory blocks, sum=54
test_shutil failed (reference leak)

== Tests result: FAILURE ==

4 tests failed:
    test_os test_pickle test_posix test_shutil

1 test OK.

Total duration: 1 min 1 sec
Total tests: run=1,642 skipped=203
Total test files: run=5/5 failed=4
Result: FAILURE

After:

» ./python.exe -m test -R 3:3 test_os test_pickle test_pickletools test_posix test_shutil
Using random seed: 1906848956
0:00:00 load avg: 1.70 Run 5 tests sequentially
0:00:00 load avg: 1.70 [1/5] test_os
beginning 6 repetitions
123456
/Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=667) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=667) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=667) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=667) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=667) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
./Users/sobolev/Desktop/cpython2/Lib/os.py:861: DeprecationWarning: This process (pid=667) is multi-threaded, use of fork() may lead to deadlocks in the child.
  pid = fork()
.
0:00:08 load avg: 1.98 [2/5] test_pickle
beginning 6 repetitions
123456
......
0:00:46 load avg: 2.45 [3/5] test_pickletools -- test_pickle passed in 38.2 sec
beginning 6 repetitions
123456
......
0:00:50 load avg: 2.33 [4/5] test_posix
beginning 6 repetitions
123456
......
0:00:58 load avg: 2.20 [5/5] test_shutil
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

All 5 tests OK.

Total duration: 1 min 2 sec
Total tests: run=1,642 skipped=203
Total test files: run=5/5
Result: SUCCESS

No refleaks anymore 🎉

I will wait for the CI to get green and merge. Thanks everyone for their feedback!

vstinner · 2024-02-12T16:40:08Z

Thanks for the fix.

…structstatfs` (python#115335) It was the macro expansion! Sorry!

pythongh-87804: Fix the refleak in error handling of `_pystatvfs_from…

dd821c2

…structstatfs`

sobolevn requested review from encukou, ronaldoussoren and vstinner February 12, 2024 14:09

bedevere-app bot added the awaiting core review label Feb 12, 2024

bedevere-app bot mentioned this pull request Feb 12, 2024

MacOS os.statvfs() has rollover for >4TB disks at each 4TB (32bit counter overflow?) #87804

Closed

sobolevn added the skip news label Feb 12, 2024

colesbury reviewed Feb 12, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

Remove the last /

58e1e45

ALL_CAPS

d1da0a2

JelleZijlstra reviewed Feb 12, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

Modules/posixmodule.c Outdated Show resolved Hide resolved

All ()

3c6388b

encukou approved these changes Feb 12, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Feb 12, 2024

Address Victor's review

bbf5699

sobolevn mentioned this pull request Feb 12, 2024

gh-87804: Fix error handling and style in _pystatvfs_fromstructstatfs #115236

Merged

vstinner reviewed Feb 12, 2024

View reviewed changes

Modules/posixmodule.c Outdated Show resolved Hide resolved

Modules/posixmodule.c Outdated Show resolved Hide resolved

vstinner approved these changes Feb 12, 2024

View reviewed changes

Rename V -> SEQ

5e12106

sobolevn force-pushed the issue-87804-2 branch from 9c5d39b to 5e12106 Compare February 12, 2024 15:29

sobolevn merged commit 91bf01d into python:main Feb 12, 2024

bedevere-app bot removed the awaiting merge label Feb 12, 2024

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024

pythongh-87804: Fix the refleak in error handling of `_pystatvfs_from…

2bcdda7

…structstatfs` (python#115335) It was the macro expansion! Sorry!

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

gh-87804: Fix the refleak in error handling of `_pystatvfs_fromstructstatfs`#115335

gh-87804: Fix the refleak in error handling of `_pystatvfs_fromstructstatfs`#115335
sobolevn merged 6 commits intopython:mainfrom
sobolevn:issue-87804-2

sobolevn commented Feb 12, 2024 •

edited by bedevere-app bot

Loading

Uh oh!

sobolevn commented Feb 12, 2024

Uh oh!

bedevere-bot commented Feb 12, 2024

Uh oh!

Uh oh!

encukou commented Feb 12, 2024

Uh oh!

Uh oh!

Uh oh!

encukou left a comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Uh oh!

sobolevn commented Feb 12, 2024

Uh oh!

vstinner commented Feb 12, 2024

Uh oh!

sobolevn commented Feb 12, 2024

Uh oh!

vstinner commented Feb 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

sobolevn commented Feb 12, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Feb 12, 2024

Uh oh!

bedevere-bot commented Feb 12, 2024

Uh oh!

Uh oh!

encukou commented Feb 12, 2024

Uh oh!

Uh oh!

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Feb 12, 2024

Uh oh!

vstinner commented Feb 12, 2024

Uh oh!

sobolevn commented Feb 12, 2024

Uh oh!

vstinner commented Feb 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sobolevn commented Feb 12, 2024 •

edited by bedevere-app bot

Loading