[lumenyr]

socket.fromfd() duplicates the descriptor and then hands it to the constructor. If the constructor rejects its arguments, the duplicate has no owner and is never closed.

fd = sock.fileno()
for _ in range(3):
    try:
        socket.fromfd(fd, socket.AF_INET, socket.SOCK_STREAM, 1 << 100)
    except OverflowError:
        pass

That leaks 3 descriptors; with the change, 0. A valid call still returns a working socket and the descriptor count is unchanged.

The test counts /proc/self/fd before and after one rejected call, so it is skipped where /proc is absent. It passes on this branch and fails against unmodified Lib/socket.py with AssertionError: 7 != 6.

./python -m unittest test.test_socket.BasicTCPTest.testFromFdInvalidArgsDoesNotLeak is OK, and test_socket is green on the free-threading debug build (run=755, skipped=50).

NEWS entry included. No behaviour change on the success path.

• Issue: socket.fromfd() leaks the dup()'d fd when family/type/proto are invalid #160561

[vstinner]

I don't think that the comment is useful.

Suggested change

	# socket() rejected the arguments; the dup()'d descriptor would
	# otherwise leak, since nothing holds a reference to it.
	close(nfd)
	close(nfd)

[vstinner]

IMO a bare except is better here, since we just re-raise the exception later.

Suggested change

	except BaseException:
	except:

[vstinner]

You can use os_helper.fd_count() which works on all platforms. I suggest a shorter test name and more flat code to trigger errors:

Suggested change

	def testFromFdError(self):
	# Make sure that the fromfd() duplicated socket is closed on error.
	# In debug mode, Python test suite checks for file descriptor leaks.
	fd = self.cli_conn.fileno()
	fd_count_before = os_helper.fd_count()
	with self.assertRaises(TypeError):
	socket.fromfd(fd, "not-a-valid-family", socket.SOCK_STREAM)
	with self.assertRaises(TypeError):
	socket.fromfd(fd, socket.AF_UNIX, "not-a-valid-type")
	with self.assertRaises(OverflowError):
	socket.fromfd(fd, socket.AF_UNIX, 1 << 100)
	self.assertEqual(os_helper.fd_count(), fd_count_before)
	def _testFromFdError(self):
	pass

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[vstinner]

@lumenyr: Please sign the Python CLA: see #160579 (comment) comment.

[lumenyr]

All three applied, thank you.

• except: with the redundant comment removed; the duplicated descriptor is closed and the exception re-raised.
• The test is now testFromFdError, flat, using os_helper.fd_count(), with your three cases exactly: invalid family string and invalid type string (TypeError), then 1 << 100 (OverflowError), and the descriptor count compared before and after.

Checked both sides on a free-threading debug build: with the guard the test passes, and with the guard taken out it fails twice, one for each of the TypeError and OverflowError paths, so both paths are really exercising the fix. Checks are green.

Separately: the account was renamed, which is why the CLA check asked again. It is signed and the bot is green now.

[vstinner]

LGTM. Please just remove the outdated comment (second line of the PR).

There are warnings about AF_UNIX, but you already replaced it with AF_INET. I'm not sure why warnings are still there.

[vstinner]

I'm sorry, I suggested this comment but then added os_helper.fd_count(). This comment can be removed.

[lumenyr]

Comment removed in 3cf12d1.

On the AF_UNIX warnings: the socket.AF_UNIX calls were in f7e4d09, which 2cbb4c0 replaced with AF_INET; testFromFdError on the current head contains no AF_UNIX, so any warnings in the jobs listed for this PR belong to that superseded revision. ./python -m unittest BasicTCPTest.testFromFdError BasicTCPTest2.testFromFdError on the release free-threading build: Ran 2 tests, OK, 0.058 s.

[lumenyr]

Both done in 86b7495: the comment is gone, and the test now matches your snippet exactly, including AF_UNIX. For the record, python -W error -m unittest test.test_socket.BasicTCPTest.testFromFdError test.test_socket.BasicTCPTest2.testFromFdError is clean on a free-threading debug build, so AF_UNIX raises no warnings here; the earlier AF_INET revision is superseded and can be ignored.