Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

Conversation

andrewleech
Copy link
Contributor

@andrewleechandrewleech commentedMay 21, 2025
edited
Loading

Summary

Fix an issue where callingrecv() on a listening socket afteraccept() would cause a hard fault crash on
MicroPython 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 anOSError witherrno=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:

importsockets=socket.socket(socket.AF_INET)s.bind(('0.0.0.0',8882))s.listen()s.accept()# connection not saveds.recv(3)# incorrectly calling recv on listening socket

Once data is sent to this socket from

importsockets=socket.socket(socket.AF_INET)s.connect(('127.0.0.1',8882))s.send(b'abc')

The first device crashes with a hard fault.

Closes#16697 (the root cause of thewrap_socket() crash is calling recv on a listening socket.)

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.py

The 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.

@github-actionsGitHub Actions
Copy link

github-actionsbot commentedMay 21, 2025
edited
Loading

Code size report:

   bare-arm:    +0 +0.000% minimal x86:    +0 +0.000%    unix x64:    +0 +0.000% standard      stm32:    +0 +0.000% PYBV10     mimxrt:    +0 +0.000% TEENSY40        rp2:    +0 +0.000% RPI_PICO_W       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS  qemu rv32:    +0 +0.000% VIRT_RV32

@andrewleechandrewleechforce-pushed thesocket_listed_recv branch 2 times, most recently from583b17e to210f5c9CompareMay 21, 2025 02:22
@@ -0,0 +1,39 @@
# Test recv on listening socket after accept without saving connection
Copy link
Member

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?

Copy link
ContributorAuthor

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.

Copy link
ContributorAuthor

@andrewleechandrewleechMay 21, 2025
edited
Loading

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

Copy link
Member

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?

Copy link
ContributorAuthor

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

@dpgeorgedpgeorge added the extmodRelates to extmod/ directory in source labelMay 21, 2025
@codecovCodecov
Copy link

codecovbot commentedMay 21, 2025
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base(e57aa7e) to head(dd7888a).

Additional details and impacted files
@@           Coverage Diff           @@##           master   #17333   +/-   ##=======================================  Coverage   98.57%   98.57%           =======================================  Files         169      169             Lines       21968    21968           =======================================  Hits        21654    21654             Misses        314      314

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@@ -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()
Copy link
Member

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.

Copy link
ContributorAuthor

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.

@projectgusprojectgus self-requested a reviewMay 21, 2025 23:43
Copy link
Contributor

@projectgusprojectgus 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.

This LGTM! I ran the new unit test between PYBD_SF2 (fix applies) and ESP32_GENERIC_S3 (fix doesn't apply) and noted no issues.

I believe this also fixes#16697 - I can't reproduce the crash with test code from the issue any more, and it makes sense that this would be the root cause. So that's good news! EDIT: have added this to the description so that it creates the "close on merge" linkage.

@dpgeorgedpgeorge added this to therelease-1.26.0 milestoneJun 13, 2025
@dpgeorge
Copy link
Member

I've run this updated test on a PYBD_SF6 and RPI_PICO2_W, and it passes without the fix from this PR.

Not sure what to make of that...@andrewleech can you please rerun the test here without the fix and see if it also passes for you?

@andrewleech
Copy link
ContributorAuthor

I believe this also fixes#16697

Ah I missed that one, yes I do believe it should be essentially the same bug. The uninitialised pbuf was the same problem I found when C debugging this hard fault.

@andrewleech
Copy link
ContributorAuthor

I've run this updated test on a PYBD_SF6 and RPI_PICO2_W, and it passes without the fix from this PR.

That's a bit strange, I'll try to test again to ensure the test is hitting the actual problem.

@andrewleech
Copy link
ContributorAuthor

./run-multitests.py -i pyb:/dev/ttyACM0 -i cpython multi_net/tcp_accept_recv_listen.py

-t was needed for me to figure out what was going one; yes it was "passing" in that it's not really running:

anl@STEP:~/micropython2/tests$ ./run-multitests.py -i pyb:/dev/ttyACM0 -i cpython -t multi_net/tcp_accept_recv.pymulti_net/tcp_accept_recv.py on ttyACM0|python3:TRACE ttyACM0|python3:   166 i0 : SET IP = '192.168.0.65'   167 i0 : Test case 1/4: listen()   167 i0 : BROADCAST server_ready_0TRACE python3|python3:   103 i0 : SET IP = '192.168.0.188'   103 i0 : Test case 1/4: listen()   103 i0 : BROADCAST server_ready_0pass1 tests performed1 tests passed

There was a importantnext missing in the test.
Once that was fixed (pushed here now) running the test from this PR without code patch applied results in

anl@STEP:~/micropython2/tests$ ./run-multitests.py -i pyb:/dev/ttyACM0 -i cpython -t multi_net/tcp_accept_recv.pymulti_net/tcp_accept_recv.py on ttyACM0|python3:TRACE ttyACM0|python3:   164 i0 : SET IP = '192.168.0.65'   165 i0 : NEXT   267 i1 : NEXT   267 i0 : Test case 1/4: listen()   269 i0 : BROADCAST server_ready_0Traceback (most recent call last):  File "/home/anl/micropython2/tests/./run-multitests.py", line 632, in main    all_pass &= run_tests(test_files, instances_truth, instances_test_permutation)  File "/home/anl/micropython2/tests/./run-multitests.py", line 503, in run_tests    error, skip, output_test, output_metrics = run_test_on_instances(  File "/home/anl/micropython2/tests/./run-multitests.py", line 391, in run_test_on_instances    out, err = instance.readline()  File "/home/anl/micropython2/tests/./run-multitests.py", line 278, in readline    if self.pyb.serial.inWaiting() == 0:  File "/home/anl/.local/lib/python3.10/site-packages/serial/serialutil.py", line 594, in inWaiting    return self.in_waiting  File "/home/anl/.local/lib/python3.10/site-packages/serial/serialposix.py", line 549, in in_waiting    s = fcntl.ioctl(self.fd, TIOCINQ, TIOCM_zero_str)OSError: [Errno 5] Input/output errorDuring handling of the above exception, another exception occurred:Traceback (most recent call last):  File "/home/anl/.local/lib/python3.10/site-packages/serial/serialposix.py", line 621, in write    n = os.write(self.fd, d)OSError: [Errno 5] Input/output errorDuring handling of the above exception, another exception occurred:Traceback (most recent call last):  File "/home/anl/micropython2/tests/./run-multitests.py", line 645, in <module>    main()  File "/home/anl/micropython2/tests/./run-multitests.py", line 638, in main    i.close()  File "/home/anl/micropython2/tests/./run-multitests.py", line 254, in close    self.pyb.exit_raw_repl()  File "/home/anl/micropython2/tests/../tools/pyboard.py", line 387, in exit_raw_repl    self.serial.write(b"\r\x02")  # ctrl-B: enter friendly REPL  File "/home/anl/.local/lib/python3.10/site-packages/serial/serialposix.py", line 655, in write    raise SerialException('write failed: {}'.format(e))serial.serialutil.SerialException: write failed: [Errno 5] Input/output error

and the pyboard-d resets / hardfaults.

With the patch applied I get:

anl@STEP:~/micropython2/tests$ ./run-multitests.py -i pyb:/dev/ttyACM0 -i cpython -t multi_net/tcp_accept_recv.pymulti_net/tcp_accept_recv.py on ttyACM0|python3:TRACE ttyACM0|python3:   166 i0 : SET IP = '192.168.0.65'   166 i0 : NEXT   269 i1 : NEXT   269 i0 : Test case 1/4: listen()   270 i0 : BROADCAST server_ready_0   677 i0 : True   678 i0 : BROADCAST server_done_0   680 i0 : Test case 2/4: listen(0)   681 i0 : BROADCAST server_ready_1   885 i0 : True   886 i0 : BROADCAST server_done_1   888 i0 : Test case 3/4: listen(1)   889 i0 : BROADCAST server_ready_2  1093 i0 : True  1094 i0 : BROADCAST server_done_2  1095 i0 : Test case 4/4: listen(2)  1097 i0 : BROADCAST server_ready_3  1300 i0 : True  1302 i0 : BROADCAST server_done_3  1303 i0 : NEXTTRACE python3|python3:   103 i0 : SET IP = '192.168.0.188'   103 i0 : NEXT   205 i1 : NEXT   205 i0 : Test case 1/4: listen()   206 i0 : BROADCAST server_ready_0   310 i0 : True   311 i0 : BROADCAST server_done_0   312 i0 : Test case 2/4: listen(0)   313 i0 : BROADCAST server_ready_1   314 i0 : True   315 i0 : BROADCAST server_done_1   317 i0 : Test case 3/4: listen(1)   318 i0 : BROADCAST server_ready_2   319 i0 : True   320 i0 : BROADCAST server_done_2   423 i0 : Test case 4/4: listen(2)   425 i0 : BROADCAST server_ready_3   426 i0 : True   427 i0 : BROADCAST server_done_3   428 i0 : NEXTpass1 tests performed1 tests passed
projectgus reacted with thumbs up emoji

@projectgus
Copy link
Contributor

Oops, good catch@dpgeorge . Thanks@andrewleech for finding the root cause.

Copy link
Member

@dpgeorgedpgeorge left a comment

Choose a reason for hiding this comment

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

OK, this works now. Tested without the fix and the new test crashes PYBD_SF6 and RPI_PICO2_W. With the fix in this PR the test passes.

@dpgeorge
Copy link
Member

I was just about to merge this when I realised that it's not handling the non-blocking case correctly.

The new check added in this PR should go at the top oflwip_tcp_receive(), before checking forsocket->incoming.tcp.pbuf == NULL. Otherwise in the case of a non-blocking socket it'll raise EAGAIN instead of ENOTCONN (unix port of MicroPython and CPython both raise ENOTCONN in non-blocking mode).

I think the non-blocking version of the test should also be added to the test, as an additional combination. So, it'll have a loop of 8 different tests (4 in blocking mode, 4 in non-blocking mode). Adds.setblocking(False) right after thes.accept() line.

(@andrewleech sorry but I force pushed to your branch, so be sure to pull --rebase)

@andrewleech
Copy link
ContributorAuthor

Great catch@dpgeorge I've updated as suggested and re-tested:

corona@NUG:~/mpy/socket_listed_recv/tests$ ./run-multitests.py -t -i pyb:/dev/usb/tty-Pyboard_Virtual_Comm_Port_in_FS_Mode-3254335D3037 -i cpython multi_net/tcp_accept_recv.pyTRACE tty-Pyboard_Virtual_Comm_Port_in_FS_Mode-3254335D3037|python3:   181 i0 : SET IP = '192.168.0.65'   181 i0 : NEXT   284 i1 : NEXT   285 i0 : Test case 1/8: listen() blocking=True   287 i0 : BROADCAST server_ready_1   694 i0 : True   696 i0 : BROADCAST server_done_1   698 i0 : Test case 2/8: listen(0) blocking=True   699 i0 : BROADCAST server_ready_2   903 i0 : True   905 i0 : BROADCAST server_done_2   908 i0 : Test case 3/8: listen(1) blocking=True   910 i0 : BROADCAST server_ready_3  1114 i0 : True  1116 i0 : BROADCAST server_done_3  1118 i0 : Test case 4/8: listen(2) blocking=True  1120 i0 : BROADCAST server_ready_4  1325 i0 : True  1326 i0 : BROADCAST server_done_4  1329 i0 : Test case 5/8: listen() blocking=False  1331 i0 : BROADCAST server_ready_5  1535 i0 : True  1537 i0 : BROADCAST server_done_5  1539 i0 : Test case 6/8: listen(0) blocking=False  1541 i0 : BROADCAST server_ready_6  1644 i0 : True  1646 i0 : BROADCAST server_done_6  1648 i0 : Test case 7/8: listen(1) blocking=False  1650 i0 : BROADCAST server_ready_7  1855 i0 : True  1857 i0 : BROADCAST server_done_7  1859 i0 : Test case 8/8: listen(2) blocking=False  1861 i0 : BROADCAST server_ready_8  2065 i0 : True  2067 i0 : BROADCAST server_done_8TRACE python3|python3:   103 i0 : SET IP = '192.168.0.197'   103 i0 : NEXT   206 i1 : NEXT   206 i0 : Test case 1/8: listen() blocking=True   207 i0 : BROADCAST server_ready_1   311 i0 : True   312 i0 : BROADCAST server_done_1   313 i0 : Test case 2/8: listen(0) blocking=True   314 i0 : BROADCAST server_ready_2   316 i0 : True   317 i0 : BROADCAST server_done_2   318 i0 : Test case 3/8: listen(1) blocking=True   319 i0 : BROADCAST server_ready_3   320 i0 : True   322 i0 : BROADCAST server_done_3   323 i0 : Test case 4/8: listen(2) blocking=True   324 i0 : BROADCAST server_ready_4   325 i0 : True   326 i0 : BROADCAST server_done_4   328 i0 : Test case 5/8: listen() blocking=False   329 i0 : BROADCAST server_ready_5   330 i0 : True   331 i0 : BROADCAST server_done_5   333 i0 : Test case 6/8: listen(0) blocking=False   334 i0 : BROADCAST server_ready_6   335 i0 : True   336 i0 : BROADCAST server_done_6   337 i0 : Test case 7/8: listen(1) blocking=False   339 i0 : BROADCAST server_ready_7   340 i0 : True   341 i0 : BROADCAST server_done_7   342 i0 : Test case 8/8: listen(2) blocking=False   343 i0 : BROADCAST server_ready_8   345 i0 : True   346 i0 : BROADCAST server_done_8pass1 tests performed1 tests passed

Add check to prevent calling recv on a socket in the listening state.  Thisprevents a crash/hard fault when user code mistakenly tries to recv on thelistening 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>
Copy link
Member

@dpgeorgedpgeorge left a comment

Choose a reason for hiding this comment

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

I retested on PYBD_SF6, with the test here, both before and after the fix in this PR. Test fails without the fix and passes with the fix.

andrewleech reacted with heart emoji
@dpgeorgedpgeorge merged commit2f04381 intomicropython:masterJun 26, 2025
65 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@projectgusprojectgusprojectgus approved these changes

@dpgeorgedpgeorgedpgeorge approved these changes

Assignees
No one assigned
Labels
extmodRelates to extmod/ directory in source
Projects
None yet
Milestone
release-1.26.0
Development

Successfully merging this pull request may close these issues.

Callingwrap_socket on a newly created non-SSL socket causes a hard-fault.
4 participants
@andrewleech@dpgeorge@projectgus@pi-anl

[8]ページ先頭

©2009-2025 Movatter.jp