From ae95823935838714772554582b61da0e8784ffd2 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 10:16:44 +0530 Subject: [PATCH 01/20] Replace deprecated ExAllocatePoolWithTag with downlevel-safe wrappers 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. --- include/os/windows/spl/sys/kmem.h | 2 +- module/os/windows/debug.c | 2 +- module/os/windows/driver.c | 2 +- module/os/windows/spl/spl-kstat.c | 2 +- module/os/windows/spl/spl-proc_list.c | 2 +- module/os/windows/spl/spl-seg_kmem.c | 2 +- module/os/windows/spl/spl-windows.c | 4 ++-- module/os/windows/zfs/zfs_vnops_windows.c | 4 ++-- module/os/windows/zfs/zfs_vnops_windows_lib.c | 8 ++++---- module/os/windows/zfs/zfs_windows_zvol.c | 19 ++++--------------- module/os/windows/zfs/zfs_windows_zvol_scsi.c | 8 +++----- 11 files changed, 21 insertions(+), 34 deletions(-) diff --git a/include/os/windows/spl/sys/kmem.h b/include/os/windows/spl/sys/kmem.h index f457df156439..85725ca5a1e2 100644 --- a/include/os/windows/spl/sys/kmem.h +++ b/include/os/windows/spl/sys/kmem.h @@ -61,7 +61,7 @@ extern uint64_t physmem; */ #define MALLOC(A, C, S, T, F) \ - (A) = (C)ExAllocatePoolWithTag(NonPagedPoolNx, (S), '!SFZ') + (A) = (C)ExAllocatePoolUninitialized(NonPagedPoolNx, (S), '!SFZ') #define FREE(A, T) \ ExFreePoolWithTag((A), '!SFZ') diff --git a/module/os/windows/debug.c b/module/os/windows/debug.c index d97637e44861..6d5d73ed8caa 100644 --- a/module/os/windows/debug.c +++ b/module/os/windows/debug.c @@ -51,7 +51,7 @@ static unsigned long long startOff = 0; int initDbgCircularBuffer(void) { - cbuf = ExAllocatePoolWithTag(NonPagedPoolNx, cbuf_size, '!GBD'); + cbuf = ExAllocatePoolZero(NonPagedPoolNx, cbuf_size, '!GBD'); ASSERT(cbuf); KeInitializeSpinLock(&cbuf_spin); return (0); diff --git a/module/os/windows/driver.c b/module/os/windows/driver.c index 5873fae229ef..995e27a636d2 100644 --- a/module/os/windows/driver.c +++ b/module/os/windows/driver.c @@ -319,7 +319,7 @@ spl_kstat_registry(void *arg, kstat_t *ksp) break; // Something is wrong - or we finished // Allocate space to hold - regBuffer = (PKEY_VALUE_FULL_INFORMATION)ExAllocatePoolWithTag( + regBuffer = (PKEY_VALUE_FULL_INFORMATION)ExAllocatePoolUninitialized( NonPagedPoolNx, length, 'zfsr'); if (regBuffer == NULL) diff --git a/module/os/windows/spl/spl-kstat.c b/module/os/windows/spl/spl-kstat.c index bd2a2c045b10..9a472b80fa46 100644 --- a/module/os/windows/spl/spl-kstat.c +++ b/module/os/windows/spl/spl-kstat.c @@ -155,7 +155,7 @@ struct sbuf { /* sbuf_new() and family does exist in XNU, but Apple wont let us call them */ #define M_SBUF 105 /* string buffers */ #define SBMALLOC(size) \ - (struct sbuf *)ExAllocatePoolWithTag(NonPagedPoolNx, (size), '!SFZ') + (struct sbuf *)ExAllocatePoolUninitialized(NonPagedPoolNx, (size), '!SFZ') #define SBFREE(buf) ExFreePoolWithTag((buf), '!SFZ') #define SBUF_SETFLAG(s, f) do { (s)->s_flags |= (f); } while (0) diff --git a/module/os/windows/spl/spl-proc_list.c b/module/os/windows/spl/spl-proc_list.c index 1d750447c77c..aeabb33d6015 100644 --- a/module/os/windows/spl/spl-proc_list.c +++ b/module/os/windows/spl/spl-proc_list.c @@ -81,7 +81,7 @@ procfs_list_addr(kstat_t *ksp, loff_t n) ksp->ks_private1 = list_next(&pl->pl_list, elt); if (ksp->ks_private1) { - p = ExAllocatePoolWithTag(NonPagedPoolNx, sizeof (*p), '!SFZ'); + p = ExAllocatePoolUninitialized(NonPagedPoolNx, sizeof (*p), '!SFZ'); p->pli_pl = pl; p->pli_elt = ksp->ks_private1; } diff --git a/module/os/windows/spl/spl-seg_kmem.c b/module/os/windows/spl/spl-seg_kmem.c index 958d7a4f2acb..e4b2f7bdfe4a 100644 --- a/module/os/windows/spl/spl-seg_kmem.c +++ b/module/os/windows/spl/spl-seg_kmem.c @@ -121,7 +121,7 @@ osif_malloc(uint64_t size) #ifdef _KERNEL void *tr = NULL; - tr = ExAllocatePoolWithTag(NonPagedPoolNx, size, '!SFZ'); + tr = ExAllocatePoolUninitialized(NonPagedPoolNx, size, '!SFZ'); ASSERT(P2PHASE(tr, PAGE_SIZE) == 0); if (tr != NULL) { atomic_inc_64(&stat_osif_malloc_success); diff --git a/module/os/windows/spl/spl-windows.c b/module/os/windows/spl/spl-windows.c index 3aabbaa91116..3faa96058587 100644 --- a/module/os/windows/spl/spl-windows.c +++ b/module/os/windows/spl/spl-windows.c @@ -713,7 +713,7 @@ spl_GetZfsTotalMemory(PUNICODE_STRING RegistryPath) break; // Something is wrong - or we finished // Allocate space to hold - regBuffer = (PKEY_VALUE_FULL_INFORMATION)ExAllocatePoolWithTag( + regBuffer = (PKEY_VALUE_FULL_INFORMATION)ExAllocatePoolUninitialized( NonPagedPoolNx, length, 'zfsr'); if (regBuffer == NULL) @@ -801,7 +801,7 @@ spl_getZfsPreallocSize(PUNICODE_STRING RegistryPath) break; // Something is wrong - or we finished // Allocate space to hold - regBuffer = (PKEY_VALUE_FULL_INFORMATION)ExAllocatePoolWithTag( + regBuffer = (PKEY_VALUE_FULL_INFORMATION)ExAllocatePoolUninitialized( NonPagedPoolNx, length, 'zfsr'); if (regBuffer == NULL) diff --git a/module/os/windows/zfs/zfs_vnops_windows.c b/module/os/windows/zfs/zfs_vnops_windows.c index 96b68d138a84..5a5f342813fc 100644 --- a/module/os/windows/zfs/zfs_vnops_windows.c +++ b/module/os/windows/zfs/zfs_vnops_windows.c @@ -419,7 +419,7 @@ zfs_find_dvp_vp(zfsvfs_t *zfsvfs, char *filename, int finalpartmaynotexist, * - maharmstone */ REPARSE_DATA_BUFFER *rpb; - rpb = ExAllocatePoolWithTag(PagedPool, + rpb = ExAllocatePoolZero(PagedPool, zp->z_size, '!FSZ'); zfs_uio_t uio; struct iovec iov = { rpb, zp->z_size }; @@ -1763,7 +1763,7 @@ pnp_query_id(PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp) zmo = (mount_t *)DeviceObject->DeviceExtension; - Irp->IoStatus.Information = (void *)ExAllocatePoolWithTag(PagedPool, + Irp->IoStatus.Information = (void *)ExAllocatePoolZero(PagedPool, zmo->bus_name.Length + sizeof (UNICODE_NULL), '!OIZ'); if (Irp->IoStatus.Information == NULL) return (STATUS_NO_MEMORY); diff --git a/module/os/windows/zfs/zfs_vnops_windows_lib.c b/module/os/windows/zfs/zfs_vnops_windows_lib.c index 402b182b7610..c5f2f2e31d52 100644 --- a/module/os/windows/zfs/zfs_vnops_windows_lib.c +++ b/module/os/windows/zfs/zfs_vnops_windows_lib.c @@ -1559,7 +1559,7 @@ zfs_uid2sid(uint64_t uid, SID **sid) // Root? num = (uid == 0) ? 1 : 2; - tmp = ExAllocatePoolWithTag(PagedPool, + tmp = ExAllocatePoolUninitialized(PagedPool, offsetof(SID, SubAuthority) + (num * sizeof (ULONG)), 'zsid'); tmp->Revision = 1; @@ -1618,7 +1618,7 @@ zfs_gid2sid(uint64_t gid, SID **sid) ASSERT(sid != NULL); - tmp = ExAllocatePoolWithTag(PagedPool, + tmp = ExAllocatePoolUninitialized(PagedPool, offsetof(SID, SubAuthority) + (num * sizeof (ULONG)), 'zsid'); tmp->Revision = 1; @@ -1660,7 +1660,7 @@ zfs_set_acl(dacl *dacls) i++; } - acl = ExAllocatePoolWithTag(PagedPool, size, 'zacl'); + acl = ExAllocatePoolUninitialized(PagedPool, size, 'zacl'); if (!acl) return (NULL); @@ -1726,7 +1726,7 @@ zfs_set_security_root(struct vnode *vp) ASSERT(buflen != 0); - void *tmp = ExAllocatePoolWithTag(PagedPool, buflen, 'ZSEC'); + void *tmp = ExAllocatePoolUninitialized(PagedPool, buflen, 'ZSEC'); if (tmp == NULL) goto err; diff --git a/module/os/windows/zfs/zfs_windows_zvol.c b/module/os/windows/zfs/zfs_windows_zvol.c index d71f3d8810c2..2c85a854bb4e 100644 --- a/module/os/windows/zfs/zfs_windows_zvol.c +++ b/module/os/windows/zfs/zfs_windows_zvol.c @@ -105,18 +105,13 @@ zvol_start(PDRIVER_OBJECT DriverObject, PUNICODE_STRING pRegistryPath) // array. now we can go up to 32,640 zvols. pwzvolDrvInfo->NumberOfBuses = 1; pwzvolDrvInfo->zvContextArray = - (wzvolContext*)ExAllocatePoolWithTag(NonPagedPoolNx, + (wzvolContext*)ExAllocatePoolZero(NonPagedPoolNx, ((SIZE_T)pwzvolDrvInfo->MaximumNumberOfTargets * pwzvolDrvInfo->MaximumNumberOfLogicalUnits * sizeof (wzvolContext)), MP_TAG_GENERAL); if (pwzvolDrvInfo->zvContextArray == NULL) return (STATUS_NO_MEMORY); - RtlZeroMemory(pwzvolDrvInfo->zvContextArray, - ((SIZE_T)pwzvolDrvInfo->MaximumNumberOfTargets * - pwzvolDrvInfo->MaximumNumberOfLogicalUnits * - (sizeof (wzvolContext)))); - RtlZeroMemory(&hwInitData, sizeof (VIRTUAL_HW_INITIALIZATION_DATA)); hwInitData.HwInitializationDataSize = @@ -345,11 +340,9 @@ wzvol_HwReportAdapter(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolWithTag(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { - RtlZeroMemory(pWnode, size); - // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. @@ -445,11 +438,9 @@ wzvol_HwReportLink(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolWithTag(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { - RtlZeroMemory(pWnode, size); - // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. @@ -535,11 +526,9 @@ wzvol_HwReportLog(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolWithTag(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { - RtlZeroMemory(pWnode, size); - // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. diff --git a/module/os/windows/zfs/zfs_windows_zvol_scsi.c b/module/os/windows/zfs/zfs_windows_zvol_scsi.c index d73fbaddaf63..04e2b3a6bf8e 100644 --- a/module/os/windows/zfs/zfs_windows_zvol_scsi.c +++ b/module/os/windows/zfs/zfs_windows_zvol_scsi.c @@ -156,7 +156,7 @@ wzvol_assign_targetid(zvol_state_t *zv) { wzvolContext* zv_targets = STOR_wzvolDriverInfo.zvContextArray; ASSERT(zv->zv_zso->zso_target_context == NULL); - PIO_REMOVE_LOCK pIoRemLock = ExAllocatePoolWithTag(NonPagedPoolNx, + PIO_REMOVE_LOCK pIoRemLock = ExAllocatePoolUninitialized(NonPagedPoolNx, sizeof (*pIoRemLock), MP_TAG_GENERAL); if (!pIoRemLock) { @@ -397,7 +397,7 @@ ScsiGetMPIOExt( } if (pNextEntry == &pHBAExt->pwzvolDrvObj->ListMPIOExt) { - pLUMPIOExt = ExAllocatePoolWithTag(NonPagedPoolNx, + pLUMPIOExt = ExAllocatePoolZero(NonPagedPoolNx, sizeof (HW_LU_EXTENSION_MPIO), MP_TAG_GENERAL); if (!pLUMPIOExt) { @@ -405,8 +405,6 @@ ScsiGetMPIOExt( goto Done; } - RtlZeroMemory(pLUMPIOExt, sizeof (HW_LU_EXTENSION_MPIO)); - pLUMPIOExt->ScsiAddr.PathId = pSrb->PathId; pLUMPIOExt->ScsiAddr.TargetId = pSrb->TargetId; pLUMPIOExt->ScsiAddr.Lun = pSrb->Lun; @@ -1088,7 +1086,7 @@ DiReadWriteSetup(zvol_state_t *zv, MpWkRtnAction action, zfsiodesc_t *pIo) { // cannot use kmem_alloc with sleep if IRQL dispatch so get straight // from NP pool. - pMP_WorkRtnParms pWkRtnParms = (pMP_WorkRtnParms)ExAllocatePoolWithTag( + pMP_WorkRtnParms pWkRtnParms = (pMP_WorkRtnParms)ExAllocatePoolZero( NonPagedPoolNx, ALIGN_UP_BY(sizeof (MP_WorkRtnParms), 16) + IoSizeofWorkItem(), MP_TAG_GENERAL); if (NULL == pWkRtnParms) { From 3f19da4f209c5c0733b904688912ba4837cf4b04 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 10:19:46 +0530 Subject: [PATCH 02/20] Replace deprecated ExAllocatePool with ExAllocatePoolUninitialized/Zero 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. --- module/os/windows/zfs/zfs_vnops_windows.c | 4 ++-- module/os/windows/zfs/zfs_vnops_windows_mount.c | 8 ++------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/module/os/windows/zfs/zfs_vnops_windows.c b/module/os/windows/zfs/zfs_vnops_windows.c index 5a5f342813fc..17ae975df4c0 100644 --- a/module/os/windows/zfs/zfs_vnops_windows.c +++ b/module/os/windows/zfs/zfs_vnops_windows.c @@ -5319,8 +5319,8 @@ _Function_class_(DRIVER_DISPATCH) TargetDeviceRelation) { PDEVICE_RELATIONS DeviceRelations; DeviceRelations = - (PDEVICE_RELATIONS)ExAllocatePool(PagedPool, - sizeof (DEVICE_RELATIONS)); + (PDEVICE_RELATIONS)ExAllocatePoolUninitialized(PagedPool, + sizeof (DEVICE_RELATIONS), '!DRZ'); if (!DeviceRelations) { TraceEvent(TRACE_NOISY, "enomem DeviceRelations\n"); Status = STATUS_INSUFFICIENT_RESOURCES; diff --git a/module/os/windows/zfs/zfs_vnops_windows_mount.c b/module/os/windows/zfs/zfs_vnops_windows_mount.c index 297205e4a456..a687178ba612 100644 --- a/module/os/windows/zfs/zfs_vnops_windows_mount.c +++ b/module/os/windows/zfs/zfs_vnops_windows_mount.c @@ -325,15 +325,13 @@ SendVolumeArrivalNotification(PUNICODE_STRING DeviceName) dprintf("=> SendVolumeArrivalNotification: '%wZ'\n", DeviceName); length = sizeof (MOUNTMGR_TARGET_NAME) + DeviceName->Length - 1; - targetName = ExAllocatePool(PagedPool, length); + targetName = ExAllocatePoolZero(PagedPool, length, 'ZVAN'); if (targetName == NULL) { dprintf(" can't allocate MOUNTMGR_TARGET_NAME\n"); return (STATUS_INSUFFICIENT_RESOURCES); } - RtlZeroMemory(targetName, length); - targetName->DeviceNameLength = DeviceName->Length; RtlCopyMemory(targetName->DeviceName, DeviceName->Buffer, DeviceName->Length); @@ -448,15 +446,13 @@ SendVolumeCreatePoint(__in PUNICODE_STRING DeviceName, length = sizeof (MOUNTMGR_CREATE_POINT_INPUT) + MountPoint->Length + DeviceName->Length; - point = ExAllocatePool(PagedPool, length); + point = ExAllocatePoolZero(PagedPool, length, 'ZVCP'); if (point == NULL) { dprintf(" can't allocate MOUNTMGR_CREATE_POINT_INPUT\n"); return (STATUS_INSUFFICIENT_RESOURCES); } - RtlZeroMemory(point, length); - dprintf(" DeviceName: %wZ\n", DeviceName); point->DeviceNameOffset = sizeof (MOUNTMGR_CREATE_POINT_INPUT); point->DeviceNameLength = DeviceName->Length; From 9db0f913dd858fa377bb67d84338664f782a9c3c Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 10:20:30 +0530 Subject: [PATCH 03/20] Replace deprecated FsRtlAllocatePoolWithQuotaTag with ExAllocatePoolZero 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. --- module/os/windows/zfs/zfs_vnops_windows.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/os/windows/zfs/zfs_vnops_windows.c b/module/os/windows/zfs/zfs_vnops_windows.c index 17ae975df4c0..5edd6110cd5d 100644 --- a/module/os/windows/zfs/zfs_vnops_windows.c +++ b/module/os/windows/zfs/zfs_vnops_windows.c @@ -2281,7 +2281,7 @@ BufferUserBuffer(IN OUT PIRP Irp, IN ULONG BufferLength) if (Irp->AssociatedIrp.SystemBuffer == NULL) { UserBuffer = MapUserBuffer(Irp); Irp->AssociatedIrp.SystemBuffer = - FsRtlAllocatePoolWithQuotaTag(NonPagedPoolNx, + ExAllocatePoolZero(NonPagedPoolNx, BufferLength, 'qtaf'); // From bc0061f975e1fdc1569792148f86b025f9685203 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 11:05:47 +0530 Subject: [PATCH 04/20] Replace deprecated _snprintf with real-semantics snprintf() shim 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. --- include/os/windows/spl/sys/types.h | 35 ++++++++++++++++++++++++++++-- module/os/windows/debug.c | 4 ++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/include/os/windows/spl/sys/types.h b/include/os/windows/spl/sys/types.h index 40cb9b22a3a4..3d1f712e5bba 100644 --- a/include/os/windows/spl/sys/types.h +++ b/include/os/windows/spl/sys/types.h @@ -94,9 +94,40 @@ typedef uintptr_t pc_t; #include #include #include +#include - -#define snprintf _snprintf +/* + * Kernel-mode _snprintf() returns -1 on truncation (not the would-be + * length) and does not NUL-terminate the buffer on truncation, unlike + * standard snprintf(). Portable ZFS/SPL code assumes real snprintf() + * semantics, so give it those semantics here rather than the raw + * deprecated function. No _vscprintf in ntoskrnl.lib - measure via + * _vsnprintf(NULL, 0, ...), which returns the true length for count==0 + * (same idiom kmem_asprintf() already relies on). + */ +static __inline int +spl_vsnprintf(char *buf, size_t size, const char *fmt, va_list args) +{ + va_list args_copy = args; /* x64 MSVC va_list is a plain pointer */ + int needed = _vsnprintf(NULL, 0, fmt, args_copy); + if (size > 0 && buf != NULL) + _vsnprintf_s(buf, size, _TRUNCATE, fmt, args); + return (needed); +} + +static __inline int +spl_snprintf(char *buf, size_t size, const char *fmt, ...) +{ + va_list args; + int ret; + + va_start(args, fmt); + ret = spl_vsnprintf(buf, size, fmt, args); + va_end(args); + return (ret); +} + +#define snprintf spl_snprintf #define vprintf(...) vKdPrintEx((DPFLTR_IHVDRIVER_ID, DPFLTR_INFO_LEVEL, \ __VA_ARGS__)) #define vsnprintf _vsnprintf diff --git a/module/os/windows/debug.c b/module/os/windows/debug.c index 6d5d73ed8caa..595621ea4fb8 100644 --- a/module/os/windows/debug.c +++ b/module/os/windows/debug.c @@ -125,12 +125,12 @@ printBuffer(const char *fmt, ...) va_list args; va_start(args, fmt); char buf[max_line_length]; - _snprintf(buf, 18, "%p: ", PsGetCurrentThread()); + _snprintf_s(buf, sizeof (buf), _TRUNCATE, "%p: ", PsGetCurrentThread()); int tmp = _vsnprintf_s(&buf[17], sizeof (buf), max_line_length, fmt, args); if (tmp >= max_line_length) { - _snprintf(&buf[17], 17, "buffer too small"); + _snprintf_s(&buf[17], 17, _TRUNCATE, "buffer too small"); } KeAcquireSpinLock(&cbuf_spin, &level); From 13f9114b4728908720b6ab4ebc8e8cc1549f4ab4 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 12:21:11 +0530 Subject: [PATCH 05/20] Replace deprecated strncpy with strlen+memcpy-based spl_strlcpy 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. --- include/os/windows/spl/sys/types.h | 24 +++++++++++++++++++++ module/icp/core/kcf_mech_tabs.c | 32 ++++++++++++++-------------- module/icp/spi/kcf_spi.c | 2 +- module/os/windows/spl/spl-kmem.c | 2 +- module/os/windows/spl/spl-kstat.c | 2 +- module/os/windows/spl/spl-taskq.c | 2 +- module/os/windows/zfs/zfs_ioctl_os.c | 2 +- module/zfs/dsl_dir.c | 6 ++---- module/zfs/dsl_prop.c | 6 +++--- module/zfs/spa_misc.c | 2 +- module/zfs/zcp_get.c | 3 +-- module/zfs/zfs_ioctl.c | 2 +- module/zfs/zio_inject.c | 2 +- 13 files changed, 54 insertions(+), 33 deletions(-) diff --git a/include/os/windows/spl/sys/types.h b/include/os/windows/spl/sys/types.h index 3d1f712e5bba..a3f03d2b4547 100644 --- a/include/os/windows/spl/sys/types.h +++ b/include/os/windows/spl/sys/types.h @@ -132,6 +132,30 @@ spl_snprintf(char *buf, size_t size, const char *fmt, ...) __VA_ARGS__)) #define vsnprintf _vsnprintf +/* + * Kernel-mode strncpy() does not NUL-terminate if strlen(src) >= n, + * and zero-fills the whole remainder of the buffer if strlen(src) < n + * - neither behavior is depended on by any of this codebase's callers. + * strlcpy() is the semantically-closest safe replacement (always + * terminates, never overflows) but has no kernel-linkable + * implementation here, so provide one - mirrors lib/libspl/strlcpy.c's + * existing user-mode algorithm exactly. Callers pass n == the size of + * the destination buffer (or the intended-substring-length + 1), + * unlike strncpy's n == copy-length - not a drop-in same-args swap. + */ +static __inline size_t +spl_strlcpy(char *dst, const char *src, size_t dstsize) +{ + size_t srclen = strlen(src); + size_t copied = (srclen < dstsize) ? srclen : dstsize - 1; + + if (dstsize != 0) { + memcpy(dst, src, copied); + dst[copied] = '\0'; + } + return (srclen); +} + #ifndef ULLONG_MAX #define ULLONG_MAX (~0ULL) #endif diff --git a/module/icp/core/kcf_mech_tabs.c b/module/icp/core/kcf_mech_tabs.c index 2642b317d698..d9f185eea8b0 100644 --- a/module/icp/core/kcf_mech_tabs.c +++ b/module/icp/core/kcf_mech_tabs.c @@ -177,72 +177,72 @@ kcf_init_mech_tabs(void) /* Then the pre-defined mechanism entries */ /* Two digests */ - (void) strncpy(kcf_digest_mechs_tab[0].me_name, SUN_CKM_MD5, + (void) spl_strlcpy(kcf_digest_mechs_tab[0].me_name, SUN_CKM_MD5, CRYPTO_MAX_MECH_NAME); kcf_digest_mechs_tab[0].me_threshold = kcf_md5_threshold; - (void) strncpy(kcf_digest_mechs_tab[1].me_name, SUN_CKM_SHA1, + (void) spl_strlcpy(kcf_digest_mechs_tab[1].me_name, SUN_CKM_SHA1, CRYPTO_MAX_MECH_NAME); kcf_digest_mechs_tab[1].me_threshold = kcf_sha1_threshold; /* The symmetric ciphers in various modes */ - (void) strncpy(kcf_cipher_mechs_tab[0].me_name, SUN_CKM_DES_CBC, + (void) spl_strlcpy(kcf_cipher_mechs_tab[0].me_name, SUN_CKM_DES_CBC, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[0].me_threshold = kcf_des_threshold; - (void) strncpy(kcf_cipher_mechs_tab[1].me_name, SUN_CKM_DES3_CBC, + (void) spl_strlcpy(kcf_cipher_mechs_tab[1].me_name, SUN_CKM_DES3_CBC, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[1].me_threshold = kcf_des3_threshold; - (void) strncpy(kcf_cipher_mechs_tab[2].me_name, SUN_CKM_DES_ECB, + (void) spl_strlcpy(kcf_cipher_mechs_tab[2].me_name, SUN_CKM_DES_ECB, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[2].me_threshold = kcf_des_threshold; - (void) strncpy(kcf_cipher_mechs_tab[3].me_name, SUN_CKM_DES3_ECB, + (void) spl_strlcpy(kcf_cipher_mechs_tab[3].me_name, SUN_CKM_DES3_ECB, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[3].me_threshold = kcf_des3_threshold; - (void) strncpy(kcf_cipher_mechs_tab[4].me_name, SUN_CKM_BLOWFISH_CBC, + (void) spl_strlcpy(kcf_cipher_mechs_tab[4].me_name, SUN_CKM_BLOWFISH_CBC, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[4].me_threshold = kcf_bf_threshold; - (void) strncpy(kcf_cipher_mechs_tab[5].me_name, SUN_CKM_BLOWFISH_ECB, + (void) spl_strlcpy(kcf_cipher_mechs_tab[5].me_name, SUN_CKM_BLOWFISH_ECB, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[5].me_threshold = kcf_bf_threshold; - (void) strncpy(kcf_cipher_mechs_tab[6].me_name, SUN_CKM_AES_CBC, + (void) spl_strlcpy(kcf_cipher_mechs_tab[6].me_name, SUN_CKM_AES_CBC, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[6].me_threshold = kcf_aes_threshold; - (void) strncpy(kcf_cipher_mechs_tab[7].me_name, SUN_CKM_AES_ECB, + (void) spl_strlcpy(kcf_cipher_mechs_tab[7].me_name, SUN_CKM_AES_ECB, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[7].me_threshold = kcf_aes_threshold; - (void) strncpy(kcf_cipher_mechs_tab[8].me_name, SUN_CKM_RC4, + (void) spl_strlcpy(kcf_cipher_mechs_tab[8].me_name, SUN_CKM_RC4, CRYPTO_MAX_MECH_NAME); kcf_cipher_mechs_tab[8].me_threshold = kcf_rc4_threshold; /* 4 HMACs */ - (void) strncpy(kcf_mac_mechs_tab[0].me_name, SUN_CKM_MD5_HMAC, + (void) spl_strlcpy(kcf_mac_mechs_tab[0].me_name, SUN_CKM_MD5_HMAC, CRYPTO_MAX_MECH_NAME); kcf_mac_mechs_tab[0].me_threshold = kcf_md5_threshold; - (void) strncpy(kcf_mac_mechs_tab[1].me_name, SUN_CKM_MD5_HMAC_GENERAL, + (void) spl_strlcpy(kcf_mac_mechs_tab[1].me_name, SUN_CKM_MD5_HMAC_GENERAL, CRYPTO_MAX_MECH_NAME); kcf_mac_mechs_tab[1].me_threshold = kcf_md5_threshold; - (void) strncpy(kcf_mac_mechs_tab[2].me_name, SUN_CKM_SHA1_HMAC, + (void) spl_strlcpy(kcf_mac_mechs_tab[2].me_name, SUN_CKM_SHA1_HMAC, CRYPTO_MAX_MECH_NAME); kcf_mac_mechs_tab[2].me_threshold = kcf_sha1_threshold; - (void) strncpy(kcf_mac_mechs_tab[3].me_name, SUN_CKM_SHA1_HMAC_GENERAL, + (void) spl_strlcpy(kcf_mac_mechs_tab[3].me_name, SUN_CKM_SHA1_HMAC_GENERAL, CRYPTO_MAX_MECH_NAME); kcf_mac_mechs_tab[3].me_threshold = kcf_sha1_threshold; /* 1 random number generation pseudo mechanism */ - (void) strncpy(kcf_misc_mechs_tab[0].me_name, SUN_RANDOM, + (void) spl_strlcpy(kcf_misc_mechs_tab[0].me_name, SUN_RANDOM, CRYPTO_MAX_MECH_NAME); kcf_mech_hash = mod_hash_create_strhash_nodtr("kcf mech2id hash", diff --git a/module/icp/spi/kcf_spi.c b/module/icp/spi/kcf_spi.c index 34b36b81c0ab..edfb9765002c 100644 --- a/module/icp/spi/kcf_spi.c +++ b/module/icp/spi/kcf_spi.c @@ -606,7 +606,7 @@ init_prov_mechs(crypto_provider_info_t *info, kcf_provider_desc_t *desc) rand_mi = &desc->pd_mechanisms[mcount - 1]; bzero(rand_mi, sizeof (crypto_mech_info_t)); - (void) strncpy(rand_mi->cm_mech_name, SUN_RANDOM, + (void) spl_strlcpy(rand_mi->cm_mech_name, SUN_RANDOM, CRYPTO_MAX_MECH_NAME); rand_mi->cm_func_group_mask = CRYPTO_FG_RANDOM; } else { diff --git a/module/os/windows/spl/spl-kmem.c b/module/os/windows/spl/spl-kmem.c index 54cc47adbae4..10a58ac9c274 100644 --- a/module/os/windows/spl/spl-kmem.c +++ b/module/os/windows/spl/spl-kmem.c @@ -3560,7 +3560,7 @@ kmem_cache_create( /* * Set cache properties. */ - (void) strncpy(cp->cache_name, name, KMEM_CACHE_NAMELEN); + (void) spl_strlcpy(cp->cache_name, name, KMEM_CACHE_NAMELEN + 1); strident_canon(cp->cache_name, KMEM_CACHE_NAMELEN + 1); cp->cache_bufsize = bufsize; cp->cache_align = align; diff --git a/module/os/windows/spl/spl-kstat.c b/module/os/windows/spl/spl-kstat.c index 9a472b80fa46..640f29ec98b5 100644 --- a/module/os/windows/spl/spl-kstat.c +++ b/module/os/windows/spl/spl-kstat.c @@ -785,7 +785,7 @@ void kstat_set_string(char *dst, const char *src) { bzero(dst, KSTAT_STRLEN); - (void) strncpy(dst, src, KSTAT_STRLEN - 1); + (void) spl_strlcpy(dst, src, KSTAT_STRLEN); } void diff --git a/module/os/windows/spl/spl-taskq.c b/module/os/windows/spl/spl-taskq.c index 499485d904c2..a521ea40982a 100644 --- a/module/os/windows/spl/spl-taskq.c +++ b/module/os/windows/spl/spl-taskq.c @@ -2458,7 +2458,7 @@ taskq_create_common(const char *name, int instance, int nthreads, pri_t pri, * Make sure the name is 0-terminated, and conforms to the rules for * C indentifiers */ - (void) strncpy(tq->tq_name, name, TASKQ_NAMELEN + 1); + (void) spl_strlcpy(tq->tq_name, name, TASKQ_NAMELEN + 1); strident_canon(tq->tq_name, TASKQ_NAMELEN + 1); tq->tq_flags = flags | TASKQ_CHANGING; diff --git a/module/os/windows/zfs/zfs_ioctl_os.c b/module/os/windows/zfs/zfs_ioctl_os.c index 64f465b8b5d1..4aa4f95f2fac 100644 --- a/module/os/windows/zfs/zfs_ioctl_os.c +++ b/module/os/windows/zfs/zfs_ioctl_os.c @@ -139,7 +139,7 @@ NTSTATUS zpool_zfs_get_metrics(PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_ perf->zpool_allocated = 0; perf->zpool_size = 0; perf->zfs_volSize = 0; - strncpy(perf->zpoolHealthState, "", sizeof(perf->zpoolHealthState)); + spl_strlcpy(perf->zpoolHealthState, "", sizeof(perf->zpoolHealthState)); perf->l2arc_alloc_size = 0; perf->l2arc_space = 0; perf->special_mirror_alloc_size = 0; diff --git a/module/zfs/dsl_dir.c b/module/zfs/dsl_dir.c index e1c1e5d8ba81..48b4970eb5f7 100644 --- a/module/zfs/dsl_dir.c +++ b/module/zfs/dsl_dir.c @@ -441,8 +441,7 @@ getcomponent(const char *path, char *component, const char **nextp) if (p - path >= ZFS_MAX_DATASET_NAME_LEN) return (SET_ERROR(ENAMETOOLONG)); - (void) strncpy(component, path, p - path); - component[p - path] = '\0'; + (void) spl_strlcpy(component, path, (p - path) + 1); p++; } else if (p[0] == '@') { /* @@ -455,8 +454,7 @@ getcomponent(const char *path, char *component, const char **nextp) if (p - path >= ZFS_MAX_DATASET_NAME_LEN) return (SET_ERROR(ENAMETOOLONG)); - (void) strncpy(component, path, p - path); - component[p - path] = '\0'; + (void) spl_strlcpy(component, path, (p - path) + 1); } else { panic("invalid p=%p", (void *)p); } diff --git a/module/zfs/dsl_prop.c b/module/zfs/dsl_prop.c index ed42ac5aef3c..75afbaf78de3 100644 --- a/module/zfs/dsl_prop.c +++ b/module/zfs/dsl_prop.c @@ -58,7 +58,7 @@ dodefault(zfs_prop_t prop, int intsz, int numints, void *buf) if (intsz != 1) return (SET_ERROR(EOVERFLOW)); - (void) strncpy(buf, zfs_prop_default_string(prop), + (void) spl_strlcpy(buf, zfs_prop_default_string(prop), numints); } else { if (intsz != 8 || numints < 1) @@ -1029,8 +1029,8 @@ dsl_prop_get_all_impl(objset_t *mos, uint64_t propobj, if (flags & DSL_PROP_GET_LOCAL) continue; - (void) strncpy(buf, za.za_name, (suffix - za.za_name)); - buf[suffix - za.za_name] = '\0'; + (void) spl_strlcpy(buf, za.za_name, + (suffix - za.za_name) + 1); propname = buf; if (!(flags & DSL_PROP_GET_RECEIVED)) { diff --git a/module/zfs/spa_misc.c b/module/zfs/spa_misc.c index 904256323143..34915bf12474 100644 --- a/module/zfs/spa_misc.c +++ b/module/zfs/spa_misc.c @@ -1677,7 +1677,7 @@ spa_altroot(spa_t *spa, char *buf, size_t buflen) if (spa->spa_root == NULL) buf[0] = '\0'; else - (void) strncpy(buf, spa->spa_root, buflen); + (void) spl_strlcpy(buf, spa->spa_root, buflen); } int diff --git a/module/zfs/zcp_get.c b/module/zfs/zcp_get.c index 7256e4de1915..1d334b7783bb 100644 --- a/module/zfs/zcp_get.c +++ b/module/zfs/zcp_get.c @@ -611,8 +611,7 @@ parse_userquota_prop(const char *prop_name, zfs_userquota_prop_t *type, */ int domain_len = strrchr(cp, '-') - cp; domain_val = kmem_alloc(domain_len + 1, KM_SLEEP); - (void) strncpy(domain_val, cp, domain_len); - domain_val[domain_len] = '\0'; + (void) spl_strlcpy(domain_val, cp, domain_len + 1); cp += domain_len + 1; (void) ddi_strtoll(cp, &end, 10, (longlong_t *)rid); diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index e7bb4a32f38f..c266aac80a62 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -745,7 +745,7 @@ zfs_get_parent(const char *datasetname, char *parent, int parentsize) /* * Remove the @bla or /bla from the end of the name to get the parent. */ - (void) strncpy(parent, datasetname, parentsize); + (void) spl_strlcpy(parent, datasetname, parentsize); cp = strrchr(parent, '@'); if (cp != NULL) { cp[0] = '\0'; diff --git a/module/zfs/zio_inject.c b/module/zfs/zio_inject.c index f494db4a20ef..aacff6a6142f 100644 --- a/module/zfs/zio_inject.c +++ b/module/zfs/zio_inject.c @@ -894,7 +894,7 @@ zio_inject_list_next(int *id, char *name, size_t buflen, if (handler) { *record = handler->zi_record; *id = handler->zi_id; - (void) strncpy(name, spa_name(handler->zi_spa), buflen); + (void) spl_strlcpy(name, spa_name(handler->zi_spa), buflen); ret = 0; } else { ret = SET_ERROR(ENOENT); From 35c953b1a93cdedd95a03fff76b0eed9afe31e2c Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 13:03:34 +0530 Subject: [PATCH 06/20] Replace deprecated vsnprintf with real-semantics snprintf() shim 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. --- include/os/windows/spl/sys/types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/os/windows/spl/sys/types.h b/include/os/windows/spl/sys/types.h index a3f03d2b4547..292e42bede1a 100644 --- a/include/os/windows/spl/sys/types.h +++ b/include/os/windows/spl/sys/types.h @@ -130,7 +130,7 @@ spl_snprintf(char *buf, size_t size, const char *fmt, ...) #define snprintf spl_snprintf #define vprintf(...) vKdPrintEx((DPFLTR_IHVDRIVER_ID, DPFLTR_INFO_LEVEL, \ __VA_ARGS__)) -#define vsnprintf _vsnprintf +#define vsnprintf spl_vsnprintf /* * Kernel-mode strncpy() does not NUL-terminate if strlen(src) >= n, From 9bc441c4094f6dde17ead31cf9c5b1095855f799 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 13:18:09 +0530 Subject: [PATCH 07/20] Replace deprecated strcat with spl_strlcat, fix real overflow in stream_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. --- include/os/windows/spl/sys/types.h | 26 +++++++++++++++++++++++ lib/os/windows/zlib-1.2.3/gzio.c | 4 ++-- module/os/windows/zfs/zfs_ctldir.c | 4 ++-- module/os/windows/zfs/zfs_vnops_windows.c | 4 +++- module/zfs/dmu_send.c | 4 ++-- 5 files changed, 35 insertions(+), 7 deletions(-) diff --git a/include/os/windows/spl/sys/types.h b/include/os/windows/spl/sys/types.h index 292e42bede1a..9d0d54fb93e6 100644 --- a/include/os/windows/spl/sys/types.h +++ b/include/os/windows/spl/sys/types.h @@ -156,6 +156,32 @@ spl_strlcpy(char *dst, const char *src, size_t dstsize) return (srclen); } +/* + * strcat() has no size parameter at all - unbounded by construction. + * strlcat() is the closest safe replacement (always terminates, never + * overflows, return value is the total length it tried to create) but + * - same as strlcpy() - has no kernel-linkable implementation here. + * Mirrors lib/libspl/strlcat.c's existing user-mode algorithm exactly. + */ +static __inline size_t +spl_strlcat(char *dst, const char *src, size_t dstsize) +{ + char *df = dst; + size_t left = dstsize; + size_t l1, l2 = strlen(src), copied; + + while (left-- != 0 && *df != '\0') + df++; + l1 = df - dst; + if (dstsize == l1) + return (l1 + l2); + + copied = (l1 + l2 >= dstsize) ? dstsize - l1 - 1 : l2; + memcpy(dst + l1, src, copied); + dst[l1 + copied] = '\0'; + return (l1 + l2); +} + #ifndef ULLONG_MAX #define ULLONG_MAX (~0ULL) #endif diff --git a/lib/os/windows/zlib-1.2.3/gzio.c b/lib/os/windows/zlib-1.2.3/gzio.c index 8a8181c80b31..d3b774188804 100644 --- a/lib/os/windows/zlib-1.2.3/gzio.c +++ b/lib/os/windows/zlib-1.2.3/gzio.c @@ -1095,8 +1095,8 @@ gzerror( if (s->msg == Z_NULL) return ((const char *)ERR_MSG(Z_MEM_ERROR)); strcpy(s->msg, s->path); - strcat(s->msg, ": "); - strcat(s->msg, m); + spl_strlcat(s->msg, ": ", strlen(s->path) + strlen(m) + 3); + spl_strlcat(s->msg, m, strlen(s->path) + strlen(m) + 3); return ((const char *)s->msg); } diff --git a/module/os/windows/zfs/zfs_ctldir.c b/module/os/windows/zfs/zfs_ctldir.c index b3fc0875a7e1..488c5f30f87d 100644 --- a/module/os/windows/zfs/zfs_ctldir.c +++ b/module/os/windows/zfs/zfs_ctldir.c @@ -995,8 +995,8 @@ zfsctl_snapshot_name(zfsvfs_t *zfsvfs, const char *snap_name, int len, if ((strlen(full_name) + 1 + strlen(snap_name)) >= len) return (SET_ERROR(ENAMETOOLONG)); - (void) strcat(full_name, "@"); - (void) strcat(full_name, snap_name); + (void) spl_strlcat(full_name, "@", len); + (void) spl_strlcat(full_name, snap_name, len); return (0); } diff --git a/module/os/windows/zfs/zfs_vnops_windows.c b/module/os/windows/zfs/zfs_vnops_windows.c index 5edd6110cd5d..f7c10dba2bf4 100644 --- a/module/os/windows/zfs/zfs_vnops_windows.c +++ b/module/os/windows/zfs/zfs_vnops_windows.c @@ -297,7 +297,9 @@ stream_parse(char *filename, char **streamname) *colon = 0; // Cut off streamname from filename // We now ADD ":$DATA" to the stream name. - strcat(*streamname, ":$DATA"); + size_t remaining = PATH_MAX - (*streamname - filename); + if (spl_strlcat(*streamname, ":$DATA", remaining) >= remaining) + return (SET_ERROR(ENAMETOOLONG)); return (0); } diff --git a/module/zfs/dmu_send.c b/module/zfs/dmu_send.c index 551043fafbfc..31153df201cf 100644 --- a/module/zfs/dmu_send.c +++ b/module/zfs/dmu_send.c @@ -3000,8 +3000,8 @@ dmu_send_estimate_fast(dsl_dataset_t *origds, dsl_dataset_t *fromds, char dsname[ZFS_MAX_DATASET_NAME_LEN + 6]; dsl_dataset_name(origds, dsname); - (void) strcat(dsname, "/"); - (void) strcat(dsname, recv_clone_name); + (void) spl_strlcat(dsname, "/", sizeof (dsname)); + (void) spl_strlcat(dsname, recv_clone_name, sizeof (dsname)); err = dsl_dataset_hold(origds->ds_dir->dd_pool, dsname, FTAG, &ds); From 53924a343ca3a44d847196aee6fbea61de20f7a2 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 13:29:44 +0530 Subject: [PATCH 08/20] Replace deprecated _vsnprintf with spl_vsnprintf 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). --- module/os/windows/spl/spl-err.c | 2 +- module/os/windows/spl/spl-kmem.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/module/os/windows/spl/spl-err.c b/module/os/windows/spl/spl-err.c index f54ff5405e27..d4250bfc51b2 100644 --- a/module/os/windows/spl/spl-err.c +++ b/module/os/windows/spl/spl-err.c @@ -36,7 +36,7 @@ vcmn_err(int ce, const char *fmt, va_list ap) { char msg[MAXMSGLEN]; - _vsnprintf(msg, MAXMSGLEN - 1, fmt, ap); + spl_vsnprintf(msg, MAXMSGLEN - 1, fmt, ap); switch (ce) { case CE_IGNORE: diff --git a/module/os/windows/spl/spl-kmem.c b/module/os/windows/spl/spl-kmem.c index 10a58ac9c274..0304d8bed703 100644 --- a/module/os/windows/spl/spl-kmem.c +++ b/module/os/windows/spl/spl-kmem.c @@ -6621,13 +6621,13 @@ kmem_asprintf(const char *fmt, ...) char *buf; va_start(adx, fmt); - size = _vsnprintf(NULL, 0, fmt, adx) + 1; + size = spl_vsnprintf(NULL, 0, fmt, adx) + 1; va_end(adx); buf = kmem_alloc(size, KM_SLEEP); va_start(adx, fmt); - (void) _vsnprintf(buf, size, fmt, adx); + (void) spl_vsnprintf(buf, size, fmt, adx); va_end(adx); return (buf); From 1f2e2d51187c239224798c397d9794426bb51492 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 13:31:43 +0530 Subject: [PATCH 09/20] Replace deprecated sprintf with spl_snprintf 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: "" 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. --- lib/os/windows/zlib-1.2.3/gzio.c | 2 +- module/os/windows/spl/spl-kstat.c | 3 ++- module/os/windows/zfs/zfs_windows_zvol_scsi.c | 3 ++- module/zcommon/zfs_fletcher.c | 4 ++-- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/os/windows/zlib-1.2.3/gzio.c b/lib/os/windows/zlib-1.2.3/gzio.c index d3b774188804..91b6a00fb950 100644 --- a/lib/os/windows/zlib-1.2.3/gzio.c +++ b/lib/os/windows/zlib-1.2.3/gzio.c @@ -234,7 +234,7 @@ gzdopen( if (fd < 0) return ((gzFile)Z_NULL); - sprintf(name, "", fd); /* for debugging */ + spl_snprintf(name, sizeof (name), "", fd); /* for debugging */ return (gz_open(name, mode, fd)); } diff --git a/module/os/windows/spl/spl-kstat.c b/module/os/windows/spl/spl-kstat.c index 640f29ec98b5..60107acbd450 100644 --- a/module/os/windows/spl/spl-kstat.c +++ b/module/os/windows/spl/spl-kstat.c @@ -1034,7 +1034,8 @@ kstat_create_zone(const char *ks_module, int ks_instance, const char *ks_name, if (ks_name == NULL) { char buf[KSTAT_STRLEN]; kstat_set_string(buf, ks_module); - (void) sprintf(namebuf, "%s%d", buf, ks_instance); + (void) spl_snprintf(namebuf, sizeof (namebuf), "%s%d", buf, + ks_instance); ks_name = namebuf; } diff --git a/module/os/windows/zfs/zfs_windows_zvol_scsi.c b/module/os/windows/zfs/zfs_windows_zvol_scsi.c index 04e2b3a6bf8e..ecc21d6d1350 100644 --- a/module/os/windows/zfs/zfs_windows_zvol_scsi.c +++ b/module/os/windows/zfs/zfs_windows_zvol_scsi.c @@ -513,7 +513,8 @@ ScsiOpInquiry( pHBAExt->ProductRevision, 4); memset((PCHAR)pInqData->VendorSpecific, ' ', sizeof (pInqData->VendorSpecific)); - sprintf(pInqData->VendorSpecific, "%.04d-%.04d-%.04d", + spl_snprintf(pInqData->VendorSpecific, + sizeof (pInqData->VendorSpecific), "%.04d-%.04d-%.04d", pSrb->PathId, pSrb->TargetId, pSrb->Lun); pInqData->VendorSpecific[strlen(pInqData->VendorSpecific)] = ' '; diff --git a/module/zcommon/zfs_fletcher.c b/module/zcommon/zfs_fletcher.c index ffd395474010..00bb44365a5d 100644 --- a/module/zcommon/zfs_fletcher.c +++ b/module/zcommon/zfs_fletcher.c @@ -900,12 +900,12 @@ fletcher_4_param_get(char *buffer, zfs_kernel_param_t *unused) /* list fastest */ fmt = IMPL_FMT(impl, IMPL_FASTEST); - cnt += sprintf(buffer + cnt, fmt, "fastest"); + cnt += spl_snprintf(buffer + cnt, PAGE_SIZE - cnt, fmt, "fastest"); /* list all supported implementations */ for (uint32_t i = 0; i < fletcher_4_supp_impls_cnt; ++i) { fmt = IMPL_FMT(impl, i); - cnt += sprintf(buffer + cnt, fmt, + cnt += spl_snprintf(buffer + cnt, PAGE_SIZE - cnt, fmt, fletcher_4_supp_impls[i]->name); } From 51d57847fe50005dff2f9e418cc2794127815f00 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 13:58:14 +0530 Subject: [PATCH 10/20] Replace deprecated strcpy with spl_strlcpy 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. --- lib/os/windows/zlib-1.2.3/gzio.c | 4 ++-- module/os/windows/zfs/zfs_ioctl_os.c | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/os/windows/zlib-1.2.3/gzio.c b/lib/os/windows/zlib-1.2.3/gzio.c index 91b6a00fb950..90ce63b6e1f6 100644 --- a/lib/os/windows/zlib-1.2.3/gzio.c +++ b/lib/os/windows/zlib-1.2.3/gzio.c @@ -132,7 +132,7 @@ gz_open( if (s->path == NULL) { return (destroy(s), (gzFile)Z_NULL); } - strcpy(s->path, path); /* do this early for debugging */ + spl_strlcpy(s->path, path, strlen(path) + 1); /* do this early for debugging */ s->mode = '\0'; do { @@ -1094,7 +1094,7 @@ gzerror( s->msg = (char *)ALLOC(strlen(s->path) + strlen(m) + 3); if (s->msg == Z_NULL) return ((const char *)ERR_MSG(Z_MEM_ERROR)); - strcpy(s->msg, s->path); + spl_strlcpy(s->msg, s->path, strlen(s->path) + strlen(m) + 3); spl_strlcat(s->msg, ": ", strlen(s->path) + strlen(m) + 3); spl_strlcat(s->msg, m, strlen(s->path) + strlen(m) + 3); return ((const char *)s->msg); diff --git a/module/os/windows/zfs/zfs_ioctl_os.c b/module/os/windows/zfs/zfs_ioctl_os.c index 4aa4f95f2fac..26155c33c8c4 100644 --- a/module/os/windows/zfs/zfs_ioctl_os.c +++ b/module/os/windows/zfs/zfs_ioctl_os.c @@ -185,7 +185,8 @@ NTSTATUS zpool_zfs_get_metrics(PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_ perf->zpool_allocated = pool_alloc; perf->zpool_size = pool_size; - strcpy(perf->zpoolHealthState, healthState); + spl_strlcpy(perf->zpoolHealthState, healthState, + sizeof (perf->zpoolHealthState)); } else perf->zfs_volSize = getZvolSize(perf->name); From 95521b058e146cde71a19956818a8e53a0fa79e4 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 13:58:46 +0530 Subject: [PATCH 11/20] Replace deprecated sscanf with sscanf_s 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. --- module/zfs/zcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/zfs/zcp.c b/module/zfs/zcp.c index 4c8af269ee01..a596c8eedd90 100644 --- a/module/zfs/zcp.c +++ b/module/zfs/zcp.c @@ -261,7 +261,7 @@ zcp_table_to_nvlist(lua_State *state, int index, int depth) /* check if this could collide with a number or bool */ long long tmp; int parselen; - if ((sscanf(key, "%lld%n", &tmp, &parselen) > 0 && + if ((sscanf_s(key, "%lld%n", &tmp, &parselen) > 0 && parselen == strlen(key)) || strcmp(key, "true") == 0 || strcmp(key, "false") == 0) { From 3b230bf9c8684c6edaacf88b4fccb0baae0a7f02 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 13:59:22 +0530 Subject: [PATCH 12/20] Replace deprecated _snwprintf with RtlStringCchPrintfW 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. --- module/os/windows/zfs/zfs_vnops_windows_mount.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/os/windows/zfs/zfs_vnops_windows_mount.c b/module/os/windows/zfs/zfs_vnops_windows_mount.c index a687178ba612..da472dcf2947 100644 --- a/module/os/windows/zfs/zfs_vnops_windows_mount.c +++ b/module/os/windows/zfs/zfs_vnops_windows_mount.c @@ -1008,7 +1008,7 @@ generateVolumeNameMountpoint(wchar_t *vol_mpt) wchar_t wc_guid[50]; generateGUID(&GUID); mbstowcs(&wc_guid, GUID, 50); - int len = _snwprintf(vol_mpt, 50, L"\\??\\Volume{%s}", wc_guid); + (void) RtlStringCchPrintfW(vol_mpt, 50, L"\\??\\Volume{%s}", wc_guid); } int From fd4bcd5c03fc085bde3bdc3f810801887b73327e Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 15:20:31 +0530 Subject: [PATCH 13/20] Fix vsnprintf finding for real: remove the macro, call spl_vsnprintf directly The earlier vsnprintf commit (35c953b1a) 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 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. --- include/os/windows/spl/sys/types.h | 10 +++++++++- lib/os/windows/zlib-1.2.3/gzio.c | 2 +- module/lua/lcompat.c | 2 +- module/lua/lstrlib.c | 2 +- module/os/windows/spl/spl-kmem.c | 10 +++++----- module/os/windows/spl/spl-kstat.c | 2 +- module/os/windows/spl/spl-proc_list.c | 2 +- module/os/windows/zfs/zfs_debug.c | 4 ++-- module/zfs/spa_misc.c | 4 ++-- module/zfs/vdev.c | 2 +- module/zfs/zcp.c | 2 +- module/zfs/zio.c | 2 +- 12 files changed, 26 insertions(+), 18 deletions(-) diff --git a/include/os/windows/spl/sys/types.h b/include/os/windows/spl/sys/types.h index 9d0d54fb93e6..6c2a5e92da18 100644 --- a/include/os/windows/spl/sys/types.h +++ b/include/os/windows/spl/sys/types.h @@ -130,7 +130,15 @@ spl_snprintf(char *buf, size_t size, const char *fmt, ...) #define snprintf spl_snprintf #define vprintf(...) vKdPrintEx((DPFLTR_IHVDRIVER_ID, DPFLTR_INFO_LEVEL, \ __VA_ARGS__)) -#define vsnprintf spl_vsnprintf +/* + * No #define vsnprintf here (unlike snprintf above): CodeQL's + * extended-deprecated-apis check flags macro invocations by the + * macro's own name against Microsoft's banned-API list, regardless of + * what the macro expands to - "vsnprintf" (no underscore) is on that + * list, "snprintf" is not. A macro named vsnprintf can never pass the + * check no matter its target, so every caller below calls + * spl_vsnprintf directly instead of going through a same-named macro. + */ /* * Kernel-mode strncpy() does not NUL-terminate if strlen(src) >= n, diff --git a/lib/os/windows/zlib-1.2.3/gzio.c b/lib/os/windows/zlib-1.2.3/gzio.c index 90ce63b6e1f6..5c7ed4aaf16b 100644 --- a/lib/os/windows/zlib-1.2.3/gzio.c +++ b/lib/os/windows/zlib-1.2.3/gzio.c @@ -666,7 +666,7 @@ gzprintf(gzFile file, const char *format, /* args */ ...) va_end(va); len = strlen(buf); #else - len = vsnprintf(buf, sizeof (buf), format, va); + len = spl_vsnprintf(buf, sizeof (buf), format, va); va_end(va); #endif #endif diff --git a/module/lua/lcompat.c b/module/lua/lcompat.c index c0a27182c7d8..a43ca10c331e 100644 --- a/module/lua/lcompat.c +++ b/module/lua/lcompat.c @@ -12,7 +12,7 @@ lcompat_sprintf(char *buf, size_t size, const char *fmt, ...) va_list args; va_start(args, fmt); - res = vsnprintf(buf, size, fmt, args); + res = spl_vsnprintf(buf, size, fmt, args); va_end(args); return (res); diff --git a/module/lua/lstrlib.c b/module/lua/lstrlib.c index 12027757bf53..13e5941d6f5d 100644 --- a/module/lua/lstrlib.c +++ b/module/lua/lstrlib.c @@ -37,7 +37,7 @@ static size_t str_sprintf(char *buf, const char *fmt, ...) { size_t len; va_start(args, fmt); - len = vsnprintf(buf, INT_MAX, fmt, args); + len = spl_vsnprintf(buf, INT_MAX, fmt, args); va_end(args); return len; diff --git a/module/os/windows/spl/spl-kmem.c b/module/os/windows/spl/spl-kmem.c index 0304d8bed703..62eeb1e88777 100644 --- a/module/os/windows/spl/spl-kmem.c +++ b/module/os/windows/spl/spl-kmem.c @@ -1885,7 +1885,7 @@ kmem_dumppr(char **pp, char *e, const char *format, ...) va_list ap; va_start(ap, format); - n = vsnprintf(p, e - p, format, ap); + n = spl_vsnprintf(p, e - p, format, ap); va_end(ap); *pp = p + n; } @@ -6621,13 +6621,13 @@ kmem_asprintf(const char *fmt, ...) char *buf; va_start(adx, fmt); - size = spl_vsnprintf(NULL, 0, fmt, adx) + 1; + size = spl_spl_vsnprintf(NULL, 0, fmt, adx) + 1; va_end(adx); buf = kmem_alloc(size, KM_SLEEP); va_start(adx, fmt); - (void) spl_vsnprintf(buf, size, fmt, adx); + (void) spl_spl_vsnprintf(buf, size, fmt, adx); va_end(adx); return (buf); @@ -6645,11 +6645,11 @@ kmem_vasprintf(const char *fmt, va_list ap) int size; int r = -1; - size = vsnprintf(NULL, 0, fmt, ap); + size = spl_vsnprintf(NULL, 0, fmt, ap); if ((size >= 0) && (size < INT_MAX)) { ptr = (char *)kmem_alloc(size + 1, KM_SLEEP); // +1 for null if (ptr) { - r = vsnprintf(ptr, size + 1, fmt, ap); // +1 for null + r = spl_vsnprintf(ptr, size + 1, fmt, ap); // +1 for null if ((r < 0) || (r > size)) { kmem_free(ptr, size); r = -1; diff --git a/module/os/windows/spl/spl-kstat.c b/module/os/windows/spl/spl-kstat.c index 60107acbd450..9c0befbe9232 100644 --- a/module/os/windows/spl/spl-kstat.c +++ b/module/os/windows/spl/spl-kstat.c @@ -309,7 +309,7 @@ sbuf_vprintf(struct sbuf *s, const char *fmt, va_list ap) do { va_copy(ap_copy, ap); - len = vsnprintf(&s->s_buf[s->s_len], SBUF_FREESPACE(s) + 1, + len = spl_vsnprintf(&s->s_buf[s->s_len], SBUF_FREESPACE(s) + 1, fmt, ap_copy); // left-side must be assignable. Win tries to set to 0. // va_end(ap_copy); diff --git a/module/os/windows/spl/spl-proc_list.c b/module/os/windows/spl/spl-proc_list.c index aeabb33d6015..6fea0a75cf34 100644 --- a/module/os/windows/spl/spl-proc_list.c +++ b/module/os/windows/spl/spl-proc_list.c @@ -35,7 +35,7 @@ seq_printf(struct seq_file *f, const char *fmt, ...) va_list adx; va_start(adx, fmt); - (void) vsnprintf(f->sf_buf, f->sf_size, fmt, adx); + (void) spl_vsnprintf(f->sf_buf, f->sf_size, fmt, adx); va_end(adx); } diff --git a/module/os/windows/zfs/zfs_debug.c b/module/os/windows/zfs/zfs_debug.c index 01584506703d..b3211328ee17 100644 --- a/module/os/windows/zfs/zfs_debug.c +++ b/module/os/windows/zfs/zfs_debug.c @@ -228,7 +228,7 @@ __dprintf(boolean_t dprint, const char *file, const char *func, } va_start(adx, fmt); - size = vsnprintf(NULL, 0, fmt, adx); + size = spl_vsnprintf(NULL, 0, fmt, adx); va_end(adx); size += snprintf(NULL, 0, "%s%s:%d:%s(): ", prefix, newfile, line, @@ -246,7 +246,7 @@ __dprintf(boolean_t dprint, const char *file, const char *func, va_start(adx, fmt); i = snprintf(buf, size + 1, "%s%s:%d:%s(): ", prefix, newfile, line, func); - roger = vsnprintf(buf + i, size -i + 1, fmt, adx); + roger = spl_vsnprintf(buf + i, size -i + 1, fmt, adx); va_end(adx); /* diff --git a/module/zfs/spa_misc.c b/module/zfs/spa_misc.c index 34915bf12474..0e3063ab9297 100644 --- a/module/zfs/spa_misc.c +++ b/module/zfs/spa_misc.c @@ -400,7 +400,7 @@ spa_load_failed(spa_t *spa, const char *fmt, ...) char buf[256]; va_start(adx, fmt); - (void) vsnprintf(buf, sizeof (buf), fmt, adx); + (void) spl_vsnprintf(buf, sizeof (buf), fmt, adx); va_end(adx); zfs_dbgmsg("spa_load(%s, config %s): FAILED: %s", spa->spa_name, @@ -415,7 +415,7 @@ spa_load_note(spa_t *spa, const char *fmt, ...) char buf[256]; va_start(adx, fmt); - (void) vsnprintf(buf, sizeof (buf), fmt, adx); + (void) spl_vsnprintf(buf, sizeof (buf), fmt, adx); va_end(adx); zfs_dbgmsg("spa_load(%s, config %s): %s", spa->spa_name, diff --git a/module/zfs/vdev.c b/module/zfs/vdev.c index 255202ab2bee..0935b32ed48e 100644 --- a/module/zfs/vdev.c +++ b/module/zfs/vdev.c @@ -145,7 +145,7 @@ vdev_dbgmsg(vdev_t *vd, const char *fmt, ...) char buf[256]; va_start(adx, fmt); - (void) vsnprintf(buf, sizeof (buf), fmt, adx); + (void) spl_vsnprintf(buf, sizeof (buf), fmt, adx); va_end(adx); if (vd->vdev_path != NULL) { diff --git a/module/zfs/zcp.c b/module/zfs/zcp.c index a596c8eedd90..08aced4c7f29 100644 --- a/module/zfs/zcp.c +++ b/module/zfs/zcp.c @@ -1244,7 +1244,7 @@ zcp_args_error(lua_State *state, const char *fname, const zcp_arg_t *pargs, va_list argp; va_start(argp, fmt); - VERIFY3U(len, >, vsnprintf(errmsg, len, fmt, argp)); + VERIFY3U(len, >, spl_vsnprintf(errmsg, len, fmt, argp)); va_end(argp); /* diff --git a/module/zfs/zio.c b/module/zfs/zio.c index be06c386e3df..4cc3a2ef3584 100644 --- a/module/zfs/zio.c +++ b/module/zfs/zio.c @@ -929,7 +929,7 @@ zfs_blkptr_verify_log(spa_t *spa, const blkptr_t *bp, char buf[256]; va_start(adx, fmt); - (void) vsnprintf(buf, sizeof (buf), fmt, adx); + (void) spl_vsnprintf(buf, sizeof (buf), fmt, adx); va_end(adx); switch (blk_verify) { From 8ea0e0b32aa7e98410d1202651ecd6f7f2d44291 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 15:51:44 +0530 Subject: [PATCH 14/20] Fix build break: spl_spl_vsnprintf typo from previous commit The previous commit (fd4bcd5c0) 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. --- module/os/windows/spl/spl-kmem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module/os/windows/spl/spl-kmem.c b/module/os/windows/spl/spl-kmem.c index 62eeb1e88777..5e265ca17ad8 100644 --- a/module/os/windows/spl/spl-kmem.c +++ b/module/os/windows/spl/spl-kmem.c @@ -6621,13 +6621,13 @@ kmem_asprintf(const char *fmt, ...) char *buf; va_start(adx, fmt); - size = spl_spl_vsnprintf(NULL, 0, fmt, adx) + 1; + size = spl_vsnprintf(NULL, 0, fmt, adx) + 1; va_end(adx); buf = kmem_alloc(size, KM_SLEEP); va_start(adx, fmt); - (void) spl_spl_vsnprintf(buf, size, fmt, adx); + (void) spl_vsnprintf(buf, size, fmt, adx); va_end(adx); return (buf); From 708f2385bf83ee89e6cf371ec5e1e2a2c33f92ef Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 16:21:16 +0530 Subject: [PATCH 15/20] Add spl_* shims to the user-mode types.h too 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 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. --- lib/libspl/include/os/windows/sys/types.h | 43 +++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/lib/libspl/include/os/windows/sys/types.h b/lib/libspl/include/os/windows/sys/types.h index 10090c77652a..fb0e153febff 100644 --- a/lib/libspl/include/os/windows/sys/types.h +++ b/lib/libspl/include/os/windows/sys/types.h @@ -228,4 +228,47 @@ typedef uint64_t zoff_t; #include #endif +#include +#include + +/* + * Mirrors include/os/windows/spl/sys/types.h's kernel-mode shims of the + * same name. Several shared module/zfs, module/icp, and module/lua + * source files (built both into the ZFSin kernel driver and into + * user-mode libzpool/libicp/zlib here) call these directly by name, not + * through a portable macro. In user mode, real strlcpy/strlcat + * (lib/libspl) and real, C99-conformant UCRT vsnprintf are already + * available, so these are simple passthroughs - no downlevel- + * unavailability workaround is needed here, unlike the kernel version. + */ +static __inline size_t +spl_strlcpy(char *dst, const char *src, size_t dstsize) +{ + return (strlcpy(dst, src, dstsize)); +} + +static __inline size_t +spl_strlcat(char *dst, const char *src, size_t dstsize) +{ + return (strlcat(dst, src, dstsize)); +} + +static __inline int +spl_vsnprintf(char *buf, size_t size, const char *fmt, va_list args) +{ + return (vsnprintf(buf, size, fmt, args)); +} + +static __inline int +spl_snprintf(char *buf, size_t size, const char *fmt, ...) +{ + va_list args; + int ret; + + va_start(args, fmt); + ret = spl_vsnprintf(buf, size, fmt, args); + va_end(args); + return (ret); +} + #endif From 038172c24667734469132aac331a01b36004a5c5 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 17:53:25 +0530 Subject: [PATCH 16/20] Fix wrong-CRT OpenSSL crypto lib linked into Debug executables 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. --- lib/libzfs/CMakeLists.txt | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/libzfs/CMakeLists.txt b/lib/libzfs/CMakeLists.txt index 488bc9826742..5fc68999915e 100644 --- a/lib/libzfs/CMakeLists.txt +++ b/lib/libzfs/CMakeLists.txt @@ -20,12 +20,24 @@ add_library(libzfs os/windows/libzfs_util_os.c ) -#variable_watch(CRYPTO_STATIC) -# set(CRYPTO_STATIC "notset") -set(CMAKE_FIND_DEBUG_MODE TRUE) -find_library(CRYPTO_STATIC_TEST +# find_package(OpenSSL)'s own LIB_EAY_DEBUG/LIB_EAY_RELEASE search (see +# contrib/windows/cmake/FindOpenSSL.cmake) correctly locates the CRT- +# matched static libs, but nothing in this tree actually links against +# OPENSSL_CRYPTO_LIBRARY - hence this direct find_library, picking the +# variant that matches CMAKE_BUILD_TYPE (a fresh search hardcoded to the +# MTd/debug name here would silently link the debug-CRT crypto lib into +# a Release build too). Cache variable name changed from the old +# CRYPTO_STATIC_TEST so this re-searches on the next configure rather +# than reusing a stale cached path from before this logic existed. +if(CMAKE_BUILD_TYPE STREQUAL "Debug") + set(_libzfs_openssl_crypto_name libcrypto64MTd) +else() + set(_libzfs_openssl_crypto_name libcrypto64MT) +endif() + +find_library(LIBZFS_OPENSSL_CRYPTO NAMES - libcrypto64MTd + ${_libzfs_openssl_crypto_name} NAMES_PER_DIR HINTS "C:/Program Files/OpenSSL-Win64/lib/VC/static" @@ -33,7 +45,8 @@ find_library(CRYPTO_STATIC_TEST lib REQUIRED ) +unset(_libzfs_openssl_crypto_name) target_include_directories(libzfs PRIVATE "${CMAKE_SOURCE_DIR}/lib/libzfs") target_link_libraries(libzfs PUBLIC libpthread zlib libzutil libshare libzfs_core libnvpair libuutil) -target_link_libraries(libzfs PRIVATE Crypt32.lib ${CRYPTO_STATIC_TEST}) +target_link_libraries(libzfs PRIVATE Crypt32.lib ${LIBZFS_OPENSSL_CRYPTO}) From 715f8d76cfb840887e9eebe8014eaef19cc53401 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 19:54:40 +0530 Subject: [PATCH 17/20] Stop relying on ExAllocatePoolZero's implicit zero-fill; zero explicitly 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 --- module/os/windows/debug.c | 4 +++- module/os/windows/zfs/zfs_vnops_windows.c | 14 +++++++++++--- .../os/windows/zfs/zfs_vnops_windows_mount.c | 8 ++++++-- module/os/windows/zfs/zfs_windows_zvol.c | 19 +++++++++++++++---- module/os/windows/zfs/zfs_windows_zvol_scsi.c | 6 ++++-- 5 files changed, 39 insertions(+), 12 deletions(-) diff --git a/module/os/windows/debug.c b/module/os/windows/debug.c index 595621ea4fb8..8713568b0ee1 100644 --- a/module/os/windows/debug.c +++ b/module/os/windows/debug.c @@ -51,8 +51,10 @@ static unsigned long long startOff = 0; int initDbgCircularBuffer(void) { - cbuf = ExAllocatePoolZero(NonPagedPoolNx, cbuf_size, '!GBD'); + cbuf = ExAllocatePoolUninitialized(NonPagedPoolNx, cbuf_size, '!GBD'); ASSERT(cbuf); + if (cbuf != NULL) + RtlZeroMemory(cbuf, cbuf_size); KeInitializeSpinLock(&cbuf_spin); return (0); } diff --git a/module/os/windows/zfs/zfs_vnops_windows.c b/module/os/windows/zfs/zfs_vnops_windows.c index f7c10dba2bf4..dbfe06aae150 100644 --- a/module/os/windows/zfs/zfs_vnops_windows.c +++ b/module/os/windows/zfs/zfs_vnops_windows.c @@ -421,8 +421,10 @@ zfs_find_dvp_vp(zfsvfs_t *zfsvfs, char *filename, int finalpartmaynotexist, * - maharmstone */ REPARSE_DATA_BUFFER *rpb; - rpb = ExAllocatePoolZero(PagedPool, + rpb = ExAllocatePoolUninitialized(PagedPool, zp->z_size, '!FSZ'); + if (rpb != NULL) + RtlZeroMemory(rpb, zp->z_size); zfs_uio_t uio; struct iovec iov = { rpb, zp->z_size }; zfs_uio_iovec_init(&uio, &iov, 1, 0, @@ -1765,11 +1767,14 @@ pnp_query_id(PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp) zmo = (mount_t *)DeviceObject->DeviceExtension; - Irp->IoStatus.Information = (void *)ExAllocatePoolZero(PagedPool, + Irp->IoStatus.Information = (void *)ExAllocatePoolUninitialized(PagedPool, zmo->bus_name.Length + sizeof (UNICODE_NULL), '!OIZ'); if (Irp->IoStatus.Information == NULL) return (STATUS_NO_MEMORY); + RtlZeroMemory(Irp->IoStatus.Information, + zmo->bus_name.Length + sizeof (UNICODE_NULL)); + RtlCopyMemory(Irp->IoStatus.Information, zmo->bus_name.Buffer, zmo->bus_name.Length); // TraceEvent(TRACE_NOISY, "replying with '%.*S'\n", zmo->uuid.Length/sizeof (WCHAR), @@ -2283,9 +2288,12 @@ BufferUserBuffer(IN OUT PIRP Irp, IN ULONG BufferLength) if (Irp->AssociatedIrp.SystemBuffer == NULL) { UserBuffer = MapUserBuffer(Irp); Irp->AssociatedIrp.SystemBuffer = - ExAllocatePoolZero(NonPagedPoolNx, + ExAllocatePoolUninitialized(NonPagedPoolNx, BufferLength, 'qtaf'); + if (Irp->AssociatedIrp.SystemBuffer != NULL) + RtlZeroMemory(Irp->AssociatedIrp.SystemBuffer, + BufferLength); // // Set the flags so that the completion code knows to // deallocate the buffer. diff --git a/module/os/windows/zfs/zfs_vnops_windows_mount.c b/module/os/windows/zfs/zfs_vnops_windows_mount.c index da472dcf2947..9fab546651f1 100644 --- a/module/os/windows/zfs/zfs_vnops_windows_mount.c +++ b/module/os/windows/zfs/zfs_vnops_windows_mount.c @@ -325,13 +325,15 @@ SendVolumeArrivalNotification(PUNICODE_STRING DeviceName) dprintf("=> SendVolumeArrivalNotification: '%wZ'\n", DeviceName); length = sizeof (MOUNTMGR_TARGET_NAME) + DeviceName->Length - 1; - targetName = ExAllocatePoolZero(PagedPool, length, 'ZVAN'); + targetName = ExAllocatePoolUninitialized(PagedPool, length, 'ZVAN'); if (targetName == NULL) { dprintf(" can't allocate MOUNTMGR_TARGET_NAME\n"); return (STATUS_INSUFFICIENT_RESOURCES); } + RtlZeroMemory(targetName, length); + targetName->DeviceNameLength = DeviceName->Length; RtlCopyMemory(targetName->DeviceName, DeviceName->Buffer, DeviceName->Length); @@ -446,13 +448,15 @@ SendVolumeCreatePoint(__in PUNICODE_STRING DeviceName, length = sizeof (MOUNTMGR_CREATE_POINT_INPUT) + MountPoint->Length + DeviceName->Length; - point = ExAllocatePoolZero(PagedPool, length, 'ZVCP'); + point = ExAllocatePoolUninitialized(PagedPool, length, 'ZVCP'); if (point == NULL) { dprintf(" can't allocate MOUNTMGR_CREATE_POINT_INPUT\n"); return (STATUS_INSUFFICIENT_RESOURCES); } + RtlZeroMemory(point, length); + dprintf(" DeviceName: %wZ\n", DeviceName); point->DeviceNameOffset = sizeof (MOUNTMGR_CREATE_POINT_INPUT); point->DeviceNameLength = DeviceName->Length; diff --git a/module/os/windows/zfs/zfs_windows_zvol.c b/module/os/windows/zfs/zfs_windows_zvol.c index 2c85a854bb4e..a60a2f66b25f 100644 --- a/module/os/windows/zfs/zfs_windows_zvol.c +++ b/module/os/windows/zfs/zfs_windows_zvol.c @@ -105,13 +105,18 @@ zvol_start(PDRIVER_OBJECT DriverObject, PUNICODE_STRING pRegistryPath) // array. now we can go up to 32,640 zvols. pwzvolDrvInfo->NumberOfBuses = 1; pwzvolDrvInfo->zvContextArray = - (wzvolContext*)ExAllocatePoolZero(NonPagedPoolNx, + (wzvolContext*)ExAllocatePoolUninitialized(NonPagedPoolNx, ((SIZE_T)pwzvolDrvInfo->MaximumNumberOfTargets * pwzvolDrvInfo->MaximumNumberOfLogicalUnits * sizeof (wzvolContext)), MP_TAG_GENERAL); if (pwzvolDrvInfo->zvContextArray == NULL) return (STATUS_NO_MEMORY); + RtlZeroMemory(pwzvolDrvInfo->zvContextArray, + ((SIZE_T)pwzvolDrvInfo->MaximumNumberOfTargets * + pwzvolDrvInfo->MaximumNumberOfLogicalUnits * + (sizeof (wzvolContext)))); + RtlZeroMemory(&hwInitData, sizeof (VIRTUAL_HW_INITIALIZATION_DATA)); hwInitData.HwInitializationDataSize = @@ -340,9 +345,11 @@ wzvol_HwReportAdapter(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = ExAllocatePoolUninitialized(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { + RtlZeroMemory(pWnode, size); + // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. @@ -438,9 +445,11 @@ wzvol_HwReportLink(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = ExAllocatePoolUninitialized(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { + RtlZeroMemory(pWnode, size); + // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. @@ -526,9 +535,11 @@ wzvol_HwReportLog(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = ExAllocatePoolUninitialized(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { + RtlZeroMemory(pWnode, size); + // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. diff --git a/module/os/windows/zfs/zfs_windows_zvol_scsi.c b/module/os/windows/zfs/zfs_windows_zvol_scsi.c index ecc21d6d1350..1b2fb5a3b375 100644 --- a/module/os/windows/zfs/zfs_windows_zvol_scsi.c +++ b/module/os/windows/zfs/zfs_windows_zvol_scsi.c @@ -397,7 +397,7 @@ ScsiGetMPIOExt( } if (pNextEntry == &pHBAExt->pwzvolDrvObj->ListMPIOExt) { - pLUMPIOExt = ExAllocatePoolZero(NonPagedPoolNx, + pLUMPIOExt = ExAllocatePoolUninitialized(NonPagedPoolNx, sizeof (HW_LU_EXTENSION_MPIO), MP_TAG_GENERAL); if (!pLUMPIOExt) { @@ -405,6 +405,8 @@ ScsiGetMPIOExt( goto Done; } + RtlZeroMemory(pLUMPIOExt, sizeof (HW_LU_EXTENSION_MPIO)); + pLUMPIOExt->ScsiAddr.PathId = pSrb->PathId; pLUMPIOExt->ScsiAddr.TargetId = pSrb->TargetId; pLUMPIOExt->ScsiAddr.Lun = pSrb->Lun; @@ -1087,7 +1089,7 @@ DiReadWriteSetup(zvol_state_t *zv, MpWkRtnAction action, zfsiodesc_t *pIo) { // cannot use kmem_alloc with sleep if IRQL dispatch so get straight // from NP pool. - pMP_WorkRtnParms pWkRtnParms = (pMP_WorkRtnParms)ExAllocatePoolZero( + pMP_WorkRtnParms pWkRtnParms = (pMP_WorkRtnParms)ExAllocatePoolUninitialized( NonPagedPoolNx, ALIGN_UP_BY(sizeof (MP_WorkRtnParms), 16) + IoSizeofWorkItem(), MP_TAG_GENERAL); if (NULL == pWkRtnParms) { From f70f86092cf6d7000405787ddeb4de59cc7ee24e Mon Sep 17 00:00:00 2001 From: Arun KV Date: Tue, 11 Aug 2026 19:55:08 +0530 Subject: [PATCH 18/20] Fix off-by-one buffer-size overstatement in __dprintf __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 --- module/os/windows/zfs/zfs_debug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module/os/windows/zfs/zfs_debug.c b/module/os/windows/zfs/zfs_debug.c index b3211328ee17..a34cd82543f4 100644 --- a/module/os/windows/zfs/zfs_debug.c +++ b/module/os/windows/zfs/zfs_debug.c @@ -244,9 +244,9 @@ __dprintf(boolean_t dprint, const char *file, const char *func, int roger = 0; va_start(adx, fmt); - i = snprintf(buf, size + 1, "%s%s:%d:%s(): ", + i = snprintf(buf, size, "%s%s:%d:%s(): ", prefix, newfile, line, func); - roger = spl_vsnprintf(buf + i, size -i + 1, fmt, adx); + roger = spl_vsnprintf(buf + i, size - i, fmt, adx); va_end(adx); /* From 2e1a80b736177ff2bceaaf689d67701c4885c78e Mon Sep 17 00:00:00 2001 From: Arun KV Date: Wed, 12 Aug 2026 12:34:40 +0530 Subject: [PATCH 19/20] Eliminate last CodeQL finding in spl_vsnprintf; harden against review 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 --- include/os/windows/spl/sys/types.h | 29 ++++--- module/os/windows/spl/spl-kmem.c | 118 ++++++++++++++++++++++++++++- module/os/windows/spl/spl-kstat.c | 10 +++ module/os/windows/zfs/zfs_debug.c | 10 +++ 4 files changed, 154 insertions(+), 13 deletions(-) diff --git a/include/os/windows/spl/sys/types.h b/include/os/windows/spl/sys/types.h index 6c2a5e92da18..db7b1fb545e8 100644 --- a/include/os/windows/spl/sys/types.h +++ b/include/os/windows/spl/sys/types.h @@ -101,19 +101,24 @@ typedef uintptr_t pc_t; * length) and does not NUL-terminate the buffer on truncation, unlike * standard snprintf(). Portable ZFS/SPL code assumes real snprintf() * semantics, so give it those semantics here rather than the raw - * deprecated function. No _vscprintf in ntoskrnl.lib - measure via - * _vsnprintf(NULL, 0, ...), which returns the true length for count==0 - * (same idiom kmem_asprintf() already relies on). + * deprecated function. + * + * There is no _vscprintf() in ntoskrnl.lib, and ntstrsafe.h's + * String RtlStringCchPrintfEx family cannot measure a formatted + * string's length without a real, non-zero destination buffer (a + * cchDest of 0 short-circuits before formatting even happens) - so + * "how long would this be" can only be discovered by actually + * formatting into a real, possibly-grown, scratch buffer. + * + * spl_vsnprintf() is implemented out-of-line in + * module/os/windows/spl/spl-kmem.c, NOT as a static inline here, + * because that implementation needs kmem_alloc()/kmem_free() - + * sys/kmem.h itself #includes sys/types.h, so an inline definition + * here could never see kmem_alloc()'s declaration without an + * unsupportable circular include. */ -static __inline int -spl_vsnprintf(char *buf, size_t size, const char *fmt, va_list args) -{ - va_list args_copy = args; /* x64 MSVC va_list is a plain pointer */ - int needed = _vsnprintf(NULL, 0, fmt, args_copy); - if (size > 0 && buf != NULL) - _vsnprintf_s(buf, size, _TRUNCATE, fmt, args); - return (needed); -} +extern int spl_vsnprintf(char *buf, size_t size, const char *fmt, + va_list args); static __inline int spl_snprintf(char *buf, size_t size, const char *fmt, ...) diff --git a/module/os/windows/spl/spl-kmem.c b/module/os/windows/spl/spl-kmem.c index 5e265ca17ad8..4fad95d648c1 100644 --- a/module/os/windows/spl/spl-kmem.c +++ b/module/os/windows/spl/spl-kmem.c @@ -6613,6 +6613,118 @@ kmem_asdprintf(const char *fmt, ...) return (ptr); } +#define SPL_VSNPRINTF_PROBE_MIN 256 +/* + * 1 MiB: roughly 256x the largest single formatted string anywhere + * in this tree today (PAGE_SIZE == 4096, in zfs_fletcher.c). No real + * caller is expected to ever reach this; it exists only to bound a + * pathological/malformed format string's retry loop. + */ +#define SPL_VSNPRINTF_PROBE_MAX (1024 * 1024) + +/* + * True-length-preserving, deprecated-API-free replacement for + * _vsnprintf(). See the comment above its prototype in sys/types.h + * for why this can't be ntstrsafe.h-based and why it lives here + * rather than as a header inline. + * + * Contract (do not change without auditing every caller of + * spl_vsnprintf()/spl_snprintf()/snprintf() in the tree - e.g. + * dmu_redact.c, zcp_iter.c, zfs_fletcher.c, kmem_asprintf(), + * kmem_vasprintf()): + * - If the formatted string (plus NUL) fits in [buf, buf+size), + * it is written in full and the exact number of characters + * written (excluding the NUL) is returned. + * - Otherwise (including buf==NULL/size==0), the return value is + * still the exact number of characters the FULL, untruncated + * result would have needed - real snprintf() semantics, not + * _vsnprintf()'s -1 - even though buf itself may be left + * truncated exactly as _vsnprintf_s(..., _TRUNCATE, ...) leaves + * it (or untouched, if buf==NULL/size==0). + */ +int +spl_vsnprintf(char *buf, size_t size, const char *fmt, va_list args) +{ + va_list args_copy; + int ret; + size_t cap; + char stackbuf[SPL_VSNPRINTF_PROBE_MIN]; + + /* + * Tier 1: try the caller's own buffer first. This covers every + * call site that already passes a real, adequately sized + * buffer (the common case) with zero extra allocation - IRQL- + * safe (no allocation), cheaper than the old code, which always + * paid for a wasted measuring call even when the real write + * succeeded. + */ + if (buf != NULL && size > 0) { + args_copy = args; /* x64 MSVC va_list is a plain pointer */ + ret = _vsnprintf_s(buf, size, _TRUNCATE, fmt, args_copy); + if (ret >= 0) + return (ret); /* fit: ret IS the true length */ + } + + /* + * Tier 2: a small on-stack probe. Still IRQL-safe (no + * allocation) - every real caller in this tree writes a buffer + * under a few hundred bytes (the one known exception, + * module/lua/lstrlib.c's Lua channel-program formatting, is + * intentionally unbounded and falls through to Tier 3), so this + * is what makes every measure-only caller (buf==NULL, e.g. + * __dprintf's first call, kmem_asprintf(), kmem_vasprintf()) + * avoid the allocator entirely in the overwhelmingly common + * case. + */ + args_copy = args; + ret = _vsnprintf_s(stackbuf, sizeof (stackbuf), _TRUNCATE, fmt, + args_copy); + if (ret >= 0) + return (ret); + + /* + * Tier 3: only reached when even a 256-byte probe truncates. + * This is the only tier that allocates, so it is the only tier + * that can violate IRQL rules (KM_SLEEP can block) - guard it + * explicitly here, at the one place that actually needs it, + * rather than requiring every current and future caller + * (vcmn_err included) to remember its own guard. + * + * This is a real, if rare, new return value. Every live caller + * in the tree has been individually audited to confirm this is + * safe: callers that discard the return value are unaffected + * (their buffer is already correctly truncated by Tier 1/2's + * _vsnprintf_s call); kmem_vasprintf() already anticipates and + * handles a negative return from its measuring call; + * kmem_asprintf() is hardened alongside this change specifically + * because it previously was not safe against one. + */ + if (KeGetCurrentIrql() >= DISPATCH_LEVEL) + return (-1); + + cap = SPL_VSNPRINTF_PROBE_MIN * 2; + if (size > cap) + cap = size; + if (cap >= SPL_VSNPRINTF_PROBE_MAX) + cap = SPL_VSNPRINTF_PROBE_MAX; + + for (;;) { + /* KM_SLEEP: always succeeds, never returns NULL. */ + char *tmp = kmem_alloc(cap, KM_SLEEP); + args_copy = args; + ret = _vsnprintf_s(tmp, cap, _TRUNCATE, fmt, args_copy); + kmem_free(tmp, cap); + if (ret >= 0) + return (ret); + if (cap >= SPL_VSNPRINTF_PROBE_MAX) + return (-1); /* honest failure, not a fabricated length */ + if (cap > SPL_VSNPRINTF_PROBE_MAX / 2) + cap = SPL_VSNPRINTF_PROBE_MAX; + else + cap *= 2; + } +} + char * kmem_asprintf(const char *fmt, ...) { @@ -6621,9 +6733,13 @@ kmem_asprintf(const char *fmt, ...) char *buf; va_start(adx, fmt); - size = spl_vsnprintf(NULL, 0, fmt, adx) + 1; + size = spl_vsnprintf(NULL, 0, fmt, adx); va_end(adx); + if (size < 0) + return (NULL); /* honest failure, not KMEM_ZERO_SIZE_PTR */ + size++; + buf = kmem_alloc(size, KM_SLEEP); va_start(adx, fmt); diff --git a/module/os/windows/spl/spl-kstat.c b/module/os/windows/spl/spl-kstat.c index 9c0befbe9232..61579cf812dd 100644 --- a/module/os/windows/spl/spl-kstat.c +++ b/module/os/windows/spl/spl-kstat.c @@ -313,6 +313,16 @@ sbuf_vprintf(struct sbuf *s, const char *fmt, va_list ap) fmt, ap_copy); // left-side must be assignable. Win tries to set to 0. // va_end(ap_copy); + /* + * spl_vsnprintf() can return -1 on failure (e.g. format + * needs more than its ~1 MiB growth ceiling). Treat that + * as "nothing written" rather than let a negative len + * flow into the signed s_len accounting below, which + * would silently decrement s_len and corrupt the next + * sbuf_vprintf() call's buffer offset. + */ + if (len < 0) + len = 0; } while (len > SBUF_FREESPACE(s) && sbuf_extend(s, len - SBUF_FREESPACE(s)) == 0); s->s_len += min(len, SBUF_FREESPACE(s)); diff --git a/module/os/windows/zfs/zfs_debug.c b/module/os/windows/zfs/zfs_debug.c index a34cd82543f4..36c3fd09e99b 100644 --- a/module/os/windows/zfs/zfs_debug.c +++ b/module/os/windows/zfs/zfs_debug.c @@ -236,6 +236,16 @@ __dprintf(boolean_t dprint, const char *file, const char *func, size++; /* null byte in the "buf" string */ + /* + * size is negative only if both spl_vsnprintf() measuring calls + * above independently failed (e.g. each needed more than the + * ~1 MiB spl_vsnprintf() will grow to) - not realistic for a + * single log line, but kmem_alloc() must never see a negative + * size turn into a huge size_t. + */ + if (size <= 0) + return; + /* * There is one byte of string in sizeof (zfs_dbgmsg_t), used * for the terminating null. From 9e080d81f5a40c8e65d360e9c1156f1067131270 Mon Sep 17 00:00:00 2001 From: Arun KV Date: Wed, 12 Aug 2026 12:35:08 +0530 Subject: [PATCH 20/20] Centralize ExAllocatePoolZero fix into a shared spl_ExAllocatePoolZero 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 --- include/os/windows/spl/sys/kmem.h | 20 ++++++++++++++ module/os/windows/debug.c | 5 ++-- module/os/windows/zfs/zfs_vnops_windows.c | 14 +++------- .../os/windows/zfs/zfs_vnops_windows_mount.c | 8 ++---- module/os/windows/zfs/zfs_windows_zvol.c | 26 ++++++------------- module/os/windows/zfs/zfs_windows_zvol_scsi.c | 8 +++--- 6 files changed, 40 insertions(+), 41 deletions(-) diff --git a/include/os/windows/spl/sys/kmem.h b/include/os/windows/spl/sys/kmem.h index 85725ca5a1e2..9993b8419b94 100644 --- a/include/os/windows/spl/sys/kmem.h +++ b/include/os/windows/spl/sys/kmem.h @@ -65,6 +65,26 @@ extern uint64_t physmem; #define FREE(A, T) \ ExFreePoolWithTag((A), '!SFZ') +/* + * Centralizes the "allocate uninitialized, then zero on success" + * pattern used throughout the Windows port, in one place, so the + * allocation size can never drift between the alloc call and the + * zero call (two historical call sites had exactly that bug - see + * zfs_windows_zvol.c's zvol_start() and zfs_vnops_windows.c's + * pnp_query_id(), before this was centralized). A real function, + * not a macro: a macro that referenced its Size argument twice would + * silently reintroduce the same double-evaluation bug for any future + * caller passing a computed expression. + */ +static __inline PVOID +spl_ExAllocatePoolZero(POOL_TYPE PoolType, SIZE_T Size, ULONG Tag) +{ + PVOID ptr = ExAllocatePoolUninitialized(PoolType, Size, Tag); + if (ptr != NULL) + RtlZeroMemory(ptr, Size); + return (ptr); +} + // Work around symbol collisions in XNU #define kmem_alloc(size, kmflags) zfs_kmem_alloc((size), (kmflags)) #define kmem_zalloc(size, kmflags) zfs_kmem_zalloc((size), (kmflags)) diff --git a/module/os/windows/debug.c b/module/os/windows/debug.c index 8713568b0ee1..be8c349be086 100644 --- a/module/os/windows/debug.c +++ b/module/os/windows/debug.c @@ -28,6 +28,7 @@ #define _NO_CRT_STDIO_INLINE #include +#include #include #include #include @@ -51,10 +52,8 @@ static unsigned long long startOff = 0; int initDbgCircularBuffer(void) { - cbuf = ExAllocatePoolUninitialized(NonPagedPoolNx, cbuf_size, '!GBD'); + cbuf = spl_ExAllocatePoolZero(NonPagedPoolNx, cbuf_size, '!GBD'); ASSERT(cbuf); - if (cbuf != NULL) - RtlZeroMemory(cbuf, cbuf_size); KeInitializeSpinLock(&cbuf_spin); return (0); } diff --git a/module/os/windows/zfs/zfs_vnops_windows.c b/module/os/windows/zfs/zfs_vnops_windows.c index dbfe06aae150..4b498fe41a1c 100644 --- a/module/os/windows/zfs/zfs_vnops_windows.c +++ b/module/os/windows/zfs/zfs_vnops_windows.c @@ -421,10 +421,8 @@ zfs_find_dvp_vp(zfsvfs_t *zfsvfs, char *filename, int finalpartmaynotexist, * - maharmstone */ REPARSE_DATA_BUFFER *rpb; - rpb = ExAllocatePoolUninitialized(PagedPool, + rpb = spl_ExAllocatePoolZero(PagedPool, zp->z_size, '!FSZ'); - if (rpb != NULL) - RtlZeroMemory(rpb, zp->z_size); zfs_uio_t uio; struct iovec iov = { rpb, zp->z_size }; zfs_uio_iovec_init(&uio, &iov, 1, 0, @@ -1767,14 +1765,11 @@ pnp_query_id(PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp) zmo = (mount_t *)DeviceObject->DeviceExtension; - Irp->IoStatus.Information = (void *)ExAllocatePoolUninitialized(PagedPool, + Irp->IoStatus.Information = (void *)spl_ExAllocatePoolZero(PagedPool, zmo->bus_name.Length + sizeof (UNICODE_NULL), '!OIZ'); if (Irp->IoStatus.Information == NULL) return (STATUS_NO_MEMORY); - RtlZeroMemory(Irp->IoStatus.Information, - zmo->bus_name.Length + sizeof (UNICODE_NULL)); - RtlCopyMemory(Irp->IoStatus.Information, zmo->bus_name.Buffer, zmo->bus_name.Length); // TraceEvent(TRACE_NOISY, "replying with '%.*S'\n", zmo->uuid.Length/sizeof (WCHAR), @@ -2288,12 +2283,9 @@ BufferUserBuffer(IN OUT PIRP Irp, IN ULONG BufferLength) if (Irp->AssociatedIrp.SystemBuffer == NULL) { UserBuffer = MapUserBuffer(Irp); Irp->AssociatedIrp.SystemBuffer = - ExAllocatePoolUninitialized(NonPagedPoolNx, + spl_ExAllocatePoolZero(NonPagedPoolNx, BufferLength, 'qtaf'); - if (Irp->AssociatedIrp.SystemBuffer != NULL) - RtlZeroMemory(Irp->AssociatedIrp.SystemBuffer, - BufferLength); // // Set the flags so that the completion code knows to // deallocate the buffer. diff --git a/module/os/windows/zfs/zfs_vnops_windows_mount.c b/module/os/windows/zfs/zfs_vnops_windows_mount.c index 9fab546651f1..4b8dd03fe447 100644 --- a/module/os/windows/zfs/zfs_vnops_windows_mount.c +++ b/module/os/windows/zfs/zfs_vnops_windows_mount.c @@ -325,15 +325,13 @@ SendVolumeArrivalNotification(PUNICODE_STRING DeviceName) dprintf("=> SendVolumeArrivalNotification: '%wZ'\n", DeviceName); length = sizeof (MOUNTMGR_TARGET_NAME) + DeviceName->Length - 1; - targetName = ExAllocatePoolUninitialized(PagedPool, length, 'ZVAN'); + targetName = spl_ExAllocatePoolZero(PagedPool, length, 'ZVAN'); if (targetName == NULL) { dprintf(" can't allocate MOUNTMGR_TARGET_NAME\n"); return (STATUS_INSUFFICIENT_RESOURCES); } - RtlZeroMemory(targetName, length); - targetName->DeviceNameLength = DeviceName->Length; RtlCopyMemory(targetName->DeviceName, DeviceName->Buffer, DeviceName->Length); @@ -448,15 +446,13 @@ SendVolumeCreatePoint(__in PUNICODE_STRING DeviceName, length = sizeof (MOUNTMGR_CREATE_POINT_INPUT) + MountPoint->Length + DeviceName->Length; - point = ExAllocatePoolUninitialized(PagedPool, length, 'ZVCP'); + point = spl_ExAllocatePoolZero(PagedPool, length, 'ZVCP'); if (point == NULL) { dprintf(" can't allocate MOUNTMGR_CREATE_POINT_INPUT\n"); return (STATUS_INSUFFICIENT_RESOURCES); } - RtlZeroMemory(point, length); - dprintf(" DeviceName: %wZ\n", DeviceName); point->DeviceNameOffset = sizeof (MOUNTMGR_CREATE_POINT_INPUT); point->DeviceNameLength = DeviceName->Length; diff --git a/module/os/windows/zfs/zfs_windows_zvol.c b/module/os/windows/zfs/zfs_windows_zvol.c index a60a2f66b25f..b0055dab7906 100644 --- a/module/os/windows/zfs/zfs_windows_zvol.c +++ b/module/os/windows/zfs/zfs_windows_zvol.c @@ -104,19 +104,15 @@ zvol_start(PDRIVER_OBJECT DriverObject, PUNICODE_STRING pRegistryPath) // supporting more would mean bigger changes in the zv_targets // array. now we can go up to 32,640 zvols. pwzvolDrvInfo->NumberOfBuses = 1; + SIZE_T zvContextArraySize = (SIZE_T)pwzvolDrvInfo->MaximumNumberOfTargets * + pwzvolDrvInfo->MaximumNumberOfLogicalUnits * sizeof (wzvolContext); + pwzvolDrvInfo->zvContextArray = - (wzvolContext*)ExAllocatePoolUninitialized(NonPagedPoolNx, - ((SIZE_T)pwzvolDrvInfo->MaximumNumberOfTargets * - pwzvolDrvInfo->MaximumNumberOfLogicalUnits * - sizeof (wzvolContext)), MP_TAG_GENERAL); + (wzvolContext*)spl_ExAllocatePoolZero(NonPagedPoolNx, + zvContextArraySize, MP_TAG_GENERAL); if (pwzvolDrvInfo->zvContextArray == NULL) return (STATUS_NO_MEMORY); - RtlZeroMemory(pwzvolDrvInfo->zvContextArray, - ((SIZE_T)pwzvolDrvInfo->MaximumNumberOfTargets * - pwzvolDrvInfo->MaximumNumberOfLogicalUnits * - (sizeof (wzvolContext)))); - RtlZeroMemory(&hwInitData, sizeof (VIRTUAL_HW_INITIALIZATION_DATA)); hwInitData.HwInitializationDataSize = @@ -345,11 +341,9 @@ wzvol_HwReportAdapter(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolUninitialized(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = spl_ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { - RtlZeroMemory(pWnode, size); - // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. @@ -445,11 +439,9 @@ wzvol_HwReportLink(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolUninitialized(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = spl_ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { - RtlZeroMemory(pWnode, size); - // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. @@ -535,11 +527,9 @@ wzvol_HwReportLog(__in pHW_HBA_EXT pHBAExt) WnodeSizeInstanceName + WnodeSizeDataBlock; - pWnode = ExAllocatePoolUninitialized(NonPagedPoolNx, size, MP_TAG_GENERAL); + pWnode = spl_ExAllocatePoolZero(NonPagedPoolNx, size, MP_TAG_GENERAL); if (NULL != pWnode) { - RtlZeroMemory(pWnode, size); - // Fill out most of header. StorPort will set the // ProviderId and TimeStamp in the header. diff --git a/module/os/windows/zfs/zfs_windows_zvol_scsi.c b/module/os/windows/zfs/zfs_windows_zvol_scsi.c index 1b2fb5a3b375..10142719668c 100644 --- a/module/os/windows/zfs/zfs_windows_zvol_scsi.c +++ b/module/os/windows/zfs/zfs_windows_zvol_scsi.c @@ -397,7 +397,7 @@ ScsiGetMPIOExt( } if (pNextEntry == &pHBAExt->pwzvolDrvObj->ListMPIOExt) { - pLUMPIOExt = ExAllocatePoolUninitialized(NonPagedPoolNx, + pLUMPIOExt = spl_ExAllocatePoolZero(NonPagedPoolNx, sizeof (HW_LU_EXTENSION_MPIO), MP_TAG_GENERAL); if (!pLUMPIOExt) { @@ -405,8 +405,6 @@ ScsiGetMPIOExt( goto Done; } - RtlZeroMemory(pLUMPIOExt, sizeof (HW_LU_EXTENSION_MPIO)); - pLUMPIOExt->ScsiAddr.PathId = pSrb->PathId; pLUMPIOExt->ScsiAddr.TargetId = pSrb->TargetId; pLUMPIOExt->ScsiAddr.Lun = pSrb->Lun; @@ -1089,6 +1087,10 @@ DiReadWriteSetup(zvol_state_t *zv, MpWkRtnAction action, zfsiodesc_t *pIo) { // cannot use kmem_alloc with sleep if IRQL dispatch so get straight // from NP pool. + // Not spl_ExAllocatePoolZero(): this allocation is deliberately + // larger than what gets zeroed below - the extra IoSizeofWorkItem() + // bytes are opaque storage that IoInitializeWorkItem() fills in + // itself, so zeroing them would be redundant work. pMP_WorkRtnParms pWkRtnParms = (pMP_WorkRtnParms)ExAllocatePoolUninitialized( NonPagedPoolNx, ALIGN_UP_BY(sizeof (MP_WorkRtnParms), 16) + IoSizeofWorkItem(), MP_TAG_GENERAL);