privsep: Ensure we recv for real after a successful recv MSG_PEEK #556
Conversation
Adjust the code flow so that the same errors would be caught after the final recv. This ensures we read what is really meant for us and not something silly. Should fix #555.
WalkthroughReplaced direct recv with recvmsg using msghdr/iovec and a two-phase peek + full-read (MSG_PEEK then MSG_WAITALL) flow for PSR messages; added centralized final recv path, MSG_TRUNC/ENOBUFS validation, and adjusted dynamic data allocation and iovec length semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Peer as Peer process
participant Root as privsep-root
participant Validator as Validator
Peer->>Root: UDP/TCP PSR message arrives
note right of Root `#e6f2ff`: 1) recvmsg(MSG_PEEK) reads header only
Root->>Root: determine psr_datalen, attempt realloc if mdata used
alt enough buffer / proceed
Peer->>Root: recvmsg(MSG_WAITALL, iov[header,data])
Root->>Validator: validate total size == header + psr_datalen
alt MSG_TRUNC or size mismatch
Root->>Validator: signal truncation / ENOBUFS
else exact match
Root->>Validator: deliver message
end
else insufficient buffer / realloc failed
Peer->>Root: recvmsg to consume/truncate remaining data
Root->>Validator: report ENOBUFS and discard
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/privsep-root.c(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: netbsd
- GitHub Check: freebsd
- GitHub Check: openbsd
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/privsep-root.c (1)
129-138: Critical: Fix MSG_WAITALL hang by using incoming message size.Lines 134 and 137 set
iov_lenfrom the context buffer sizes (psr_ctx->psr_mdatalen,psr_ctx->psr_datalen), which can be larger than the actual incoming message sizepsr_error->psr_datalen. WhenMSG_WAITALLis used with aniov_lenlarger than available data,recvmsgblocks indefinitely waiting for bytes that will never arrive, wedging the privileged proxy and causing cascading failures like "No buffer space available."This is the same critical issue identified in the past review and must be fixed.
Apply this diff to clamp
iov_lento the actual incoming message size:if (psr_error->psr_datalen != 0) { if (psr_ctx->psr_usemdata) { iov[1].iov_base = psr_ctx->psr_mdata; - /* psr_mdatalen could be smaller then psr_datalen - * if the above malloc failed. */ - iov[1].iov_len = - MIN(psr_ctx->psr_mdatalen, psr_ctx->psr_datalen); + /* Clamp to actual incoming size to avoid MSG_WAITALL hang. + * If buffer is too small, MSG_TRUNC will catch it. */ + iov[1].iov_len = psr_error->psr_datalen; + if (iov[1].iov_len > psr_ctx->psr_mdatalen) + iov[1].iov_len = psr_ctx->psr_mdatalen; } else { iov[1].iov_base = psr_ctx->psr_data; - iov[1].iov_len = psr_ctx->psr_datalen; + iov[1].iov_len = psr_error->psr_datalen; + if (iov[1].iov_len > psr_ctx->psr_datalen) + iov[1].iov_len = psr_ctx->psr_datalen; } }
🧹 Nitpick comments (1)
src/privsep-root.c (1)
102-111: Consider removing MSG_WAITALL from the MSG_PEEK operation.Using
MSG_WAITALLwithMSG_PEEKmay cause unnecessary blocking when the message header hasn't fully arrived yet. Since this is just a peek operation to determine buffer size, and you're checking for short reads immediately after (line 110),MSG_PEEKalone should suffice.Apply this diff:
- len = recvmsg(fd, &msg, MSG_PEEK | MSG_WAITALL); + len = recvmsg(fd, &msg, MSG_PEEK);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/privsep-root.c(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: freebsd
- GitHub Check: openbsd
🔇 Additional comments (2)
src/privsep-root.c (2)
113-127: LGTM! Proper handling of allocation failure.The allocation failure handling is correct: when
reallocfails, the code leaves the smaller buffer in place and relies onMSG_TRUNCdetection in the subsequentrecvmsgto signalENOBUFS. This ensures the message is properly discarded from the queue.
141-151: LGTM! Comprehensive validation of received message.The final receive path correctly:
- Centralizes the actual message consumption for both early failures and normal flow
- Validates minimum header size
- Detects truncation via
MSG_TRUNCand returnsENOBUFS- Ensures exact size match between received data and expected header + payload
Adjust the code flow so that the same errors would be caught after the final recv.
This ensures we read what is really meant for us and not something silly.
Should fix #555.