8000 Ensure orderly shutdown of ssl socket by jepler · Pull Request #7291 · adafruit/circuitpython · GitHub
[go: up one dir, main page]
Skip to content

Ensure orderly shutdown of ssl socket #7291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree t 8000 o our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Dec 8, 2022
Merged

Ensure orderly shutdown of ssl socket #7291

merged 3 commits into from
Dec 8, 2022

Conversation

jepler
Copy link
@jepler jepler commented Dec 2, 2022

A crash would occur if an SSL socket was not shut down before gc_deinit().

I do not fully understand the root cause, but some object deinitialization / deallocation prior to gc_deinit leaves the SSL object in an inconsistent state.

Rather than resolve the root cause, instead ensure that the closing of the user socket also closes the SSL socket.

Testing performed: on Feather ESP32S3 TFT run the reproducer script >10 times. Before, it crashed after 1 time. However, running 10 times helps show I'm not using up socket file descriptors accidentally.

This feels a bit voodoo so I'd love it if instead of incorporating this blindly, someone else went deeper and found the truth.

Closes: #6502

@jepler
Ensure orderly shutdown of ssl socket
7583cca 
A crash would occur if an SSL socket was not shut down before
`gc_deinit()`.

I do not fully understand the root cause, but some object deinitialization
/ deallocation prior to `gc_deinit` leaves the SSL object in an
inconsistent state.

Rather than resolve the root cause, instead ensure that the closing of
the user socket also closes the SSL socket.

Closes: adafruit#6502
@dhalbert
Copy link
Collaborator
dhalbert commented Dec 2, 2022

I am going to spend a little time seeing if I can find the root cause.

@dhalbert
Copy link
Collaborator
dhalbert commented Dec 2, 2022 •
edited
Loading

I do not understand the flow all that well, but this seems a bit fishy to me:

circuitpython/ports/espressif/common-hal/socketpool/Socket.c

Lines 609 to 616 in 9e104c0

void socketpool_socket_reset(socketpool_socket_obj_t *self) {
if (self->base.type == &socketpool_socket_type) {
return;
}
self->base.type = &socketpool_socket_type;
self->connected = false;
self->num = -1;
}

Under what circumstances is the passed in socket not a Socket object? Could it be an SSLSocket? If so then fields in it are set willy-nilly. Also the SSLSocket would not be shut down. And if it's a plain Socket, it's kind of being rendered unfunctional, but the underlying lwip socket isn't decommissioned in any way.

dhalbert
dhalbert previously requested changes Dec 2, 2022
View reviewed changes
Copy link
Collaborator
@dhalbert dhalbert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like the basic idea of this: it is more robust than the previous code.

O 8000 ne q on a specific change, and also a question in the main comments thread.

@jepler
Copy link
Author
jepler commented Dec 7, 2022

I had a theory that for an ssl socket sss created from a regular socket s, closing s first then sss could provoke the same problem without going through interpreter shutdown. However, I was not able to make this happen in practice.

@jepler jepler requested a review from dhalbert December 7, 2022 21:51
@jepler
Copy link
Author
jepler commented Dec 7, 2022

The program I tried:

import wifi, socketpool, ssl, time, gc
wifi.radio.connect('<omitted>')
import socketpool
socket = socketpool.SocketPool(wifi.radio)
ctx = ssl.create_default_context()

b = bytearray(24)
for i in range(8):
    print(i)
    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    sss = ctx.wrap_socket(s)
    sss.connect(('finance.yahoo.com', 443))
    print(sss.send(b"GET /quote/%5EIXIC HTTP/1.0\r\nHost: finance.yahoo.com\r\n\r\n"))
    print(sss.recv_into(b))
    print(bytes(b))
    s.close()
    gc.collect()
    sss.close()
    gc.collect()

@jepler
Copy link
Author
jepler commented Dec 7, 2022

Under what circumstances is the passed in socket not a Socket object? Could it be an SSLSocket? If so then fields in it are set willy-nilly. Also the SSLSocket would not be shut down. And if it's a plain Socket, it's kind of being rendered unfunctional, but the underlying lwip socket isn't decommissioned in any way.

Scott answered this in a voice chat. The routine is actually initializing a socket object for use by the web workflow. The name is not very self-documenting, so changing it and adding appropriate comments would be welcome as a separate PR.

@jepler jepler dismissed dhalbert’s stale review December 7, 2022 23:57

changes made

dhalbert
dhalbert approved these changes Dec 8, 2022
View reviewed changes
Copy link
Collaborator
@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

👍

@dhalbert dhalbert merged commit 44af052 into adafruit:main Dec 8, 2022
@ehagerty ehagerty mentioned this pull request Mar 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalbert dhalbert dhalbert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ESP32-S3 Ctrl-D caused hard crash after get message is sent to adafruit_requests object unless _free_sockets() is called
2 participants
@jepler @dhalbert
0