SSV-26770 , SSV-26896 CodeQL must fix error fixes - #118
Merged
Conversation
CodeQL cpp/drivers/wdk-deprecated-api flagged 27 ExAllocatePoolWithTag call sites (required for WHCP Static Tools Logo Test certification). The real replacement, ExAllocatePool2, isn't available: this driver targets _WIN32_WINNT=0x0601 (Windows 7) and ExAllocatePool2 needs NTDDI_WIN10_VB. Use Microsoft's downlevel-safe inline wrappers instead: - ExAllocatePoolUninitialized where the allocation is already fully overwritten right after (behavior-preserving, identical passthrough to ExAllocatePoolWithTag under the hood). - ExAllocatePoolZero where it wasn't already zeroed and safety outweighs the negligible one-off cost (debug ring buffer dumped via windbg .writemem; a reparse buffer and a PnP query-id buffer that could leak stale pool memory toward user-mode). Also drops now redundant explicit RtlZeroMemory calls at sites already zeroing. Covers 8 of the 27 sites via two shared macros (MALLOC in kmem.h, SBMALLOC in spl-kstat.c), confirmed to have no other callers.
CodeQL cpp/drivers/wdk-deprecated-api flagged 3 ExAllocatePool call sites, none of which passed a pool tag. Same constraint as the prior ExAllocatePoolWithTag fix: ExAllocatePool2 needs NTDDI_WIN10_VB, not available at this driver's current _WIN32_WINNT=0x0601 target, so use the downlevel-safe wrappers and add a tag (previously missing): - zfs_vnops_windows.c:5322 (PnP device relations) -> Uninitialized, tag '!DRZ' (matches this file's existing '!FSZ'/'!OIZ' style); fully overwritten right after allocation. - zfs_vnops_windows_mount.c:328/451 -> Zero, tags 'ZVAN'/'ZVCP'; both were already explicitly RtlZeroMemory'd, so this also drops the now-redundant zero call.
CodeQL cpp/drivers/wdk-deprecated-api flagged this as a call to the deprecated ExAllocatePoolWithQuotaTag (FsRtlAllocatePoolWithQuotaTag is a WDK macro expanding to it). Microsoft's replacement is ExAllocatePool2 + POOL_FLAG_USE_QUOTA, unavailable at this driver's current _WIN32_WINNT=0x0601 target (needs NTDDI_WIN10_VB), and neither downlevel-safe wrapper (ExAllocatePoolZero/Uninitialized) supports quota-charging. Decision: drop quota-charging for this allocation and use ExAllocatePoolZero. The IRP buffer is fully overwritten by RtlCopyMemory right after anyway, so this only changes whether the allocation is charged against the caller's pool quota - a rarely exercised accounting feature, not a correctness path. Revisit with real ExAllocatePool2 + POOL_FLAG_USE_QUOTA if the driver's minimum Windows version is ever raised.
CodeQL cpp/drivers/extended-deprecated-apis flagged 128 _snprintf calls. 126 of these are not direct calls: portable snprintf() calls throughout cross-platform ZFS/SPL code resolve to _snprintf only because of `#define snprintf _snprintf` in types.h. Kernel-mode _snprintf returns -1 on truncation (not the would-be length real snprintf() returns) and does not NUL-terminate on truncation, so Windows-only bugs were possible wherever portable code assumed real snprintf() semantics: - zfs_fletcher.c / vdev_raidz_math.c kstat formatters use `off += snprintf(buf+off, size-off, ...)`. A mid-chain truncation set off=-1, corrupting buf+off/size-off into an out-of-bounds write on Windows only - cannot happen on other OpenZFS platforms. - dmu_redact.c:1081 and zcp_iter.c:554 check `if (n >= SIZE) return ENAMETOOLONG`, which never fired on Windows (-1 is never >= SIZE), so over-length dataset/bookmark names were silently truncated and accepted instead of rejected. This fix makes that check work correctly - Windows now rejects them like every other platform. Intentional behavior change, not a side effect to hide. Fix: added spl_snprintf/spl_vsnprintf (types.h), built only from functions confirmed exported by this driver's actual ntoskrnl.lib target (_vsnprintf, _vsnprintf_s - no _vscprintf available in kernel mode). Required length is measured via _vsnprintf(NULL, 0, ...), which returns the true length for count==0 - the same idiom kmem_asprintf() already relies on in this codebase. Retargeted only the `snprintf` macro to this shim; `vsnprintf`/`_vsnprintf` are separate findings left for later commits. Also fixed the 2 genuine direct _snprintf calls (module/os/windows/ debug.c:128,133, not macro-routed) to _snprintf_s directly, matching CodeQL's suggested replacement - both already discard the return value so no return-semantics concern there.
CodeQL cpp/drivers/extended-deprecated-apis flagged 29 strncpy calls. Unlike _snprintf, strncpy is not behind any macro - every site calls it directly, and it's a genuine kernel CRT export (confirmed via dumpbin on ntoskrnl.lib), so this is a real per-site fix. Audited every site for the two things that must not change: - Return value: discarded everywhere ((void)-cast or bare statement), so the replacement's return type is a non-issue at every site. - Output buffer: strncpy's lesser-known behavior is zero-filling the entire remainder of the buffer when the source is shorter, not just appending one terminator. No site depends on that - every one treats the destination purely as a NUL-terminated string, never a fixed-width blob. strlcpy is the semantically closest safe replacement (always terminates, doesn't zero-pad, return value already known unused) but has no kernel-linkable implementation here (declared in sunddi.h, never given a body compiled into the driver - same "declared but unavailable" trap _vscprintf was). Added spl_snprintf's sibling, spl_strlcpy, mirroring lib/libspl/strlcpy.c's existing user-mode algorithm (strlen+memcpy+explicit terminator) exactly. The one thing that actually risked changing output content: strncpy's count argument means "copy at most N bytes" while strlcpy's means "the destination buffer is N bytes total" - different contracts. Most sites already pass the true buffer size (safe to reuse as-is), but a few pass buffer-size-minus-one (spl-kmem.c cache_name, spl-kstat.c kstat_set_string - the classic "reserve the terminator byte" idiom) or an exact computed substring length with a manual dst[n]='\0' right after (dsl_dir.c's getcomponent() x2, dsl_prop.c, zcp_get.c - "copy exactly this many chars into a larger buffer"). Passing those verbatim to spl_strlcpy would have silently dropped the last character at each such site. Adjusted the count argument at those 6 sites to match spl_strlcpy's contract instead of reusing strncpy's old value, and removed the now-redundant manual terminator lines where spl_strlcpy's own termination lands at the same index. 17 of the 29 sites (module/icp/core/kcf_mech_tabs.c's mechanism-table registration, all identical: short compile-time string literal into a generously-sized fixed field, return value discarded) are risk-free by construction - truncation was never reachable there either way.
CodeQL cpp/drivers/extended-deprecated-apis flagged 14 vsnprintf calls, all via `#define vsnprintf _vsnprintf` in types.h - the sibling of the snprintf macro already fixed. Same fix, already-built shim: retarget to spl_vsnprintf (added in the _snprintf commit), no new code needed. Audited all 14 call sites the same way as _snprintf/strncpy. Safe everywhere - nothing depends on the old -1-on-truncation return value or on the buffer being left unterminated. Three sites get a genuine, intentional correctness fix as a result, same family as the _snprintf-driven dmu_redact.c/zcp_iter.c fixes: - spl-kstat.c:312 sbuf_vprintf's grow loop (`while (len > SBUF_FREESPACE(s) && sbuf_extend(...) == 0)` then `s->s_len += min(len, SBUF_FREESPACE(s))`). Today, truncation makes len=-1, so the loop never grows the buffer and s_len gets decremented by 1, corrupting sbuf state instead of extending it as SBUF_AUTOEXTEND callers expect. - spl-kmem.c:1888 kmem_dumppr (used by kmem_dump_finish): `n = vsnprintf(p, e-p, ...); *pp = p + n;`. Today, truncation makes n=-1, walking the output pointer backward instead of stopping at the buffer end. - spa_misc.c:403/418 (spa_load_failed/spa_load_note), vdev.c:148 (vdev_dbgmsg), zio.c:932 (zfs_blkptr_verify_log): all four format into an uninitialized `char buf[256]` then read it back via %s. Today, a formatted message >=256 chars leaves buf unterminated, and the %s read walks off the stack buffer - a real stack over-read risk. Now always terminated. _vsnprintf (5 findings: spl-err.c, kmem_asprintf's 2 direct calls, zcp.c, and spl_vsnprintf's own internal NULL,0-sizing call) is untouched - separate commit, and the shim's own call can't be fixed the same way since it's the mechanism the shim depends on.
…am_parse
CodeQL cpp/drivers/extended-deprecated-apis flagged 7 strcat calls.
strcat has no size parameter at all, so any safe replacement needs the
caller to state the destination buffer's true capacity - something
raw strcat never required. strcat_s is linkable but returns errno_t
and aborts on overflow without _TRUNCATE; the String{Cb,Cch}Cat family
only exists as Rtl-prefixed kernel-header inlines; strlcat is
semantically closest but not kernel-linkable (declared in sunddi.h,
no body - same situation strlcpy was in). Added spl_strlcat, mirroring
lib/libspl/strlcat.c's existing algorithm exactly.
6 of 7 sites are safe-by-construction (gzio.c: buffer allocated to the
exact combined size of both appends; zfs_ctldir.c: existing pre-check
already bounds both appends together before either runs; dmu_send.c:
buffer padded exactly for the fixed "/%recv" suffix) - converting them
is a straight swap, supplying the buffer size each already knows.
zfs_vnops_windows.c:300 (stream_parse()) is a real, pre-existing
buffer overflow, unrelated to deprecation and not something to just
paper over: *streamname aliases the tail of the shared 1024-byte
(PATH_MAX) filename allocation - a substring of user-controlled
FileObject->FileName content - and strcat appended ":$DATA" (7 bytes)
there with no check that room remained. A filename close to PATH_MAX
containing exactly one colon could overflow the heap allocation by up
to 6 bytes. Fixed properly: compute the actual remaining room and use
spl_strlcat's return value to detect a would-be overflow, returning
ENAMETOOLONG - the same idiom already established in this codebase
(zfs_ctldir.c, dmu_redact.c, zcp_iter.c) for "this doesn't fit". The
single caller already does generic `if (error) return
STATUS_INVALID_PARAMETER`, so the new error path needed no caller
change.
CodeQL cpp/drivers/extended-deprecated-apis flagged the direct _vsnprintf calls not already covered by the vsnprintf macro fix. (A 5th, zcp.c:1247, turned out to already be resolved: it calls the portable vsnprintf() macro, which the earlier commit retargeted to spl_vsnprintf - the CodeQL scan that still listed it predates that commit.) kmem_asprintf's pair (spl-kmem.c:6624/6630, sizing call then real write) is provably identical, not just safe: size is computed as exactly the true formatted length + 1, so the real write never truncates on either the old or new path - same bytes written, same (discarded) return. vcmn_err (spl-err.c:39) is identical in the common case, but not in one: if a formatted message reaches the 255-byte cap, today's raw _vsnprintf fills the buffer without a NUL terminator (undefined content beyond it), while spl_vsnprintf always terminates. Calling this out explicitly rather than treating it as "safe" - the old behavior in that case is an unterminated-buffer bug, not a contract worth preserving, and the fix is the same class as the char buf[256] fixes already made in the vsnprintf commit (spa_misc.c/vdev.c/zio.c).
CodeQL cpp/drivers/extended-deprecated-apis flagged 5 sprintf calls. sprintf has no size parameter at all (same shape as strcat) - any safe replacement needs the caller to state a destination size that raw sprintf never required. sprintf_s is linkable here but returns errno_t and aborts via the invalid-parameter handler on overflow, no truncate option. spl_snprintf (already built for the _snprintf/vsnprintf fixes) returns the same "chars written" int contract as plain sprintf in the non-truncating case, so reused it directly - no new shim needed. Verified, not assumed, that all 5 sites never actually reach the truncating case, so spl_snprintf's output and return value are provably identical to sprintf's at every site: - gzio.c:237: "<fd:%d>" on a real int, <=16 chars into a 46-byte buf. - spl-kstat.c:1037: "%s%d" on a string already capped to <=254 bytes by kstat_set_string's own spl_strlcpy, plus an int (<=11 digits), into a 271-byte buf. - zfs_windows_zvol_scsi.c:516: "%.04d-%.04d-%.04d" on 3 UCHAR fields (always exactly 14 chars) into a 20-byte WDK INQUIRYDATA field. - zfs_fletcher.c:903/908: a chained cnt-accumulating pair (Linux module_param_call "get" callback contract: buffer is PAGE_SIZE, per include/os/linux/kernel/linux/mod_compat.h). Moot either way - ZFS_MODULE_VIRTUAL_PARAM_CALL expands to nothing on Windows (include/os/windows/spl/sys/mod_os.h), so this function is compiled but never invoked on this platform. Sized to PAGE_SIZE - cnt to match the real (non-Windows) contract this code was written for, and spl_snprintf's return preserves the accumulator chain exactly in case it's ever wired up.
CodeQL cpp/drivers/extended-deprecated-apis flagged 3 strcpy calls. Same shape as strcat (no size parameter at all). strcpy_s is genuinely linkable here (re-verified directly via dumpbin after an earlier draft pass wrongly claimed otherwise) but returns errno_t and aborts on overflow without _TRUNCATE - same tradeoff that ruled it out for strncpy/strcat. Reused spl_strlcpy (already built) instead. All 3 sites verified return-value-unused and overflow-unreachable: - gzio.c:135 and :1097: both destinations are ALLOC'd to the exact fit for what gets copied (gzio.c:1097 reuses the identical size expression the adjacent, already-fixed spl_strlcat calls use). - zfs_ioctl_os.c:188: source is always one of a fixed set of short literals from spa_state_to_name() (max 13 chars) into a 256-byte buffer.
CodeQL cpp/drivers/extended-deprecated-apis flagged this call by its
source text, but include/os/windows/zfs/sys/zfs_context_os.h:72 has
had `#define sscanf sscanf_s` in scope here all along (its own
comment: "until we can get rid of it from lua"). Confirmed via
dumpbin that plain sscanf isn't even exported by ntoskrnl.lib in this
kernel build - only sscanf_s is - so the macro is load-bearing for
linking, not just a style choice. This call already compiled as
sscanf_s(...) before this change; renaming the source text to match
is a zero-behavior-change fix, not a mechanical swap to something
different.
No %s/%c/%[ conversions in the format string ("%lld%n", into a
long long* and an int*), so no additional size arguments are needed
even under sscanf_s's real secure-CRT contract - confirmed no
format-string overflow risk either way. Confirmed this is the only
sscanf call site in the traced build.
CodeQL cpp/drivers/extended-deprecated-apis flagged this one wide-
character printf call. _snwprintf shares _snprintf's exact quirks
(same WDK macro/annotations: -1 on truncation, no NUL-termination on
truncation, count in wchar_t units) - no _vscwprintf (wide analog of
the unavailable _vscprintf) exists here either.
Verified rather than assumed: the destination is a 50-wchar_t buffer,
and the source (a fixed-format GUID string, always exactly 36 chars,
not input-controlled) plus the literal "\??\Volume{" + "}" always
produces exactly 48 chars + NUL = 49, never reaching the 50-wchar_t
cap - truncation is not reachable. The return value was already dead
(assigned to `len`, a void function, never read).
Used RtlStringCchPrintfW directly (ntstrsafe.h, already transitively
included, header-inline, always NUL-terminates) rather than building
a parallel spl_snwprintf shim - this is the only wide-printf finding
in the whole scan, and with truncation unreachable and the return
value already unused, a new shim family isn't justified for one
one-off call site. Dropped the now-pointless `int len =` (an NTSTATUS
doesn't fit that name/type anyway) in favor of a bare (void) call.
…directly The earlier vsnprintf commit (35c953b) retargeted #define vsnprintf from _vsnprintf to spl_vsnprintf - a real runtime improvement, but a fresh CodeQL run showed all 15 vsnprintf findings unchanged, exactly as before that commit. Root cause, found by reading the actual query source (ExtendedDeprecatedApis.ql): it flags macro invocations by the macro's own name against Microsoft's banned-API list, independent of what the macro expands to. "vsnprintf" (no underscore) is on that list; "snprintf" is not - only "_snprintf" is. That's why the sibling snprintf fix cleared its finding and this one couldn't: a macro named vsnprintf can never pass the check regardless of target. (This also retroactively explains why the repo's pre-existing `#define sscanf sscanf_s` never cleared that finding either, before being fixed in this same series by bypassing the macro entirely - same mechanism, same fix shape.) Fix: removed the vsnprintf macro from types.h and renamed the call site text at all 15 locations to spl_vsnprintf directly, so none of them invoke a same-named macro anymore. Verified the include chain for each of the 14 non-gzio.c sites (not assumed) to confirm they already resolved to spl_vsnprintf via types.h, making those renames a provable preprocessor-level no-op - object-like macro substitution is lexical token replacement, so vsnprintf(a,b,c,d) under the macro and spl_vsnprintf(a,b,c,d) written directly compile to identical code. gzio.c:669 is genuinely different, not a no-op: gzio.c only includes <stdio.h> and zutil.h's own chain, never reaching types.h, so it's been governed the whole time by zutil.h's own separately-guarded `#define vsnprintf _vsnprintf` (guarded by #if !defined(vsnprintf), which is why it never conflicted with the definition in types.h - the two never coexist in the same translation unit). The earlier vsnprintf commit never touched this site's actual behavior at all; this is a first-time fix here. Verified safe via gzprintf's own bounds check three lines down - `if (len<=0 || len>=sizeof(buf) || buf[sizeof(buf)-1]!=0) return (0);` - which catches truncation identically either way (old raw _vsnprintf's -1 via len<=0; new spl_vsnprintf's true-length-on-truncation via len>=sizeof(buf)), so gzprintf's return value to its own caller is preserved in every case. Also confirmed gzio.c:665 (a second vsnprintf call, inside #ifdef HAS_vsnprintf_void) is dead/uncompiled code in this build - correctly not one of the 15 findings, needs no change.
The previous commit (fd4bcd5) used a replace_all matching the substring "vsnprintf(" to rename plain vsnprintf() calls to spl_vsnprintf() in this file. It also matched inside the two calls already correctly renamed to spl_vsnprintf() by an earlier commit (kmem_asprintf, lines 6624/6630) - "vsnprintf(" is a substring of "spl_vsnprintf(", so those became spl_spl_vsnprintf(), an undefined symbol. Caused LNK2019: unresolved external symbol spl_spl_vsnprintf. Reverted those 2 lines back to spl_vsnprintf. Lines 1888/6648/6652 (that commit's actual, correct targets) were unaffected and remain correct. Checked the other two files that commit used replace_all on (zfs_debug.c, spa_misc.c) - both clean, neither had pre-existing spl_vsnprintf text for the pattern to collide with.
libzpool/libicp (linked into zdb.exe and other user-mode tools) are separate CMake targets that compile the same module/zfs/module/icp/ module/lua source files independently, with their own include search order - #include <sys/types.h> resolves to lib/libspl/include/os/windows/sys/types.h there, not the kernel driver's include/os/windows/spl/sys/types.h where every spl_vsnprintf/ spl_snprintf/spl_strlcpy/spl_strlcat shim in this series lives. Fixes that stayed behind a portable macro (#define snprintf spl_snprintf) were safe for user-mode by accident, since that macro only exists in the kernel header - plain snprintf()/vsnprintf() calls in shared source just hit the real UCRT functions directly there. But every fix that hardcoded a direct call to spl_* by name (strncpy->spl_strlcpy, strcat->spl_strlcat, strcpy->spl_strlcpy, and the vsnprintf-macro-removal commit) put a reference to a kernel-only symbol into files also compiled for user mode, where nothing defines it. Compiled fine (implicit-function-declaration is a warning, not an error, under this clang-cl configuration), failed only at link time - exactly the zdb.exe LNK2001 errors reported, for a subset of the affected files (static libraries only pull in .obj members the specific target's call graph actually reaches, so the reported list undercounts - dmu_send.c, gzio.c, zcp_get.c, and the os/windows/zfs zfs_debug.c are also affected, confirmed by cross-referencing every touched file against the full libzpool/libicp/zlib source lists). User mode already has everything needed to make this simple: real strlcpy/strlcat (lib/libspl/strlcpy.c, strlcat.c, already linked into these targets) and real, C99-conformant UCRT vsnprintf need no downlevel workaround the way the kernel versions did. Added thin passthrough wrappers under the same names to lib/libspl/include/os/windows/sys/types.h, so both build contexts now resolve these symbols. Kernel-only files (confirmed not present in libzpool/libicp/zlib's source lists) are unaffected.
zdb.exe/zfs.exe/zstreamdump.exe failed to link in x64-Debug with unresolved externals (__imp__time64, __imp_fgets, __imp_fseek, etc.) from inside libcrypto_static.lib - a CRT-linkage mismatch, not anything in the OpenZFS source tree. Unrelated to the CodeQL deprecated-API work in prior commits. Root cause: lib/libzfs/CMakeLists.txt's own find_library( CRYPTO_STATIC_TEST NAMES libcrypto64MTd HINTS ".../VC/static") had a stale CMake cache entry pointing at libcrypto_static.lib - a value that could only have come from a search that ran before this NAMES/HINTS combination existed, since a fresh search with these exact arguments does find libcrypto64MTd.lib (confirmed via the sibling LIB_EAY_DEBUG cache entry, populated by find_package(OpenSSL) using the same search path, which correctly resolves to it). find_library never re-searches once a cache entry exists, so it stayed stuck on the wrong file. That value then reached zdb/zfs/ zstreamdump's link lines because they all link libzfs, and CMake forwards a static library's link dependencies to the final executable regardless of PUBLIC/PRIVATE. Also fixed a second, independent bug in the same spot: the search was hardcoded to the MTd (debug) name regardless of CMAKE_BUILD_TYPE, so even with a fresh cache a true Release build would have linked the debug-CRT crypto lib. Branched the search on CMAKE_BUILD_TYPE (matching the ISA-L pattern already used in the root CMakeLists.txt) and renamed the cache variable to LIBZFS_OPENSSL_CRYPTO, so the fix self-activates on the next configure without needing the cache manually cleared. Removed the leftover CMAKE_FIND_DEBUG_MODE/variable_watch debugging cruft sitting in the same block.
ExAllocatePoolZero only actually zeroes memory through a fallback (RtlZeroMemory) that the WDK header compiles in only when POOL_ZERO_DOWN_LEVEL_SUPPORT is defined - which this driver never defines. Without it, ExAllocatePoolZero reduces to ExAllocatePoolWithTag(PoolType | POOL_ZERO_ALLOCATION, ...) with no fallback, and whether the result is actually zeroed depends entirely on undocumented, OS-build-dependent behavior of the running kernel. This caused a real BSOD: zvContextArray (zfs_windows_zvol.c) came back full of garbage instead of zeros on at least one real machine, so wzvol_find_target() treated an unused slot as if it held a live zvol and dereferenced a garbage pIoRemLock pointer in IoAcquireRemoveLock. Fix: swap ExAllocatePoolZero for ExAllocatePoolUninitialized (still clears the CodeQL deprecated-API finding) and zero explicitly via RtlZeroMemory at every site that needs it, so zeroing is guaranteed by our own code instead of assumed from kernel behavior. Using Uninitialized instead of Zero also avoids doing the zero-fill twice on kernels that do happen to honor the flag natively. Sites fixed: debug.c's cbuf, zfs_vnops_windows.c's rpb/pnp_query_id buffer/BufferUserBuffer's SystemBuffer, zfs_vnops_windows_mount.c's targetName/point, zfs_windows_zvol.c's zvContextArray and the three wzvol_HwReport*'s pWnode, and zfs_windows_zvol_scsi.c's pLUMPIOExt and DiReadWriteSetup's pWkRtnParms (the latter previously only partially zeroed - now fully covered by the allocator swap). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
__dprintf computes size as the exact number of bytes needed for
"prefix + formatted fmt content + one shared NUL terminator" and
allocates exactly that many bytes via kmem_alloc(size, ...). But the
two writes into that buffer were told the buffer was one byte larger
than it actually is:
i = snprintf(buf, size + 1, ...);
roger = spl_vsnprintf(buf + i, size - i + 1, fmt, adx);
Both _vsnprintf_s (called internally by spl_vsnprintf) and the CRT's
snprintf never write past the capacity they are told, so this
overstatement is harmless as long as every measurement of the
required length agrees exactly - which is the normal case. But
spl_vsnprintf measures the required length with plain
_vsnprintf(NULL, 0, ...) and then does the real write with
_vsnprintf_s, a different CRT entry point. If those two ever disagree
on the length needed for the same fmt/args by even one character, the
phantom "+1" gives that extra character room to land one byte past
the kmem_alloc'd buffer, corrupting whatever sits right after it in
the heap.
This matches a real BSOD: a zfs_dbgmsg_t entry (allocated/freed via
this same debug-logging path, hot off dbuf_create's dprintf calls)
turned up with a garbage zdm_size field, causing vmem_hash_delete to
panic on a "bad free" when the driver later tried to purge it.
Fix: drop the phantom "+1" in both capacity arguments so they match
the true kmem_alloc(size, ...) allocation exactly. Verified this does
not change output in the normal (measurements-agree) case - it only
removes the incorrect capacity claim that had no safety margin left
for the two CRT calls to disagree.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… findings
The one remaining cpp/drivers/extended-deprecated-apis finding was the
raw _vsnprintf(NULL, 0, ...) call inside spl_vsnprintf itself, used to
measure a formatted string's true length before the real, bounds-
checked write via _vsnprintf_s. No WDK-safe replacement exists for
this specific "measure without a real destination buffer" need -
ntstrsafe.h's StringCch*/RtlStringCchPrintfEx family short-circuits
before formatting even runs when told cchDest=0, and this WDK's own
implementation of that family falls back to the same raw _vsnprintf
internally.
spl_vsnprintf is now implemented out-of-line in spl-kmem.c (an extern
function, no longer static inline in types.h) as three tiers, only
the last of which can allocate or fail:
1. Try the caller's own real buffer via _vsnprintf_s directly.
2. A small on-stack probe (256 bytes) - covers virtually every real
caller in this tree (the one intentionally-unbounded exception,
Lua channel-program formatting, falls through to tier 3).
3. A kmem_alloc-based grow-and-retry loop, only reached when even
the stack probe truncates.
It could not stay a static inline in types.h: sys/kmem.h itself
#includes sys/types.h, so kmem_alloc's declaration can never be
visible at the point types.h would define it inline, in any include
order.
A code review of this design (and of the companion ExAllocatePoolZero
fix from the previous commit) surfaced further issues, fixed here:
- Tier 3's kmem_alloc(KM_SLEEP) could block-allocate with no IRQL
check anywhere in the function, and at least one caller
(vcmn_err) has no guard of its own - a new IRQL_NOT_LESS_OR_EQUAL
risk the old raw-_vsnprintf-based code never had, since it never
allocated. Tier 3 now checks KeGetCurrentIrql() explicitly and
returns -1 rather than risk it - fixed once, at the one place
that needs it, protecting every current and future caller.
- _vsnprintf_s's -1 return is ambiguous (truncation vs. a genuine
format/invalid-parameter error per MSDN); the old retry loop
treated every -1 as "needs more room" and, on exhausting its
1 MiB cap, returned the fabricated constant SPL_VSNPRINTF_PROBE_MAX
as if it were a real length. It now returns an honest -1 instead.
- kmem_asprintf is hardened to match: without this, a negative
measuring-call return would compute size=0, and
kmem_alloc(0, KM_SLEEP) returns the sentinel KMEM_ZERO_SIZE_PTR
((void*)16) - kmem_asprintf would have hand back that wild
pointer as if it were a valid heap string. It now returns NULL.
- __dprintf and sbuf_vprintf get one-line defensive clamps against
a negative spl_vsnprintf() return flowing into a kmem_alloc size
or signed length accounting, respectively. sbuf_vprintf's is
confirmed dead code today (no live caller of sbuf_new/sbuf_printf/
sbuf_vprintf/sbuf_hexdump exists in this tree) but cheap enough to
fix now rather than leave as a landmine.
Every live caller of spl_vsnprintf/spl_snprintf/snprintf in the tree
was individually audited to confirm this preserves each one's exact
return-value contract, or - for the small number of realistically
unreachable edge cases (e.g. zcp_args_error's VERIFY3U panicking
instead of formatting a truncated Lua error message if a single
argument ever needs >=512 bytes) - degrades safely (a controlled
crash, never memory corruption) rather than silently.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit's ExAllocatePoolZero -> ExAllocatePoolUninitialized + RtlZeroMemory fix was applied by hand at 12 call sites. A code review flagged two problems with that: two sites (zvol_start's zvContextArray, pnp_query_id's Irp->IoStatus.Information) recomputed the allocation-size expression a second time for the RtlZeroMemory call instead of storing it once - a drift risk if one copy is edited without the other - and, more broadly, nothing stops a future ExAllocatePoolZero call anywhere in the tree from reintroducing the exact unreliable-implicit-zero BSOD this series exists to fix, since the fix lived at each call site instead of behind one name. Add spl_ExAllocatePoolZero(PoolType, Size, Tag) to sys/kmem.h, matching this codebase's existing MALLOC/FREE macro precedent for centralizing a raw WDK allocator call, but as a real static inline function rather than a macro: a macro referencing its Size argument twice would silently reintroduce the same double-evaluation bug for any future caller passing a computed expression. Its return value is byte-identical to what every call site already receives today (NULL, or a valid already-zeroed pointer), so every existing cast and NULL-check continues to compile and behave identically, unchanged. Applied at 11 of the 12 sites, collapsing each to a single-line call. The zvContextArray site additionally gets a named local for its size expression, computed once, as extra insurance against the multiplication ever being retyped again. The 12th site (DiReadWriteSetup) is left untouched: it deliberately zeros less than it allocates (the trailing IoSizeofWorkItem() region is reserved for IoInitializeWorkItem() to fill), which spl_ExAllocatePoolZero's single Size parameter cannot express without lying about its own contract. Added a comment there explaining why. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
datacore-PankajSharma
approved these changes
Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New Codeql changes .
Jira : SSV-26770 , SSV-26896 , SSV-26838