Details

Reviewers

gjb
pjd
kevans

Commits

rG4d83686bc387: hastd: Fix crash on empty message
rG2581bc3af381: hastd: Fix crash on empty message
rG8646d65b4533: hastd: Fix crash on empty message

Summary

A HAST message can be empty, in which case ebuf_data() will return NULL,
but in the receive path, we assert that the return value is not NULL.
Thus sending a message to hastd with an empty payload results in an
immediate crash. This is trivially reproducable by running `hastctl
status` or hastctl role init (which the rc script happens to do prior
to stopping hastd).

PR: 298085
MFC after: 3 days

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76377
Build 73260: arc lint + arc unit

Event Timeline

des created this revision.Tue, Sep 1, 1:57 PM

Herald added a subscriber: imp. · View Herald TranscriptTue, Sep 1, 1:57 PM

des requested review of this revision.Tue, Sep 1, 1:57 PM

Harbormaster completed remote builds in B76377: Diff 185561.Tue, Sep 1, 1:57 PM

drop binary

Harbormaster completed remote builds in B76378: Diff 185564.Tue, Sep 1, 1:57 PM

I'm not convinced this patch is correct. I think we should call proto_recv() in all cases.

In D59306#1360617, @des wrote:

I'm not convinced this patch is correct. I think we should call proto_recv() in all cases.

OK, I misunderstood the code; I thought proto_recv() called the handler for the message, but what it actually does is read the payload from the socket, so we only need to call it if there is a payload to be read, and the patch is fine.

tabify

Harbormaster completed remote builds in B76383: Diff 185570.Tue, Sep 1, 3:55 PM

don't call ebuf_add_tail() needlessly

Harbormaster completed remote builds in B76384: Diff 185571.Tue, Sep 1, 4:05 PM

des added inline comments.Tue, Sep 1, 4:07 PM

sbin/hastd/hast_proto.c
156	actually this is completely redundant because we already gave `hdr.size` to `ebuf_alloc()` so it already allocated enough memory for it (and a whole additional page, for some reason).

des added inline comments.Tue, Sep 1, 4:09 PM

sbin/hastd/hast_proto.c
156	hmm that turned out not to be correct in practice, probably a bug in the ebuf code

des edited the summary of this revision. (Show Details)Tue, Sep 1, 4:32 PM

des added inline comments.Tue, Sep 1, 4:47 PM

sbin/hastd/hast_proto.c
156	It's actually not a bug; `ebuf_alloc(hdr.size)` creates an ebuf with no data but with the expectation that at least `hdr.size` bytes will be written to it in the future, while `ebuf_add_tail(eb, NULL, hdr.size)` adds `hdr.size` bytes of uninitialized data to the ebuf (without allocating any additional memory); `ebuf_data()` will return `NULL` until the first call to either `ebuf_add_head()` or `ebuf_add_tail()`.