Clang analyze fixes - #4014
Open
igaw wants to merge 27 commits into
Open
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Valid boundary inputs still trigger undefined shifts, and several newly ignored failures permit indeterminate values or false-success results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Addresses Clang analyzer findings across core commands, libnvme, plugins, and tests.
Changes:
- Fixes memory ownership, error handling, and uninitialized values.
- Hardens protection-tag encoding and validation.
- Adds boundary and test-fixture coverage.
File summaries
| File | Description |
|---|---|
tests/cli/nvme_pif_sts_test.py |
Adds PIF/STS boundary tests. |
src/nvme-print-json.c |
Corrects allocation cleanup. |
src/nvme-cmds-io.c |
Validates PIF storage-tag widths. |
src/fabrics.c |
Frees resolved transport addresses. |
shared/wrap-util.c |
Removes a dead assignment. |
shared/tests/test-wrap-util.c |
Handles tmpfile() failure. |
shared/tests/test-progress-util.c |
Handles tmpfile() failure. |
plugins/wdc/wdc-nvme.c |
Cleans dead assignments and return handling. |
plugins/solidigm/solidigm-workload-tracker.c |
Removes an unused result assignment. |
plugins/solidigm/solidigm-telemetry/data-area.c |
Avoids unnecessary initialization. |
plugins/solidigm/solidigm-latency-tracking.c |
Guards negative shift counts. |
plugins/scaleflux/sfx-nvme.c |
Initializes capacity. |
plugins/sandisk/sandisk-utils.c |
Removes dead assignments. |
plugins/sandisk/sandisk-nvme.c |
Cleans telemetry and PCI-ID handling. |
plugins/ocp/ocp-telemetry-decode.c |
Fixes string-buffer handling and null checks. |
plugins/ocp/ocp-nvme.c |
Removes overwritten assignments. |
plugins/micron/micron-nvme.c |
Preserves failure errno values. |
plugins/memblaze/memblaze-nvme.c |
Guards optional output streams. |
plugins/keys/keys-plugin.c |
Checks freopen() failure. |
plugins/huawei/huawei-nvme.c |
Adjusts NSID and JSON handling. |
plugins/exclusion/exclusion-nvme.c |
Sets allocation failure errno. |
libnvme/tests/mi-mctp.c |
Clears a dangling test pointer. |
libnvme/tests/ioctl/zns.c |
Tests low-STS tag packing. |
libnvme/src/nvme/registry.c |
Handles dirfd() failures. |
libnvme/src/nvme/nvme-cmds-nvm.h |
Guards tag-packing shifts. |
Review details
Suppressed comments (2)
plugins/wdc/wdc-nvme.c:7504
read_device_idis uninitialized ifnvme_get_pci_ids()fails, but it is immediately used as the switch selector. Handle the failure instead of discarding it so this command cannot branch on an indeterminate value.
nvme_get_pci_ids(ctx, hdl, &read_vendor_id, &read_device_id, NULL, NULL, NULL);
plugins/wdc/wdc-nvme.c:8480
- If PCI-ID lookup fails,
read_device_idremains uninitialized and the next switch reads it. Preserve and return the lookup error rather than discarding it.
nvme_get_pci_ids(ctx, hdl, &read_vendor_id, &read_device_id, NULL, NULL, NULL);
- Files reviewed: 25/25 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
On the row-allocation failure path, the cleanup label freed `printable` instead of `printable_start`. `printable` is advanced in-place by the fill loop above (`*printable++ = ch`), so by the time a later row's malloc() fails, it points partway into the buffer rather than at the malloc()'d base. free() on that pointer is undefined behaviour and can corrupt the heap. Free the saved base pointer instead. Signed-off-by: Daniel Wagner <wagi@monom.org>
print_ocp_telemetry_normal() already treats a NULL ptelemetry_buffer as "nothing to parse" for the host/controller log-page header block, but the very next block unconditionally computed `ptelemetry_buffer + offsetof(...)` and handed it to generic_structure_parser(), in both the file-output and stdout-output copies of this code. Pointer arithmetic on a null base is undefined behaviour, and generic_structure_parser() would then dereference near address zero. Extend the existing NULL check to cover the reason-identifier block too, consistent with the header block right above it. Signed-off-by: Daniel Wagner <wagi@monom.org>
io_latency_histogram()'s fopen() return is never checked, and every other output helper in this file (the fPRINT_PARAM1/2 macros) already tolerates a NULL fdi by treating the file as optional. This one call site didn't: fwrite() was called unconditionally, passing NULL as its stream argument whenever fopen() fails. Make it consistent with fPRINT_PARAM1/2 instead of NULL-checking every one of ioLatencyHistogramOutput()'s call sites individually. Signed-off-by: Daniel Wagner <wagi@monom.org>
The fopen() branch for --keyfile already bails out on failure, but the freopen(NULL, "r", stdin) fallback for reading keys from stdin did not, so a failing freopen() would hand import_key() a NULL FILE* that it then passes straight into fgets(). Signed-off-by: Daniel Wagner <wagi@monom.org>
capacity_valid is only ever set inside the `fd >= 0` branch (block device path); when opening the device node fails -- e.g. it's a char device -- capacity_valid stays false and the code falls into `capacity = capacity / (1000 * 1000 * 1000)`, dividing the still-uninitialized capacity. Signed-off-by: Daniel Wagner <wagi@monom.org>
sts (storage tag size) comes straight from the command line and was only bounds-checked against the 64-bit storage_tag field, not against each PIF's actual reference-tag width. `1LL << (32 - sts)`, `(80 - sts)` and `(48 - sts)` all go negative once sts exceeds that width, which is undefined behaviour. Reject an out-of-range sts per PIF up front instead, the same way an oversized storage_tag is already rejected. While at it, switch every `1LL <<` in this function to `1ULL <<`: even within the now-valid range, e.g. sts == 17 on the 32B guard PIF computes `1LL << 63`, and shifting a 1 into a signed 64-bit value's sign bit is its own, separate undefined behaviour. Signed-off-by: Daniel Wagner <wagi@monom.org>
…case The 32B Guard PIF packs an 80-bit combined reference/storage tag field across cdw14 (bits 0-31), cdw3 (bits 32-63) and cdw2 (bits 64-79), with the storage tag occupying the top `sts` bits of that 80-bit field, at [80-sts, 80). Two of the three pieces assumed sts is comfortably large without checking: - cdw2's `storage_tag >> (sts - 16)` goes negative once sts < 16, since storage_tag then sits entirely below cdw2's own window. - cdw14's `storage_tag << (80 - sts)` needs a shift of 64 or more once sts <= 16, since storage_tag no longer reaches down into cdw14's bits at all -- unlike the sts > 16 case, where the __u32 truncation in NVME_FIELD_ENCODE() happens to zero it out for free because the shift itself stays defined (< 64). Guard both the same way the neighbouring cdw3 split already does: switch formula (or skip the term entirely for cdw14) once sts drops to 16 or below. Signed-off-by: Daniel Wagner <wagi@monom.org>
The `i < (base_val << 1)` early return above is meant to rule out error_bits going negative below, and does in every real call path -- but nothing in the function re-derives that guarantee at the point error_bits is actually used, so `1 << error_bits` a few lines down is one field-read reordering away from a negative shift. Bail out the same way the early return above does instead of relying on the caller's invariant holding all the way down. Signed-off-by: Daniel Wagner <wagi@monom.org>
dirfd() is allowed to fail, but its return value was passed straight into unlinkat()/fstatat()/openat() in delete_dir(), libnvmf_registry_device_for_each() and libnvmf_registry_attr_for_each() without a check. On failure that hands those *at() calls a negative fd instead of a valid one or AT_FDCWD. Signed-off-by: Daniel Wagner <wagi@monom.org>
Add the STS-at/over-the-new-limit pairs for all three static PIFs, exercising the bounds added in "nvme: reject out-of-range storage tag size in invalid_tags()". Before that fix these STS values drove invalid_tags() into a negative/oversized shift instead of being rejected. Signed-off-by: Daniel Wagner <wagi@monom.org>
Directly checks cdw2/cdw3/cdw14 from nvme_init_var_size_tags() at sts=8 and the sts=16 boundary against hand-derived expected values, covering the two shifts fixed in "libnvme: fix undefined shifts in nvme_init_var_size_tags() 32B guard case". Confirmed this reproduces the original bug: reverting that fix trips the cdw2 check here. Note: this lives in the ioctl/ loopback test suite, gated behind -Dioctl-tests=true (off by default), like the rest of that suite. Signed-off-by: Daniel Wagner <wagi@monom.org>
nvmf_resolve_addr() strdup()s a resolved hostname into *addr when @transport is tcp/rdma and @addr isn't already numeric; otherwise it leaves *addr pointing at the caller's original (unowned) string. None of its four callers ever freed the resolved copy, on any return path -- build_conn_tid(), fabrics_discover(), fabrics_connect() and fabrics_disconnect() all leaked it. Fixing that with the existing in-place `const char **addr` signature means every caller has to stash the address it passed in and compare it against the (possibly unchanged) result to work out whether it now owns a new allocation -- easy to get wrong, and exactly what went wrong here in the first place. Change the signature instead: @addr becomes an input-only parameter, and the resolved address is always returned through a separate `char **resolved` output, unconditionally owned by the caller on success (0), whether or not resolution actually happened. Every call site can now just target a __cleanup_free local and never think about ownership again. nvmf_resolve_addr() is static with exactly these four in-file callers, so there's no external API to preserve. Signed-off-by: Daniel Wagner <wagi@monom.org>
Both capture() helpers hand tmpfile()'s return straight to a nonnull stream parameter (shr_spinner()/shr_print_word_wrapped() via the function-pointer callback, or directly) without checking it, and then rewind()/fread()/fclose() it regardless. A failing tmpfile() (exhausted fds, unwritable TMPDIR) would feed all of those a NULL FILE*. Signed-off-by: Daniel Wagner <wagi@monom.org>
test_mi_aem_ep_based_failure_helper() points peer->tx_data at its own stack-local fn_data for the duration of the enable/process calls, but never cleared it before returning, leaving peer->tx_data dangling into a dead stack frame for whatever runs next against the same peer. Signed-off-by: Daniel Wagner <wagi@monom.org>
read_file()'s own doc comment promises "NULL on error (errno set)", which fopen()/fseek()/ftell() reliably uphold on their failure paths, but malloc() failing doesn't carry the same guarantee. The caller reads errno straight from a failed read_file() call to build its error message, so honor the documented contract on this path too. Signed-off-by: Daniel Wagner <wagi@monom.org>
Three of this function's error paths compute the errno to report (from unlink()/a recursive call, or ENAMETOOLONG) and then call closedir() before returning -1 -- but closedir() isn't guaranteed to leave errno alone, so it could overwrite the very value the caller is about to read. Save it across the closedir() call. Signed-off-by: Daniel Wagner <wagi@monom.org>
json_create_object()'s return was passed straight into json_object_add_value_string()/_int() without a check. OOM-only, but cheap to guard. Signed-off-by: Daniel Wagner <wagi@monom.org>
Several unrelated issues clang-analyze flagged in the same file: - wdc_do_cap_telemetry_log() hand-rolled its output write as a loop that just `break`s on a failed write() without ever recording the failure, so the command went on to fsync() and report success after writing a truncated file. Replace it with shr_write_all(), the repository's existing complete-write helper (retries on EINTR/EAGAIN, treats a zero-byte write as an error instead of spinning forever, returns -errno), and only fsync() when the write itself succeeded. - wdc_get_c0_log_page(), wdc_get_ca_log_page() and wdc_cu_smart_log() discard nvme_get_pci_ids()'s return entirely; on failure that leaves device_id/read_device_id uninitialized right before it's used as a switch selector. Zero-initialize them so a failed lookup deterministically falls through to the existing default case instead of switching on indeterminate memory. - wdc_get_fw_act_history_C2()'s equivalent call and wdc_read_debug_directory()'s return are also unconditionally overwritten before ever being read -- plain dead stores, dropped. - wdc_get_enc_drive_capabilities(): `ret = -1` before two `goto out`s is pointless, the function returns `capabilities`, not `ret`. - wdc_get_c0_log_page(): a leftover `length = sizeof(...)` right before the variable is reassigned from `hdr_ptr->length` usage. Signed-off-by: Daniel Wagner <wagi@monom.org>
Same underlying issues as the WDC sibling this plugin shares code with: - sndk_do_cap_telemetry_log()'s hand-rolled write loop only `break`s on a failed write(), discarding the error, so the command reports success after writing a truncated file -- and on success it still unconditionally overwrote err with the ETDAS-bit cleanup's result, which could mask an even earlier fsync() failure, and returned early out of that cleanup block on its own error without ever reaching the free(log)/close(output) below. Replace the loop with shr_write_all(), and run the ETDAS cleanup unconditionally while preserving whichever error came first. - A discarded `ret = sndk_get_pci_ids(...)` that's overwritten before ever being checked. Signed-off-by: Daniel Wagner <wagi@monom.org>
sndk_get_pci_ids(): a `ret = 0` immediately overwritten by the next read(). sndk_get_enc_drive_capabilities(): same dead `ret = -1` before `goto out` pattern as its WDC counterpart -- the function returns `capabilities`, not `ret`. Signed-off-by: Daniel Wagner <wagi@monom.org>
Each of the Data Area 1 Stats, Data Area 1 Event FIFO and Data Area 2 Event FIFO blocks set m_512_sz/m_512_off right after da1_sz/da1_off, to the exact same value -- but both are unconditionally reassigned a few lines down (inside the alignment-check ifs) before ever being read. Only the Data Area 1 Event FIFO instance was flagged by scan-build; the Data Area 2 one is textually identical and has the same dead stores, so fix it too. Signed-off-by: Daniel Wagner <wagi@monom.org>
`description = ""` only repoints the local parameter, it never touches the caller's buffer -- the caller was left with whatever was already in the buffer instead of an empty string. Write into the buffer instead, the same way the non-empty branch does. Signed-off-by: Daniel Wagner <wagi@monom.org>
Immediately overwritten by the next libnvme_get_log_dynamic_chunk() call before ever being read. Signed-off-by: Daniel Wagner <wagi@monom.org>
Its return was assigned to err and then immediately overwritten by the next libnvme_exec_admin_passthru() call before ever being checked -- on failure that leaves item->nsid at the zero from the earlier memset() and sends Identify Namespace with NSID 0 instead of reporting the real error. Signed-off-by: Daniel Wagner <wagi@monom.org>
The restore-config call's return is never read afterward. Signed-off-by: Daniel Wagner <wagi@monom.org>
Every case of the switch immediately below unconditionally reassigns last_block (including the NVME_TELEMETRY_DA_1 case, which sets it to the exact same value), so the declaration-time initializer is never read. Signed-off-by: Daniel Wagner <wagi@monom.org>
Unconditionally overwritten by `at_line_start = false` a couple of lines down, after the word is written, before ever being read. Signed-off-by: Daniel Wagner <wagi@monom.org>
igaw
force-pushed
the
clang-analyze-fixes
branch
from
September 10, 2026 16:37
e3479e6 to
a39420d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
STS values above 64 can silently lose tag data, and the new OCP NULL checks still fall through to invalid accesses.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
plugins/ocp/ocp-telemetry-decode.c:1816
- The NULL case still falls through to
get_telemetry_das_offset_and_size()and pointer arithmetic onptelemetry_bufferat lines 1832–1837, so the newly added check only delays the invalid access. Return an error here or handle NULL once before selecting the output path.
if (ptelemetry_buffer == NULL) {
printf("skip generic_structure_parser\n");
} else {
- Files reviewed: 25/25 changed files
- Comments generated: 5
- Review effort level: Balanced
Comment on lines
+962
to
+963
| if (sts >= 80) | ||
| cdw2 = 0; |
Comment on lines
+1693
to
+1695
| if (ptelemetry_buffer == NULL) { | ||
| printf("skip generic_structure_parser\n"); | ||
| } else { |
Comment on lines
+189
to
+191
| if (sts > 80) { | ||
| nvme_show_error("Storage tag size larger than reference tag width"); | ||
| return -ECLI_INVALID_TAGS; |
Comment on lines
+96
to
+100
| cmd = (struct libnvme_passthru_cmd){ 0 }; | ||
| nvme_init_var_size_tags(&cmd, NVME_NVM_PIF_32B_GUARD, 80, reftag, storage_tag); | ||
| check(cmd.cdw14 == (__u32)reftag, "cdw14 %#x, expected %#x", cmd.cdw14, (__u32)reftag); | ||
| check(cmd.cdw3 == 0, "cdw3 %#x, expected 0", cmd.cdw3); | ||
| check(cmd.cdw2 == 0, "cdw2 %#x, expected 0", cmd.cdw2); |
Comment on lines
+195
to
+197
| def test_32b_guard_sts_at_new_limit_succeeds(self): | ||
| """32B Guard: STS=80 leaves a 0-bit ref tag, still valid.""" | ||
| self._verify(sts=80, pif=PIF_32B_GUARD, ref_tag=0, storage_tag=0) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
address some of the https://monom.org/linux-nvme/clang-analyze/current/ reports