hastd: Support short reads
Needs ReviewPublic
Actions

Authored by des on Sun, Sep 6, 12:39 PM.

Details

Reviewers

kevans
markj
xtronom_gmail.com
pjd
gjb
glebius

Summary

While proto_common_recv() was intended to not return until it has
received the entire requested amount of data or an error occurs, in
practice, due to a bug in the AF_UNIX socket code, it was returning
short reads, and the protocol code was relying on this. When the socket
bug was fixed, this resulted in hastd blocking where it previously would
have returned a short read, so proto_connection_recv() would block
forever trying to read 127 bytes where the other side was only sending
four (typically "tcp\0" or "uds\0").

Replacing MSG_WAITALL with 0 reestablishes the status quo, but that
still leaves us in a situation where callers of proto_recv() may
unknowingly receive less data than they expected. We fix this by having
proto_recv(), tcp_recv(), uds_recv(), and proto_common_recv() return an
ssize_t instead of just 0 or an error code, and having the callers
check that the returned length matches their expectations.

For symmetry, we do the same for the *_send() functions, even though
they already handle short writes by looping.

We also tighten the check in proto_connection_recv() so we accept e.g.
"tcp\0" as intended but not "tcp\0is my jam\n".

While here, replace bcopy() and bzero() with memcpy() and memset().

Reported by: Martin Vidovic <xtronom@gmail.com>
MFC after: 3 days

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76592
Build 73475: arc lint + arc unit

Event Timeline

des created this revision.Sun, Sep 6, 12:39 PM

Herald added a subscriber: imp. · View Herald TranscriptSun, Sep 6, 12:39 PM

des requested review of this revision.Sun, Sep 6, 12:39 PM

Harbormaster completed remote builds in B76591: Diff 185996.Sun, Sep 6, 12:39 PM

This obsoletes D57511

fix hast_proto_recv_{data,hdr}()

Harbormaster completed remote builds in B76592: Diff 185999.Sun, Sep 6, 1:21 PM

glebius added inline comments.Sun, Sep 6, 4:10 PM

sbin/hastd/hast_proto.c
155	This can be a legitimate short read. We could receive a piece of hdr, and next piece is in the buffer or in the fly (TCP). If I read that correct, the patch may produce spurious EPROTOs once in a while.

I'm in favor of using MSG_WAITALL as it makes applications to do less work. Given that hastd protocol can predict when we are expecting to receive a descriptor, the patch like in D57511 seems a good solution for me. The problem I see in D57511 is that in the proto_descriptor_recv() the size of data is not checked, just asked to be > 0. It probably can be combined with your change that changes return values to ssize_t.

In D59465#1363972, @glebius wrote:

I'm in favor of using MSG_WAITALL as it makes applications to do less work. Given that hastd protocol can predict when we are expecting to receive a descriptor, the patch like in D57511 seems a good solution for me. The problem I see in D57511 is that in the proto_descriptor_recv() the size of data is not checked, just asked to be > 0. It probably can be combined with your change that changes return values to ssize_t.

You are completely misreading the problem. It is not about receiving descriptors, it is that the code relies on short reads and the only reason it ever worked is that MSG_WAITALL didn't always block until the entire request was satisfied. The very first read on a new connection (in proto_connection_recv()) asks for 127 bytes and gets 4.

sbin/hastd/hast_proto.c
155	It's not very likely, though. These are the first eight bytes of the message, and since there is no streaming, bundling, buffering, or pipelining in the protocol, they are also the first eight bytes of a packet.

In D59465#1364042, @des wrote:

You are completely misreading the problem. It is not about receiving descriptors, it is that the code relies on short reads and the only reason it ever worked is that MSG_WAITALL didn't always block until the entire request was satisfied. The very first read on a new connection (in proto_connection_recv()) asks for 127 bytes and gets 4.

I doubt that FreeBSD 14 unix/stream socket would make a short read being asked 127 bytes, with only 4 bytes present in the buffer and no other special cases present. It was the descriptor creating the special case that result in the short read.

glebius added inline comments.Sun, Sep 6, 6:23 PM