search

smoltcp-rs / smoltcp

a smol tcp/ip stack
BSD Zero Clause License
3.75k stars 421 forks source link

fix(tcp): don't send TCP RST with unsp. src addr #867

Closed thvdveld closed 9 months ago

thvdveld commented 10 months ago

When the destination address of the TCP packet was unspecified and the packet was not handled by any socket, smoltcp would transmit a TCP RST packet back. We should never transmit a packet with a source address unspecified.

codecov[bot] commented 10 months ago

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (36a143d) 79.62% compared to head (73ac7a4) 79.58%. Report is 9 commits behind head on main.

:exclamation: Current head 73ac7a4 differs from pull request most recent head 3369fb9. Consider uploading reports for the commit 3369fb9 to get more accurate results

Files Patch % Lines
src/iface/interface/mod.rs 61.53% 5 Missing :warning:
src/socket/dns.rs 0.00% 1 Missing :warning:
src/socket/udp.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #867 +/- ## ========================================== - Coverage 79.62% 79.58% -0.05% ========================================== Files 78 78 Lines 28043 28122 +79 ========================================== + Hits 22329 22380 +51 - Misses 5714 5742 +28 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thvdveld commented 9 months ago

Wait, this was merged when CI failed, how could the merge queue do that. I thought this was not possible as it said merge when ready with ready being all checks successful.

whitequark commented 9 months ago

Wait, this was merged when CI failed, how could the merge queue do that. I thought this was not possible as it said merge when ready with ready being all checks successful.

This can happen if the merge queue is not set up correctly. I see tests has been skipped, is that the required check? Skipped checks are counterintuitively considered successful for merge queue purposes.

thvdveld commented 9 months ago

I have no idea why that one was skipped. I don't see it in the workflow files. And I think all CI jobs are required: https://github.com/smoltcp-rs/smoltcp/pull/872#issuecomment-1849762059