Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recv_slice for RawSocket, IcmpSocket and UdpSocket may lose data #330

Closed
trinity-1686a opened this issue Mar 23, 2020 · 4 comments · Fixed by #859
Closed

recv_slice for RawSocket, IcmpSocket and UdpSocket may lose data #330

trinity-1686a opened this issue Mar 23, 2020 · 4 comments · Fixed by #859
Labels

Comments

@trinity-1686a
Copy link
Contributor

recv_slice for RawSocket, IcmpSocket and UdpSocket may lose data if the provided buffer is smaller than the packet to dequeue.

I don't know if it should not dequeue and return an error, as it might not be possible to get a bigger slice when lacking an allocator, or stay with current behavior. However I think the documentation should be clearer as it seems to say nothing about that.

@whitequark
Copy link
Contributor

Good catch! I seem to have overlooked that while adding these functions. It is possible to use PacketBuffer::dequeue_with to return an error in this case (probably Error::Truncated).

What we should do in this case is an interesting question. The recv_slice methods are, essentially, convenience methods, so any limitations they have do not fundamentally alter the capabilities of the socket. If recv_slice returns an error and does not dequeue, then the caller must handle that case to avoid getting stuck in a receive loop and use recv to dequeue the overlong packet. But that seems to defeat the purpose of using recv_slice.

Therefore I argue that it should return an error (probably Error::Truncated) and dequeue the overlong packet, since that's what the caller will have to do in any case where it can use recv_slice . In any other case recv should be used rather than recv_slice.

@stlankes
Copy link
Contributor

I this still an issue?

@whitequark
Copy link
Contributor

cc @Dirbaio

@whitequark
Copy link
Contributor

Fixed by #859.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants