Skip to content

Conversation

@graingert
Copy link
Contributor

@graingert graingert commented Dec 19, 2023

@graingert graingert force-pushed the fix-resource-warning-in-ssl branch from 97b76b3 to 9c36721 Compare December 19, 2023 10:03
@graingert graingert force-pushed the fix-resource-warning-in-ssl branch from 3d88a5f to a9b6d88 Compare December 19, 2023 10:12
@graingert graingert marked this pull request as ready for review December 19, 2023 10:16
@hugovk hugovk added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Dec 19, 2023
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can self.close() raise OSError in these cases?

How difficult to write tests?

@graingert
Copy link
Contributor Author

I'm pretty sure self.close() doesn't raise anything, but if it did it would be raised when __del__ is called.

It's tricky to test as I only managed to reproduce this intermittently

@serhiy-storchaka
Copy link
Member

Several lines below an OSError raised by self.close() is ignored.
https://github.com/python/cpython/pull/113282/files#diff-f6439be9c66350dde4c35dbeea0352c96cc970ba12b0478f6ae36f10725bd8c5R1047-R1050

I wonder whether such guard is needed in these cases.

@graingert
Copy link
Contributor Author

graingert commented Jan 27, 2024

I think self.close() only raises if os.close(sock.fileno()) or socket.close(sock.fileno()) was called, which would be useful to raise because this would be a fd use after close bug

@serhiy-storchaka
Copy link
Member

self.close() is called several times in this function, but there may be other errors which causes a leak. It is better to always close socket if SSLSocket creation failed. I created more general #114659 based on this PR.

@graingert graingert deleted the fix-resource-warning-in-ssl branch February 4, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants