Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.2k
extmod/modlwip.c: Fix crash when calling recv on listening socket#17333
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Code size report: |
583b17e to210f5c9Compare| @@ -0,0 +1,39 @@ | |||
| # Test recv on listening socket after accept without saving connection | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unless I'm missing something subtle, this test looks identical to the existingtcp_accept_recv.py test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh yeah, I referenced that unit test when adding the one here but for some reason thought it was performing recv on the correct accepted socket rather than directly on the original socket.
I just ran the test between cpython and a rpi pico_2W and it doesn't hard fault reset, but nor does it raise an exception, it never receives anything and hangs.
But then I test again with pyb-d on current master and it passesmulti_net/tcp_accept_recv.py but reliably crashes onmulti_net/tcp_accept_recv_listen.py
Turns out yeah it's rather subtle
s.bind(socket.getaddrinfo("0.0.0.0", PORT)[0][-1])- s.listen()+ s.listen(1) multitest.next()With1 it all passes. Without (or with2 etc...) it crashes - aka if the original socket is still listening for new incomming connections it crashes.
andrewleechMay 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've now consolidating the two tests, modding the original to expose the bug and re-tested before and after with pyb-d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you mean you combined the two tests into the original? Did you forget to push the changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry yes, turns out git had complained about me missing the-f on the push
codecovbot commentedMay 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## master #17333 +/- ##======================================= Coverage 98.54% 98.54% ======================================= Files 169 169 Lines 21898 21898 ======================================= Hits 21579 21579 Misses 319 319 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tests/multi_net/tcp_accept_recv.py Outdated
| @@ -11,13 +11,14 @@ def instance0(): | |||
| s = socket.socket() | |||
| s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | |||
| s.bind(socket.getaddrinfo("0.0.0.0", PORT)[0][-1]) | |||
| s.listen(1) | |||
| s.listen() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This might change the original test with 1 as an argument.
Maybe it's better to do a loop here to perform both tests (with no argument, and with an argument of 1, and then maybe some other values like 0 for good measure).
Also need the client to do a loop as well, and usemultitest.wait() andmultitest.broadcast() to sync with the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, I've got an updated looping test just pushed that passes on the pyb-d with this patch applied.
Add check to prevent calling recv on a socket in the listening state.This prevents a crash/hard fault when user code mistakenly tries torecv on the listening socket instead of on the accepted connection.Add corresponding test case to demonstrate the bug.Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Summary
Fix an issue where calling
recv()on a listening socket afteraccept()would cause a hard fault crash onMicroPython hardware. This happens when user code incorrectly tries to receive data on the listening socket
instead of the accepted connection.
With this fix, MicroPython now properly raises an
OSErrorwitherrno=107(ENOTCONN) similar to CPython,instead of crashing. This makes the behavior consistent with standard socket implementations and improves error
handling.
The bug is demonstrated by the following code pattern:
Once data is sent to this socket from
The first device crashes with a hard fault.
Testing
Added a new test file tests/multi_net/tcp_accept_recv_listen.py that demonstrates the fix by intentionally
triggering the error condition and checking for the appropriate exception.
Testing was performed against a pyboard-d (WIFI):
./run-multitests.py -i pyb:/dev/ttyACM0 -i cpython multi_net/tcp_accept_recv_listen.pyThe test confirms that a proper OSError is raised with the correct errno value (107/ENOTCONN) instead of
crashing.
While this bug was found / fixed manually the unit test and documentation was created with AI support from Claude Code.