Skip to content

Commit ef67988

Browse files
ktsaouktsaou
committed
Fix round 2 review remaining: SHM validation, generation, msg_id rollback
1. SHM service name validation: added to all 3 SHM implementations (was missing — only UDS had it). Prevents path traversal via service_name in .ipcshm path. 2. SHM owner_generation now checked during stale recovery: region is live only if PID is alive AND generation is non-zero (properly initialized). Zero generation = stale/corrupt, reclaim. 3. message_id rolled back from in-flight set on send failure in all 3 UDS implementations. Prevents stale IDs blocking retries. 4. Reserved flags validated in chunk header and hello-ack decode: flags must be 0, returns BadLayout if non-zero. All tests pass: 388 C + 99 Rust + Go all + 18 interop pairs.
1 parent 758dd86 commit ef67988

9 files changed

Lines changed: 154 additions & 15 deletions

File tree

‎src/crates/netipc/src/protocol.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,9 @@ impl ChunkHeader {
247247
if chk.version != VERSION {
248248
return Err(NipcError::BadVersion);
249249
}
250+
if chk.flags != 0 {
251+
return Err(NipcError::BadLayout);
252+
}
250253
if chk.chunk_payload_len == 0 {
251254
return Err(NipcError::BadLayout);
252255
}
@@ -565,6 +568,9 @@ impl HelloAck {
565568
if h.layout_version != 1 {
566569
return Err(NipcError::BadLayout);
567570
}
571+
if h.flags != 0 {
572+
return Err(NipcError::BadLayout);
573+
}
568574
Ok(h)
569575
}
570576
}

