src/plugin/ipc/socket/socketconnection.h (1)

64-66: Use a safer baseType mask

Mask out flags explicitly to derive the base type; 077 is brittle across platforms.

Apply this diff:

-    int baseType() const { return _sockType & 077; }
+    int baseType() const { return _sockType & ~(SOCK_CLOEXEC | SOCK_NONBLOCK); }

Optional: there is a duplicate include of sys/types.h on Lines 32-33; consider removing one. [As per coding guidelines]

src/plugin/ipc/socket/kernelbufferdrainer.cpp (4)

29-30: Unused include; mismatch with comment

sys/ioctl.h is not used. Either remove it or switch to using FIONREAD as the comment suggests.

Apply one of:

• If keeping current approach, drop the include:

-#include <sys/ioctl.h>

• If switching to FIONREAD, keep the include and update the code (see my other comment on Lines 53-57).

---

38-40: Stale comment

Comment says “using FIONREAD” but code uses recvmsg(MSG_PEEK|MSG_TRUNC). Update the comment for accuracy.

-// Reader for SOCK_SEQPACKET that preserves message boundaries by reading
-// exactly one frame per readOnce() using FIONREAD to determine size.
+// Reader for SOCK_SEQPACKET that preserves message boundaries by reading
+// exactly one frame per readOnce(). It peeks the next message size using
+// recvmsg(MSG_PEEK | MSG_TRUNC).

---

262-279: Avoid assignment in condition; set map explicitly

Using assignment in the if condition is easy to misread.

-  if (_isSeqpacket[fd] = (baseType == SOCK_SEQPACKET)) {
+  const bool isSeq = (baseType == SOCK_SEQPACKET);
+  _isSeqpacket[fd] = isSeq;
+  if (isSeq) {
     addDataSocket(new JSeqpacketReader(fd));
   } else {
     addDataSocket(new jalib::JChunkReader(fd, 512));
   }

---

344-392: Frame replay protocol ignores cmsgs; lengths are fine for AF_UNIX

The REFILL path replays only length-prefixed payloads. If you later support cmsgs, this will need to switch to sendmsg/recvmsg with control data per frame.

Short-term: document that seqpacket drain doesn’t support SCM_RIGHTS/credentials and is disabled/guarded (see earlier comment).
Long-term: extend protocol to carry cmsgs and flags per frame and replay via sendmsg.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/plugin/ipc/socket/kernelbufferdrainer.cpp (1)

68-83: Ancillary data handling still missing (previously flagged).

The previous critical concern about silently dropping ancillary data (SCM_RIGHTS/SCM_CREDENTIALS) remains unaddressed. AF_UNIX/SEQPACKET sockets commonly carry file descriptors or credentials, and dropping them breaks application semantics after restore.

As previously suggested:

• Minimum guard: Supply a control buffer, and if MSG_CTRUNC is set or msg_controllen > 0, set _hadError = true, clear _frame, set errno = ENOTSUP, and return false.
• Preferred fix: Extend drained storage to preserve per-message cmsgs and replay via sendmsg (or mark such sockets as protected and skip draining).

Apply this diff for the minimum guard:

   struct msghdr readMsg;
   memset(&readMsg, 0, sizeof(readMsg));
   readMsg.msg_iov = &iov;
   readMsg.msg_iovlen = 1;
+  char cbuf[256];
+  readMsg.msg_control = cbuf;
+  readMsg.msg_controllen = sizeof(cbuf);
   ssize_t got = ::recvmsg(fd, &readMsg, 0);
+  if (got > 0 && ((readMsg.msg_flags & MSG_CTRUNC) || readMsg.msg_controllen > 0)) {
+    _hadError = true;
+    _frame.clear();
+    errno = ENOTSUP;
+    return false;
+  }

src/plugin/ipc/socket/kernelbufferdrainer.cpp (2)

278-282: Consider clarifying assignment-in-condition.

Line 278 uses assignment within the condition (if (_isSeqpacket[fd] = ...)). While functionally correct, splitting into separate statements improves readability:

- if (_isSeqpacket[fd] = (baseType == SOCK_SEQPACKET)) {
+ _isSeqpacket[fd] = (baseType == SOCK_SEQPACKET);
+ if (_isSeqpacket[fd]) {

---

289-396: Consider refactoring for clarity and adding bounds validation.

The refill logic uses four separate loops with skip conditions, which is correct but complex. Consider the following improvements:

1. Complexity: Extract stream and seqpacket refill into separate helper methods to improve readability.

2. Bounds validation (lines 362-367, 384-394):

   • Frames use uint32_t for length. Consider validating that frame sizes fit within UINT32_MAX before casting.
   • When receiving peer frames, validate len32 is reasonable before allocating buffers to prevent potential DoS from malicious/corrupted peers.

Example bounds check for receiving frames:

   for (int c = 0; c < count; ++c) {
     uint32_t len32 = 0;
     sock.readAll((char *)&len32, sizeof(len32));
+    // Sanity check: reject unreasonably large frames
+    const uint32_t MAX_FRAME_SIZE = 1024 * 1024 * 16; // 16MB limit
+    JASSERT(len32 <= MAX_FRAME_SIZE) (len32) (MAX_FRAME_SIZE)
+      .Text("Peer sent excessively large frame");
     if (len32 > 0) {

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/plugin/ipc/socket/socketconnection.cpp (2)

437-440: Past review comment remains unaddressed.

The previous review recommended using baseType() instead of directly comparing _sockType to handle sockets created with SOCK_CLOEXEC or SOCK_NONBLOCK flags.

Based on past review comments, apply this diff:

   if (!(_sockDomain == AF_INET || _sockDomain == AF_INET6) ||
-      _sockType != SOCK_STREAM) {
+      baseType() != SOCK_STREAM) {
     return;
   }

---

484-486: Past review comment remains unaddressed.

The previous review recommended using baseType() instead of directly comparing _sockType to handle sockets created with SOCK_CLOEXEC or SOCK_NONBLOCK flags.

Based on past review comments, apply this diff:

   if (!(_sockDomain == AF_INET || _sockDomain == AF_INET6) ||
-      _sockType != SOCK_STREAM) {
+      baseType() != SOCK_STREAM) {
     return;
   }

src/plugin/ipc/socket/socketconnection.cpp (1)

659-667: Consider flattening the unnecessary block scope.

The domain-specific socket type validation using baseType() is correct and consistent with the constructor implementation.

The extra block scope at line 659 adds nesting without providing value. Consider flattening it:

-    {
-      if (_sockDomain == AF_UNIX) {
-        JWARNING(baseType() == SOCK_STREAM || baseType() == SOCK_SEQPACKET)
-        (id()) (_sockDomain) (_sockType) (_sockProtocol);
-      } else if (_sockDomain == AF_INET || _sockDomain == AF_INET6) {
-        JWARNING(baseType() == SOCK_STREAM)
-        (id()) (_sockDomain) (_sockType) (_sockProtocol);
-      }
-    }
+    if (_sockDomain == AF_UNIX) {
+      JWARNING(baseType() == SOCK_STREAM || baseType() == SOCK_SEQPACKET)
+      (id()) (_sockDomain) (_sockType) (_sockProtocol);
+    } else if (_sockDomain == AF_INET || _sockDomain == AF_INET6) {
+      JWARNING(baseType() == SOCK_STREAM)
+      (id()) (_sockDomain) (_sockType) (_sockProtocol);
+    }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro