Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Archive errors can be reported as success, and directory read failures can silently produce incomplete archives.
Review effort: Balanced
Findings: 3
Open (5)
What changed in this PR
Replaces shell-based vendor-plugin operations with in-process libarchive/libjq APIs, addressing command injection and reducing runtime tool dependencies.
Changes:
- Adds shared archive helpers and optional libarchive/libjq dependencies.
- Migrates vendor archiving, jq filtering, and diagnostics collection.
- Updates tests and documentation for optional-library builds.
| File | Description |
|---|---|
tests/cli/nvme_samsung_test.py |
Updates Samsung archive tests. |
tests/cli/nvme_mock_ipc.py |
Adds optional-library detection. |
tests/cli/micron/micron_vs_internal_log_mock_test.py |
Updates Micron archive tests. |
tests/cli/micron/micron_mock_test.py |
Exposes library detection helper. |
shared/meson.build |
Builds archive implementation or stub. |
shared/archive-util.h |
Declares archive APIs. |
shared/archive-util.c |
Implements libarchive operations. |
shared/archive-util-stub.c |
Adds unsupported-feature stubs. |
scripts/build.sh |
Disables new dependencies in constrained builds. |
plugins/wdc/wdc-nvme.c |
Replaces shell tar commands. |
plugins/solidigm/solidigm-telemetry.c |
Replaces jq subprocess usage. |
plugins/solidigm/solidigm-internal-logs.c |
Replaces ZIP subprocess usage. |
plugins/sandisk/sandisk-nvme.c |
Replaces tar command construction. |
plugins/samsung/samsung-nvme.c |
Uses shared archive and removal helpers. |
plugins/micron/micron-utils.h |
Removes spawn helper declaration. |
plugins/micron/micron-utils.c |
Removes spawn helper implementation. |
plugins/micron/micron-utils-linux.c |
Captures diagnostics directly. |
plugins/micron/micron-nvme.c |
Uses libarchive for packages. |
NEWS.md |
Documents security and dependency changes. |
meson.build |
Configures and links new dependencies. |
meson_options.txt |
Adds dependency feature options. |
Makefile |
Disables dependencies for static builds. |
Documentation/BUILDING.md |
Documents build options. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
igaw
force-pushed
the
add-libarchive
branch
3 times, most recently
from
October 2, 2026 19:04
341e880 to
ed97e01
Compare
igaw
force-pushed
the
add-libarchive
branch
3 times, most recently
from
October 2, 2026 22:35
7c780b1 to
4226f5f
Compare
Contributor
|
@igaw, Your suggestion to avoid the anti-pattern in the solidigm plugin started making sense to me, and I figured out an elegant way to call jq and nvme at the same command line while pointing to the same json file. I don't think we need the libjq dependency. Here is my proposal to eliminate the jq call: #4119 |
lgdacunh
reviewed
Oct 2, 2026
| nvme_show_perror("chdir back"); | ||
| /* no external process: archive cfg.out_dir directly */ | ||
| ret_cmd = shr_archive_create_dir(zip_path, cfg.out_dir, "", | ||
| SHR_ARCHIVE_ZIP); |
Contributor
There was a problem hiding this comment.
Looks like a great improvement to me!
Several vendor plugins archive log captures by building a tar/zip command and spawning it as an external process. Add shr_tar_create(), shr_tar_gz_create() and shr_archive_create_dir(), which write the archive directly via libarchive, so plugins no longer need to invoke an external tar/zip binary at all. libarchive is a new optional dependency (-Dlibarchive=auto, enabled by default when found). When not available, a stub implementation of the same functions returns -ENOTSUP, so callers fail cleanly instead of falling back to a shell. Signed-off-by: Daniel Wagner <wagi@monom.org>
sndk_do_cap_both_telemetry_log() built a tar command string by interpolating tar_file, host_file and controller_file and handed it to system(). tar_file is derived from a caller-supplied output path, so a path containing shell metacharacters ran arbitrary commands under the shell system() invokes, as root. Use the new shr_tar_create() instead, which writes the archive via libarchive directly: no shell is involved, so nothing in the path can be interpreted as command syntax. Signed-off-by: Daniel Wagner <wagi@monom.org>
wdc_do_sn730_get_and_tar(), wdc_vs_internal_fw_log() and wdc_do_drive_essentials() each built a tar command string from a caller-supplied output path or device-derived folder name and handed it to system(). A path containing shell metacharacters ran arbitrary commands under the shell system() invokes, as root. Add wdc_tar_dir_files(), which scans the staging directory and writes the archive via shr_tar_gz_create() (libarchive) directly: no shell is involved, so nothing in a path can be interpreted as command syntax. The optional remove_after flag reproduces "tar --remove-files" for the one caller that relied on it. Signed-off-by: Daniel Wagner <wagi@monom.org>
ZipAndRemoveDir() already spawned tar/zip through micron_run_spawn(), which uses no shell, so this was not an injection vector. But it still depended on external tar/zip/bsdtar binaries being installed. Use the new shr_archive_create_dir() (libarchive) instead, which removes that runtime dependency and the bsdtar-detection fallback (ZipWithBsdTar()) it needed. Signed-off-by: Daniel Wagner <wagi@monom.org>
solidigm_get_internal_log() already spawned zip through run_cmd(), which uses no shell, so this was not an injection vector. But it still depended on an external zip binary, plus a chdir()/getcwd() dance to make zip -r run relative to the log directory. Use the new shr_archive_create_dir() (libarchive) instead, which archives that directory's contents directly and removes the chdir() dance entirely. Signed-off-by: Daniel Wagner <wagi@monom.org>
compress_dump_files() already spawned tar and rm through run_command(), which uses no shell, so this was not an injection vector. But it still depended on external tar/rm binaries. Use the new shr_archive_create_dir() (libarchive) and the existing shr_rmdir_recursive() instead, which removes that runtime dependency and lets run_command(), list_dump_names(), free_dump_names() and struct dump_names be deleted as dead code. Signed-off-by: Daniel Wagner <wagi@monom.org>
micron_write_os_config_to_file() spawned uname, lsmod, cat and dmesg
with a static argv table (not injectable: nothing in it comes from
untrusted input), but each one is still an external process this tool
depends on being installed. Replace them with direct equivalents:
uname() for system info, a direct read of the same /proc file for
cat and lsmod ("lsmod" is itself a formatted view of /proc/modules),
and klogctl() for dmesg.
This was the last caller of micron_run_spawn(); remove it along with
its declaration, now dead code.
Signed-off-by: Daniel Wagner <wagi@monom.org>
micron_get_pcie_aer_errors() and micron_clear_pcie_aer_correctable_errors() were the last place in this plugin spawning an external program: both shelled out to setpci to access the PCIe Advanced Error Reporting status registers. Add a small shared/pci-util helper (shr_pci_config_read32/write32, shr_pci_find_ext_cap, shr_pci_open_class_config) for accessing a device's PCI config space through the sysfs "config" attribute's pread()/pwrite() support, and use it here instead. Going through the controller's sysfs "device" symlink to reach that attribute also means the BDF never needs to be separately resolved, dropping get_pcie_bdf() and its validation helper entirely. Signed-off-by: Daniel Wagner <wagi@monom.org>
Add libarchive to the optional-dependency table in BUILDING.md, and summarize the command-injection fixes and the external-process-to- library conversions across the WDC, SanDisk, Micron, Solidigm and Samsung plugins in NEWS.md. Signed-off-by: Daniel Wagner <wagi@monom.org>
The musl, minimal_static, static and nofabrics scripts/build.sh configs, and the Makefile's static target, each already explicitly disable every other optional dependency (openssl, keyutils, json-c, python, ...) so the build is deterministic regardless of what happens to be installed. Add libarchive to that list; left as the default auto, a CI image with the dev package installed would otherwise silently link it into what are meant to be minimal-dependency builds. minimal_static links statically (-static, --default-library=static) against musl, so an auto-detected libarchive .so fails the link with "attempted static link of dynamic object" instead of building without it. Signed-off-by: Daniel Wagner <wagi@monom.org>
micron_vs_internal_log_mock_test.py and nvme_samsung_test.py simulated an archive-tool failure by shadowing zip/tar/rm on PATH with a script that always exits 1. Now that those plugins archive via libarchive in-process instead of spawning an external tool, faking the tool has no effect, and the tests spuriously passed through to success. Use a destination that is itself an existing directory instead: archive_write_open_filename() fails with EISDIR regardless of privilege, giving the same deterministic failure without depending on PATH or a shell. Drop the samsung "rm fails, archive already written" sub-case: shr_rmdir_recursive() has no comparably clean, privilege- independent way to fail on demand. Signed-off-by: Daniel Wagner <wagi@monom.org>
nvme_samsung_test.py had no guard at all on its "-z" (archive) tests, and micron_vs_internal_log_mock_test.py's require_tool() checked whether zip/tar is on PATH. Both predate the switch to libarchive: archiving no longer spawns an external tool, so a host with zip/tar installed but nvme-cli built without libarchive (CONFIG_LIBARCHIVE unset, the disabled-feature stub linked in) now fails every one of those tests instead of skipping them. Add built_with_library() to nvme_mock_ipc.py, which greps `ldd <nvme_bin>` for a library's soname -- a statically linked nvme (no shared libraries at all, so no soname to find) is treated the same as one built without the feature, which matches reality since a static build always disables libarchive. Use it to skip the 8 samsung tests that need it and fix micron's require_tool() to check the same thing instead of shutil.which(). Signed-off-by: Daniel Wagner <wagi@monom.org>
checkpatch doesn't recognize __cleanup_* declarations (__cleanup_free, __cleanup_fd, ...) as variable declarations, so it misreports "Missing a blank line after declarations" whenever one sits next to another declaration. This is a known, recurring false positive, not a real style issue, and it fails the checkpatch job on any PR that adds adjacent __cleanup_* declarations. Filter that specific warning out of checkpatch's output before deciding pass/fail, instead of ignoring checkpatch's own exit code. Signed-off-by: Daniel Wagner <wagi@monom.org>
Collaborator
|
@igaw - I like the checkpatch filter you added in this PR, so I added the same filter to My PR adds a new script, |
This branch has not been deployed
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.
Do not call external programs, replace it with libarchive and libjq.
Fixes: #4092