fix: validate NCCL_TESTS_SPLIT env vars and guard division by zero

[randomizedcoder]

Static analysis findings

flawfinder (Level 3, CWE-807) flagged unvalidated getenv() input passed directly to strtoul() without error checking. Manual review of the same code path revealed a division-by-zero crash (CWE-369) and an endptr comparison bug in the parseInt helper function.

Finding	Tool	Location	Severity
strtoul(splitMaskEnv, NULL, 16) — no endptr, no errno check	flawfinder	common.cu:1262	CWE-807 untrusted input
proc % color with no zero guard	manual review	common.cu:1278	CWE-369 division by zero
proc / color with no zero guard	manual review	common.cu:1283	CWE-369 division by zero
parseInt endptr compared against wrong pointer after 0b prefix	manual review	common.cu:1237	False positive parse
parseInt missing errno check for strtoul overflow	manual review	common.cu:1232-1235	CWE-190 integer overflow

All findings are in src/common.cu inside the #ifdef MPI_SUPPORT block that handles NCCL_TESTS_SPLIT_MASK and NCCL_TESTS_SPLIT environment variables for MPI communicator splitting.

Why these matter

strtoul without endptr silently returns 0 on garbage input. The current code does:

color = proc & strtoul(splitMaskEnv, NULL, 16);

strtoul("xyz", NULL, 16) returns 0 — indistinguishable from strtoul("0", NULL, 16). For NCCL_TESTS_SPLIT_MASK=xyz, all MPI ranks silently get masked to color 0 (no split) instead of receiving an error.

Division by zero crashes the MPI process with SIGFPE. The NCCL_TESTS_SPLIT env var supports MOD and DIV operations:

color = proc % color;   // MOD operation
color = proc / color;   // DIV operation

If a user sets NCCL_TESTS_SPLIT=MOD0 or NCCL_TESTS_SPLIT=DIV0, parseInt parses "0" as a valid value, then the integer division by zero delivers SIGFPE and kills the process. The fork-based adversarial test (test_divzero_sigfpe_exploit) proves this crash.

The parseInt helper has a subtle endptr bug that creates another crash path. For input "0bxyz", parseInt matches the "0b" prefix and calls strtoul("xyz", &p, 2). Since "xyz" has no valid binary digits, strtoul consumes nothing and sets p = "xyz". But the error check compares p == s (pointing to "0bxyz"), not p == s+2 (pointing to "xyz"). Since p != s, parseInt falsely returns true with *num = 0. This feeds into the MOD/DIV path and causes the same SIGFPE crash: NCCL_TESTS_SPLIT=MOD0bxyz crashes.

Git history

All affected code traces to a single commit:

Commit	Date	Author	Description
a89cf07	2025-01-23	Junyu Ma	"Perftests: Introduce NCCL_TESTS_SPLIT env"

This commit added the entire NCCL_TESTS_SPLIT / NCCL_TESTS_SPLIT_MASK feature in one pass, including the parseInt helper and the AND/OR/MOD/DIV dispatch logic. The strtoul(..., NULL, 16) shortcut and the missing division-by-zero guards are the kind of oversights that happen when the focus is on getting the feature working for valid inputs. The parseInt endptr bug is a subtle interaction between the 0b-prefix path and the single comparison point — easy to miss in review.

This is the only commit that has ever touched this code. The SPLIT feature is relatively new (Jan 2025) and hasn't been modified since, so these bugs have been present since introduction. The parseInt helper was written specifically for this feature and isn't used anywhere else.

Changes

Commit 1: Tests (TDD — intentionally FAIL before fix)

• tests/c/test_split_safety.c — 7 test cases:
  • test_source_verified — greps source for unsafe patterns (intentionally FAILS)
  • test_strtoul_no_endptr_exploit — proves strtoul("xyz", NULL, 16) indistinguishable from "0"
  • test_strtoul_overflow_exploit — proves overflow undetectable without errno
  • test_divzero_sigfpe_exploit — fork-proves proc % 0 and proc / 0 deliver SIGFPE
  • test_parseInt_0b_endptr_exploit — proves parseInt("0bxyz") falsely succeeds with num=0
  • test_strtoul_hex_validation — table-driven: 9 cases for safe hex parsing
  • test_division_guard — table-driven: guard pattern with color==0 and color>0

Commit 2: Fixes

• parseInt: introduce start pointer so endptr comparison works for both 0b and non-prefix paths; add errno = 0 + ERANGE check for overflow
• SPLIT_MASK: replace raw strtoul(..., NULL, 16) with endptr + errno validation, warn on invalid input
• MOD/DIV: add color == 0 guard with warning, fallback to color=0 (no split)

Test plan

• make -C tests/c test — 7/7 pass after fix
• Commit 1 shows 6/7 pass (source-verification intentionally fails)
• Commit 2 shows 7/7 pass
• test_divzero_sigfpe_exploit fork-proves SIGFPE crash is real
• test_strtoul_no_endptr_exploit proves strtoul silent failure
• test_parseInt_0b_endptr_exploit proves parseInt endptr bug

Files changed

• src/common.cu — parseInt fix (errno + 0b endptr), SPLIT_MASK strtoul validation, MOD/DIV guards
• tests/c/test_split_safety.c — 7 test cases (new file)
• tests/c/Makefile — test build infrastructure
• tests/Makefile — parent test dispatcher

🤖 Generated with Claude Code

[randomizedcoder]

Ok, these are the top 3 for this repo

[sylvesterkaczmarek]

parseInt still accepts trailing garbage. strtoul("2xyz", &p, 0) consumes 2, p != start, errno is clear, so NCCL_TESTS_SPLIT=MOD2xyz is treated as MOD2. It should also require the end pointer to reach the end of the string; the mixed-input test already expects this to be invalid.

[sylvesterkaczmarek]

Rechecked a1ed1dd. parseInt now skips trailing whitespace and then requires the end pointer to reach the end of the string, so values such as MOD2xyz, 0x1fzz and invalid binary tails are rejected instead of partially parsed. The added table-driven regression covers the exact trailing-garbage case I raised plus decimal/hex/binary variants. My concern is resolved.

[randomizedcoder]

@sylvesterkaczmarek Thanks. Conflicts resolved.