smoltcp-rs / smoltcp

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

return RecvError::Truncated when calling recv_slice with a small buffer #859

Closed thvdveld closed 10 months ago

thvdveld commented 10 months ago

When calling recv_slice/peek_slice in the udp, icmp and raw sockets, data is lost when the provided buffer is smaller than the payload to be copied (not the case for peek_slice because it is not dequeued).

With this commit, an RecvError::Truncated error is returned. In case of UDP, the endpoint is also returned in the error. In case of ICMP, the IP address is also returned in the error.

I implemented it the way @whitequark proposes it in #330. Data is still lost, but at least the caller knows that the data was truncated to the size of the provided buffer. As @whitequark says, it is preferred to call recv instead of recv_slice as there would be no truncation. If this is expected behaviour for recv_slice, we could close #330.

codecov[bot] commented 10 months ago

Codecov Report

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

Comparison is base (a226d5b) 79.60% compared to head (1386417) 79.55%. Report is 1 commits behind head on main.

Files Patch % Lines
src/socket/raw.rs 44.44% 5 Missing :warning:
src/socket/udp.rs 44.44% 5 Missing :warning:
src/socket/icmp.rs 70.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #859 +/- ## ========================================== - Coverage 79.60% 79.55% -0.06% ========================================== Files 78 78 Lines 27896 27917 +21 ========================================== + Hits 22206 22208 +2 - Misses 5690 5709 +19 ```

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

thvdveld commented 10 months ago

I was not sure about that part either. I'll remove the endpoint from the Error::Truncated.

thvdveld commented 10 months ago

here was a packet in the socket that was too big, it is lost

Do you mean that we also don't really need to copy part of the packet in the provided buffer and we should drop the full packet?

whitequark commented 10 months ago

Do you mean that we also don't really need to copy part of the packet in the provided buffer and we should drop the full packet?

Yep. These are convenience methods for the case where you're pretty certain you do not care at all about the bigger packets. If you sometimes want to process only the first part, or you want to execute some complex error handling code, they're not for you.

A note to that end in the docs may be useful.

thvdveld commented 10 months ago

I made the changes and added a note in the documentation of the functions.

Note that now the behaviour of these functions is different: no data is actually copied when the buffer is too small, whereas previously, data was still copied into the smaller buffer.

whitequark commented 10 months ago

Note that now the behaviour of these functions is different: no data is actually copied when the buffer is too small, whereas previously, data was still copied into the smaller buffer.

This is a breaking change regardless of how much data is copied though, isn't it?

thvdveld commented 10 months ago

I think only because of the addition of Truncated to the enums.

thvdveld commented 10 months ago

Do you think we can close #330 now?

whitequark commented 10 months ago

I think only because of the addition of Truncated to the enums.

I mean that returning a different error, with or without payload, in case an overlong packet is detected, is a breaking change. Whether or not the buffer is filled out with data, at that point, doesn't make it any more breaking.

whitequark commented 10 months ago

Do you think we can close #330 now?

Yeah.