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.
WalkthroughReworked privsep message I/O to a two-phase recvmsg flow (peek sizing then mandatory consume), moved per-message buffers to optional heap allocation, removed per-process data cleanup hooks, and removed two fields from the public process struct. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (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)
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
Allows us to distinguish from a shortened header. Log the result and expected sizes to try and diagnose.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/privsep-root.c (2)
118-140: Dynamic buffer growth andiov_lencapping avoid hangs and handle ENOBUFS cleanlyThe
reallocofpsr_ctx->psr_mdataand the subsequent setup:
- Preserve the old buffer if
reallocfails, so the pointer and size remain valid.- Use
iov[1].iov_basefrom eitherpsr_mdataor the caller’s buffer as before.- Critically, cap
iov[1].iov_lentopsr_error->psr_datalen, so the total requested length is never larger than the actual message size, eliminating the previous MSG_WAITALL hang scenario when the context buffer was larger than the incoming payload.- In the “realloc failed” case,
psr_mdatalen < psr_error->psr_datalen, which will force a truncated read in the finalrecvmsg, correctly surfacing ENOBUFS while draining the queue.Only tiny nit: the new comment still says “failed to malloc” even though
reallocis used here.- /* If we failed to malloc then psr_mdatalen will be smaller + /* If we failed to realloc then psr_mdatalen will be smaller
142-157: Finalrecvmsgvalidation is robust; comment could be tightened to match new invariantsThe final
recvmsg(fd, &msg, MSG_WAITALL)plus:
- Short‑header check (
len < sizeof(*psr_error)),- Explicit MSG_TRUNC → ENOBUFS mapping,
- Exact size check against
sizeof(*psr_error) + psr_error->psr_datalenwith EBADMSG and logging,gives a solid guard against partial or malformed records and guarantees the caller sees consistent errors while the message is fully consumed from the queue. With the new
iov_lencapping above, the “receive buffers are bigger than the actual message” case cannot actually occur anymore, so the comment at Line 143 is now slightly misleading; consider updating it to say explicitly that the total requested length is bounded by the announcedpsr_datalen, which is what prevents stalling with MSG_WAITALL.
📜 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-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: netbsd
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: freebsd
- GitHub Check: openbsd
🔇 Additional comments (1)
src/privsep-root.c (1)
89-112: Two-phaserecvmsgwith MSG_PEEK looks correct and keeps the queue in syncThe introduction of
struct msghdr msgand the initialrecvmsg(fd, &msg, MSG_PEEK | MSG_WAITALL)followed by an unconditional non‑peekrecvmsg(viagoto recvwhen needed) ensures you (a) size buffers based on the header, and (b) always actually consume the message from the SOCK_SEQPACKET queue. This addresses the prior risk of acting only on peeked data without draining the record, without introducing obvious new failure modes.
|
Does this one seem ready to merge too? |
I would like some feedback on it first as I still don't know if it fixes people issues as I cannot replicate the problem. |
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.