‎src/crates/netipc/src/transport/posix.rs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,21 @@ impl UdsSession {
248248
hdr.header_len = protocol::HEADER_LEN;
249249
hdr.payload_len = payload.len() as u32;
250250

251+
let tracked = self.role == Role::Client && hdr.kind == KIND_REQUEST;
252+
let msg_id = hdr.message_id;
253+
254+
let result = self.send_inner(hdr, payload);
255+
256+
// Rollback: remove message_id from in-flight set on send failure
257+
if result.is_err() && tracked {
258+
self.inflight_ids.remove(&msg_id);
259+
}
260+
261+
result
262+
}
263+
264+
/// Inner send logic, separated so the caller can rollback on failure.
265+
fn send_inner(&mut self, hdr: &mut Header, payload: &[u8]) -> Result<(), UdsError> {
251266
let total_msg = HEADER_SIZE + payload.len();
252267

253268
// Single packet?

‎src/crates/netipc/src/transport/shm.rs‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -635,7 +635,27 @@ fn align64(v: u32) -> u32 {
635635
(v + (REGION_ALIGNMENT - 1)) & !(REGION_ALIGNMENT - 1)
636636
}
637637

638+
/// Validate service_name: only [a-zA-Z0-9._-], non-empty, not "." or "..".
639+
fn validate_service_name(name: &str) -> Result<(), ShmError> {
640+
if name.is_empty() {
641+
return Err(ShmError::BadParam("empty service name".into()));
642+
}
643+
if name == "." || name == ".." {
644+
return Err(ShmError::BadParam("service name cannot be '.' or '..'".into()));
645+
}
646+
for c in name.bytes() {
647+
match c {
648+
b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'.' | b'_' | b'-' => {}
649+
_ => return Err(ShmError::BadParam(
650+
format!("service name contains invalid character: {:?}", c as char),
651+
)),
652+
}
653+
}
654+
Ok(())
655+
}
656+
638657
fn build_shm_path(run_dir: &str, service_name: &str) -> Result<PathBuf, ShmError> {
658+
validate_service_name(service_name)?;
639659
let path = Path::new(run_dir).join(format!("{service_name}.ipcshm"));
640660
if path.to_string_lossy().len() >= 256 {
641661
return Err(ShmError::PathTooLong);
@@ -797,13 +817,14 @@ fn check_shm_stale(path: &Path) -> StaleResult {
797817
}
798818

799819
let owner = unsafe { (*hdr).owner_pid };
820+
let gen = unsafe { (*hdr).owner_generation };
800821
unsafe { libc::munmap(map, HEADER_LEN as usize) };
801822

802-
if pid_alive(owner) {
823+
if pid_alive(owner) && gen != 0 {
803824
return StaleResult::LiveServer;
804825
}
805826

806-
// Dead owner — stale
827+
// Dead owner or zero generation (PID reuse / legacy) — stale
807828
unsafe { libc::unlink(c_path.as_ptr()) };
808829
StaleResult::Recovered
809830
}

‎src/go/pkg/netipc/protocol/frame.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,9 @@ func DecodeChunkHeader(buf []byte) (ChunkHeader, error) {
223223
if c.Version != Version {
224224
return ChunkHeader{}, ErrBadVersion
225225
}
226+
if c.Flags != 0 {
227+
return ChunkHeader{}, ErrBadLayout
228+
}
226229
if c.ChunkPayloadLen == 0 {
227230
return ChunkHeader{}, ErrBadLayout
228231
}
@@ -509,6 +512,9 @@ func DecodeHelloAck(buf []byte) (HelloAck, error) {
509512
if h.LayoutVersion != 1 {
510513
return HelloAck{}, ErrBadLayout
511514
}
515+
if h.Flags != 0 {
516+
return HelloAck{}, ErrBadLayout
517+
}
512518
return h, nil
513519
}
514520

‎src/go/pkg/netipc/transport/posix/shm_linux.go‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -567,7 +567,30 @@ func shmAlign64(v uint32) uint32 {
567567
return (v + (shmRegionAlignment - 1)) & ^(shmRegionAlignment - 1)
568568
}
569569

570+
// validateShmServiceName checks that name contains only [a-zA-Z0-9._-],
571+
// is non-empty, and is not "." or "..".
572+
func validateShmServiceName(name string) error {
573+
if name == "" {
574+
return fmt.Errorf("%w: empty service name", ErrShmBadParam)
575+
}
576+
if name == "." || name == ".." {
577+
return fmt.Errorf("%w: service name cannot be '.' or '..'", ErrShmBadParam)
578+
}
579+
for i := 0; i < len(name); i++ {
580+
c := name[i]
581+
if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') ||
582+
(c >= '0' && c <= '9') || c == '.' || c == '_' || c == '-' {
583+
continue
584+
}
585+
return fmt.Errorf("%w: service name contains invalid character: %q", ErrShmBadParam, c)
586+
}
587+
return nil
588+
}
589+
570590
func buildShmPath(runDir, serviceName string) (string, error) {
591+
if err := validateShmServiceName(serviceName); err != nil {
592+
return "", err
593+
}
571594
path := filepath.Join(runDir, serviceName+".ipcshm")
572595
if len(path) >= shmMaxPath {
573596
return "", ErrShmPathTooLong
@@ -714,13 +737,14 @@ func checkShmStale(path string) shmStaleResult {
714737
}
715738

716739
ownerPid := int(int32(binary.LittleEndian.Uint32(data[shmHeaderOwnerPidOff : shmHeaderOwnerPidOff+4])))
740+
ownerGen := binary.LittleEndian.Uint32(data[shmHeaderOwnerGenOff : shmHeaderOwnerGenOff+4])
717741
syscall.Munmap(data)
718742

719-
if pidAlive(ownerPid) {
743+
if pidAlive(ownerPid) && ownerGen != 0 {
720744
return shmStaleLive
721745
}
722746

723-
// Dead owner — stale
747+
// Dead owner or zero generation (PID reuse / legacy) — stale
724748
os.Remove(path)
725749
return shmStaleRecovered
726750
}

‎src/go/pkg/netipc/transport/posix/uds.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,21 @@ func (s *Session) Send(hdr *protocol.Header, payload []byte) error {
195195
hdr.HeaderLen = protocol.HeaderLen
196196
hdr.PayloadLen = uint32(len(payload))
197197

198+
tracked := s.role == RoleClient && hdr.Kind == protocol.KindRequest
199+
200+
sendErr := s.sendInner(hdr, payload)
201+
202+
// Rollback: remove message_id from in-flight set on send failure
203+
if sendErr != nil && tracked {
204+
delete(s.inflightIDs, hdr.MessageID)
205+
}
206+
207+
return sendErr
208+
}
209+
210+
// sendInner performs the actual send logic, separated so the caller can
211+
// rollback the in-flight set on failure.
212+
func (s *Session) sendInner(hdr *protocol.Header, payload []byte) error {
198213
totalMsg := protocol.HeaderSize + len(payload)
199214

200215
// Single packet?

‎src/libnetdata/netipc/src/protocol/netipc_protocol.c‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,8 @@ nipc_error_t nipc_chunk_header_decode(const void *buf, size_t buf_len,
195195
return NIPC_ERR_BAD_MAGIC;
196196
if (out->version != NIPC_VERSION)
197197
return NIPC_ERR_BAD_VERSION;
198+
if (out->flags != 0)
199+
return NIPC_ERR_BAD_LAYOUT;
198200
if (out->chunk_payload_len == 0)
199201
return NIPC_ERR_BAD_LAYOUT;
200202

@@ -439,6 +441,9 @@ nipc_error_t nipc_hello_ack_decode(const void *buf, size_t buf_len,
439441
if (out->layout_version != 1)
440442
return NIPC_ERR_BAD_LAYOUT;
441443

444+
if (out->flags != 0)
445+
return NIPC_ERR_BAD_LAYOUT;
446+
442447
return NIPC_OK;
443448
}
444449

‎src/libnetdata/netipc/src/transport/posix/netipc_shm.c‎

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,33 @@ static inline uint32_t align64(uint32_t v)
3333
return (v + (NIPC_SHM_REGION_ALIGNMENT - 1)) & ~(uint32_t)(NIPC_SHM_REGION_ALIGNMENT - 1);
3434
}
3535

36+
/* Validate service_name: only [a-zA-Z0-9._-], non-empty, not "." or "..". */
37+
static int validate_service_name(const char *name)
38+
{
39+
if (!name || name[0] == '\0')
40+
return -1;
41+
42+
/* Reject "." and ".." */
43+
if (name[0] == '.' && (name[1] == '\0' || (name[1] == '.' && name[2] == '\0')))
44+
return -1;
45+
46+
for (const char *p = name; *p; p++) {
47+
char c = *p;
48+
if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') ||
49+
(c >= '0' && c <= '9') || c == '.' || c == '_' || c == '-')
50+
continue;
51+
return -1;
52+
}
53+
return 0;
54+
}
55+
3656
/* Build SHM file path: {run_dir}/{service_name}.ipcshm */
3757
static int build_shm_path(char *dst, size_t dst_len,
3858
const char *run_dir, const char *service_name)
3959
{
60+
if (validate_service_name(service_name) < 0)
61+
return -2; /* invalid service name */
62+
4063
int n = snprintf(dst, dst_len, "%s/%s.ipcshm", run_dir, service_name);
4164
if (n < 0 || (size_t)n >= dst_len)
4265
return -1;
@@ -158,13 +181,11 @@ static int check_shm_stale(const char *path)
158181
uint32_t gen = hdr->owner_generation;
159182
munmap(map, NIPC_SHM_HEADER_LEN);
160183

161-
if (pid_alive((pid_t)owner)) {
162-
return 1; /* live */
184+
if (pid_alive((pid_t)owner) && gen != 0) {
185+
return 1; /* live: PID alive and generation is valid */
163186
}
164187

165-
(void)gen; /* generation is cached on attach for runtime checks */
166-
167-
/* Dead owner -- stale. */
188+
/* Dead owner or zero generation (PID reuse / legacy) -- stale. */
168189
unlink(path);
169190
return 0;
170191
}
@@ -185,9 +206,12 @@ nipc_shm_error_t nipc_shm_server_create(const char *run_dir,
185206
if (!run_dir || !service_name || !out)
186207
return NIPC_SHM_ERR_BAD_PARAM;
187208

188-
/* Build path */
209+
/* Build path (validates service_name) */
189210
char path[256];
190-
if (build_shm_path(path, sizeof(path), run_dir, service_name) < 0)
211+
int path_rc = build_shm_path(path, sizeof(path), run_dir, service_name);
212+
if (path_rc == -2)
213+
return NIPC_SHM_ERR_BAD_PARAM;
214+
if (path_rc < 0)
191215
return NIPC_SHM_ERR_PATH_TOO_LONG;
192216

193217
/* Stale recovery */
@@ -307,7 +331,10 @@ nipc_shm_error_t nipc_shm_client_attach(const char *run_dir,
307331
return NIPC_SHM_ERR_BAD_PARAM;
308332

309333
char path[256];
310-
if (build_shm_path(path, sizeof(path), run_dir, service_name) < 0)
334+
int path_rc = build_shm_path(path, sizeof(path), run_dir, service_name);
335+
if (path_rc == -2)
336+
return NIPC_SHM_ERR_BAD_PARAM;
337+
if (path_rc < 0)
311338
return NIPC_SHM_ERR_PATH_TOO_LONG;
312339

313340
/* Open the file. */

‎src/libnetdata/netipc/src/transport/posix/netipc_uds.c‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -672,8 +672,16 @@ nipc_uds_error_t nipc_uds_send(nipc_uds_session_t *session,
672672
/* Single packet: encode header then send header+payload together */
673673
uint8_t hdr_buf[NIPC_HEADER_LEN];
674674
nipc_header_encode(hdr, hdr_buf, sizeof(hdr_buf));
675-
return raw_send_iov(session->fd, hdr_buf, NIPC_HEADER_LEN,
675+
nipc_uds_error_t send_err = raw_send_iov(session->fd, hdr_buf, NIPC_HEADER_LEN,
676676
payload, payload_len);
677+
if (send_err != NIPC_UDS_OK) {
678+
/* Rollback: remove message_id from in-flight set */
679+
if (session->role == NIPC_UDS_ROLE_CLIENT &&
680+
hdr->kind == NIPC_KIND_REQUEST) {
681+
inflight_remove(session, hdr->message_id);
682+
}
683+
}
684+
return send_err;
677685
}
678686

679687
/* Chunked send */
@@ -703,8 +711,14 @@ nipc_uds_error_t nipc_uds_send(nipc_uds_session_t *session,
703711

704712
nipc_uds_error_t err = raw_send_iov(session->fd, hdr_buf, NIPC_HEADER_LEN,
705713
payload, first_chunk_payload);
706-
if (err != NIPC_UDS_OK)
714+
if (err != NIPC_UDS_OK) {
715+
/* Rollback: remove message_id from in-flight set */
716+
if (session->role == NIPC_UDS_ROLE_CLIENT &&
717+
hdr->kind == NIPC_KIND_REQUEST) {
718+
inflight_remove(session, hdr->message_id);
719+
}
707720
return err;
721+
}
708722

709723
/* Send continuation chunks */
710724
const uint8_t *src = (const uint8_t *)payload + first_chunk_payload;
@@ -730,8 +744,14 @@ nipc_uds_error_t nipc_uds_send(nipc_uds_session_t *session,
730744

731745
err = raw_send_iov(session->fd, chk_buf, NIPC_HEADER_LEN,
732746
src, this_chunk);
733-
if (err != NIPC_UDS_OK)
747+
if (err != NIPC_UDS_OK) {
748+
/* Rollback: remove message_id from in-flight set */
749+
if (session->role == NIPC_UDS_ROLE_CLIENT &&
750+
hdr->kind == NIPC_KIND_REQUEST) {
751+
inflight_remove(session, hdr->message_id);
752+
}
734753
return err;
754+
}
735755

736756
src += this_chunk;
737757
remaining -= this_chunk;

0 commit comments

Comments
 (0)