Fixes for various memory leaks and other issues#1176
Open
vojtechtrefny wants to merge 50 commits intostoraged-project:masterfrom
Open
Fixes for various memory leaks and other issues#1176vojtechtrefny wants to merge 50 commits intostoraged-project:masterfrom
vojtechtrefny wants to merge 50 commits intostoraged-project:masterfrom
Conversation
The inner loops formatting EUI64 and NGUID hex strings reused the outer loop variable 'i', corrupting the outer iteration over namespace identify descriptors. Descriptors appearing after EUI64 or NGUID could be skipped as a result. Use a separate loop variable 'j' for the inner loops. Co-Authored-By: Claude Opus 4.6 <[email protected]>
fgetc returns ASCII character values. Comparing to integer 1 (SOH control character) instead of '1' (ASCII 49) means the "already online" check never triggers in both bd_s390_dasd_online and bd_s390_zfcp_online. Co-Authored-By: Claude Opus 4.6 <[email protected]>
next_arg was initialized to 4 instead of 8, causing subsequent arguments (-n, -V, etc.) to overwrite the --compression and --deduplication options in the argv array. This silently dropped the user's compression/deduplication settings. Also update the test to pass explicit non-default deduplication=False to verify arguments are correctly forwarded. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When a non-NULL connection was provided, the ListNames D-Bus call was inside the else block and never executed. ret remained NULL, causing g_variant_get_child_value(NULL, 0) to crash. Move the ListNames call outside the else block so it executes regardless of how the connection was obtained. Co-Authored-By: Claude Opus 4.6 <[email protected]>
label is a gchar** parameter. g_free(label) was incorrectly freeing the pointer-to-pointer instead of the allocated string. This leaked the label string and passed an invalid address to g_free. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The loop checking plugin availability overwrote the 'missing' flag each iteration without short-circuiting. This meant only the last plugin's availability was checked. Add the !missing condition to match the require_plugins branch. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When dlsym failed for a plugin function, the generated code logged a warning but left the function pointer as NULL, overwriting the stub. Subsequent calls to that API function would segfault instead of returning a graceful GError. Restore the stub function pointer on dlsym failure. Co-Authored-By: Claude Opus 4.6 <[email protected]>
mnt_table_find_source and mnt_table_find_target return pointers owned by the table. Calling mnt_free_fs on them frees table-owned memory, causing a double-free when mnt_free_table later frees its entries. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Both FEATURES_VDO and FEATURES_WRITECACHE were defined as 0, making their bitmasks identical and indistinguishable. Change FEATURES_WRITECACHE to 1 so it maps to the correct index in the features array and produces a distinct bitmask. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The device is already embedded in the table string passed via --table. Passing it as an additional argv element is not valid dmsetup syntax. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Passphrases and volume keys in BDCryptoKeyslotContext were freed with g_free without zeroing first, leaving sensitive material in freed heap memory. Use explicit_bzero to wipe the buffers before freeing. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When crypt_activate_by_passphrase failed to find the key slot, the function returned without calling crypt_free(cd). Co-Authored-By: Claude Opus 4.6 <[email protected]>
In bd_crypto_luks_add_key and bd_crypto_luks_change_key, key_buf from crypt_keyfile_device_read was leaked when the ncontext validation failed. In bd_crypto_opal_reset_device, key_buf was leaked on the crypt_wipe_hw_opal error path. Add crypt_safe_free(key_buf) to the affected error paths. Co-Authored-By: Claude Opus 4.6 <[email protected]>
ioctl returns -1 and sets errno, but the code used strerror_l(-ret) which always evaluates to strerror_l(1) (EPERM) regardless of the actual error. Use errno instead. Co-Authored-By: Claude Opus 4.6 <[email protected]>
ret is 0 at this point (from successful crypt_load), so strerror_l(-ret) produced "Success" in the error message. This is a type-check error, not a syscall failure, so the strerror is not appropriate. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Both bd_crypto_close and bd_part_close set c_locale to (locale_t) 0 without calling freelocale first, leaking the locale object on every plugin unload. Co-Authored-By: Claude Opus 4.6 <[email protected]>
g_key_file_new was called but the config was not freed before returning FALSE when g_key_file_load_from_file failed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
On reload, plugins[i].spec.so_name was set to NULL without freeing the g_strdup-ed string from the previous load, leaking memory for each plugin on every reload. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The error parameter is (out)(optional) but was dereferenced directly with *error without a NULL check. Use a local GError like bd_lvm_vgs does to avoid crashing when the caller passes NULL for error. Co-Authored-By: Claude Opus 4.6 <[email protected]>
g_prefix_error was called on the caller's error instead of &l_error. The subsequent g_propagate_error then overwrote it, losing the prefix message. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Inside the if (!table) block, table is NULL, so g_hash_table_destroy(table) is incorrect. Co-Authored-By: Claude Opus 4.6 <[email protected]>
strchr returns NULL when '\n' is not found. Adding 1 to NULL produces (char*)1, which passes the while NULL check and causes a segfault on dereference. Check strchr return before incrementing. Co-Authored-By: Claude Opus 4.6 <[email protected]>
dm_task_destroy was not called when dm_task_get_names returned NULL or names->dev was 0, leaking the DM task resource. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When fdisk_parttype_get_name succeeded but fdisk_parttype_get_string failed, the already-allocated *type_name was leaked. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The file descriptor from _open_dev was not closed before returning when _nvme_alloc failed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Accessing host_nqn[strlen(host_nqn) - 1] when the string is empty reads at index -1 (underflow to SIZE_MAX), causing an out-of-bounds read. Check for empty string before accessing the last character. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The regex was not unreffed when validation of ver_string1 or ver_string2 failed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When g_io_channel_new_file succeeded but g_io_channel_write_chars failed, the channel was never unreffed. Split the combined condition so the write failure path can properly unref the channel. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Unlike bd_lvm_vginfo, bd_lvm_vgs did not request vg_exported, so the exported field was always FALSE in the returned data. Add the field and update the expected item count from 9 to 10. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When bd_md_canonicalize_uuid failed and returned NULL, the error was set but the function continued, potentially overwriting the error with a subsequent mdadm call. Check the return value and propagate the error, matching the pattern used in bd_md_examine. Co-Authored-By: Claude Opus 4.6 <[email protected]>
g_free(dep_devs) only freed the array pointer, leaking the already-assigned device name strings. Use g_strfreev to free both the strings and the array. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The context parameter in bd_crypto_luks_resume was annotated as (nullable) but was unconditionally dereferenced, causing a NULL pointer dereference if NULL was passed. Resume requires credentials, so remove the (nullable) annotation. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When wipe was TRUE and succeeded, bd_utils_report_finished was never called because it was only in the else branch. Move it after the if/else block so it runs on both paths. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add bounds validation for the plugin parameter in bd_get_plugin_soname and bd_get_plugin_name to prevent out-of-bounds array access, matching the pattern already used by bd_is_plugin_available. Also validate plugin_name from user-supplied require_plugins in load_plugins. Use (guint64) 1 instead of int literal 1 in required_plugins_mask bit shifts to avoid undefined behavior if the number of plugins exceeds 31. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Change 'enabled' from gboolean (gint) to guint to match the %u format specifier used with sscanf in _lvm_devices_enabled. Using %u with a gboolean pointer is a type mismatch since gboolean is typedef'd to gint. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The output captured by bd_utils_exec_and_capture_output was never used, causing a memory leak. Switch to bd_utils_exec_and_report_error which doesn't capture the output at all. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Change pv_list_len from guint8 to guint so it is not silently truncated at 255 entries. While unlikely in practice, g_strv_length returns guint and the value is used to size the args array allocation. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The 'item' variable was declared g_autofree but reassigned to a new g_match_info_fetch_named result without freeing the previous value. Since g_autofree only frees the final value at scope exit, the first allocation was leaked. Replace g_autofree with explicit g_free calls at each reassignment and on every exit path for clarity. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Include ':' in the strstr search patterns so that the subsequent strchr call for ':' is guaranteed to find a match, preventing a potential NULL pointer dereference. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The error message incorrectly said "unmount" instead of "mount" for unsupported extra arguments in bd_fs_mount. Co-Authored-By: Claude Opus 4.6 <[email protected]>
bd_fs_ntfs_get_info used BD_FS_ERROR_NOT_MOUNTED when the device was actually mounted. Use BD_FS_ERROR_FAIL instead since the semantics were inverted. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When the regex failed to match the btrfs filesystem show output, the function returned NULL without setting the error, making it impossible for callers to distinguish between "no info" and "parse error". Co-Authored-By: Claude Opus 4.6 <[email protected]>
Use g_new0 instead of g_new to zero-initialize the struct, consistent with all other info struct allocations in the codebase. This prevents uninitialized fields if new members are added in the future. Co-Authored-By: Claude Opus 4.6 <[email protected]>
In both bd_fs_get_mountpoint and bd_fs_is_mountpoint, when mnt_table_set_cache fails, the cache object was not freed before returning. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Both bd_swap_swapon and bd_swap_swapoff called bd_utils_report_finished with the error message on failure, then fell through to call it again with "Completed". Add early return on error paths to prevent the duplicate call. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The error message incorrectly said "Could not set zFCP device online" in the offline function. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The format string had a hardcoded "/dev/" prefix, but the variable already contains the full path including "/dev/" when needed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The error parameter is optional but was dereferenced directly via (*error)->message when the WWPN directory was not found. Use a local l_error variable and g_propagate_error, consistent with the rest of the function. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Both bd_part_set_part_name and bd_part_set_part_uuid called bd_utils_report_started but returned without calling bd_utils_report_finished when get_device_context failed. Also used error directly instead of &l_error, inconsistent with the rest of the function. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Several variables allocated each loop iteration were never freed before reassignment: scsidev, scsidel (g_strdup_printf), and fcphbasysfs, fcpwwpnsysfs, fcplunsysfs (getline). Free them at the end of each iteration. The getline-allocated 'line' buffer and the scsifd file handle were also leaked on all error return paths inside the loop. Co-Authored-By: Claude Opus 4.6 <[email protected]>
📝 WalkthroughWalkthroughThis pull request improves memory safety, error handling, and resource cleanup across the libblockdev library. Changes include secure zeroing of sensitive data in crypto operations, proper cleanup of allocated resources on error paths, fixes to string parsing logic, adjustments to command output field handling in LVM, and architectural improvements to status checks and error propagation patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/plugins/fs/btrfs.c (2)
369-372:⚠️ Potential issue | 🟡 MinorAdd the missing space in the new multi-device error message.
The concatenated literals currently render
).Filesystem plugin...to callers.✏️ Suggested fix
- "Btrfs filesystem mounted on %s spans multiple devices (%"G_GUINT64_FORMAT")." \ - "Filesystem plugin is not suitable for multidevice Btrfs volumes, please use " \ + "Btrfs filesystem mounted on %s spans multiple devices (%"G_GUINT64_FORMAT"). " \ + "Filesystem plugin is not suitable for multidevice Btrfs volumes, please use " \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/fs/btrfs.c` around lines 369 - 372, The error string passed to g_set_error in the Btrfs code (g_set_error with BD_FS_ERROR/BD_FS_ERROR_FAIL) concatenates two literals without a space so callers see ").Filesystem"; update the string literal that includes mpoint and num_devices (the one beginning "Btrfs filesystem mounted on %s spans multiple devices...") to include a space between the closing parenthesis and "Filesystem" (e.g., add " " or include the space at the start of the second literal) so the rendered message reads "). Filesystem plugin..." and keep mpoint and num_devices placement unchanged.
321-325:⚠️ Potential issue | 🔴 CriticalChange hardcoded
devid 1todevid \d+in regex pattern.Single-device btrfs filesystems can have
devidother than 1 after device replacement or removal operations. Device IDs are assigned sequentially and not renumbered when devices are removed, so a surviving device may retain a higher devid (e.g., 2 or 3). The current regex will fail to parse valid single-device filesystems in these scenarios.Update line 325:
- "devid\\s+1\\s+size\\s+(?P<size>\\S+)\\s+\\S+"; + "devid\\s+\\d+\\s+size\\s+(?P<size>\\S+)\\s+\\S+";(Note: The "Also applies to: 352-355" in the original comment appears to be incorrect; those lines do not contain regex patterns and only one occurrence of this pattern exists in the file.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/fs/btrfs.c` around lines 321 - 325, The regex stored in the pattern variable currently hardcodes "devid 1" which fails for single-device btrfs filesystems whose device id may be >1; update the pattern in the declaration of pattern (gchar const * const pattern) to replace "devid\\s+1" with "devid\\s+\\d+" so the regex accepts any numeric devid (keep the rest of the pattern unchanged).src/plugins/s390.c (1)
806-818:⚠️ Potential issue | 🟡 MinorThe
getline()failure paths still leak the read buffers.These branches free the surrounding objects, but they still return without releasing
fcphbasysfs,fcpwwpnsysfs, orfcplunsysfswhen the read itself fails.♻️ Suggested cleanup
rc = getline (&fcphbasysfs, &len, fd); if (rc == -1) { g_set_error (&l_error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Failed to read value from %s: %m", hba_path); fclose (fd); + g_free (fcphbasysfs); g_free (hba_path); g_free (fcpsysfs); g_free (scsidev); fclose (scsifd); g_free (line); @@ rc = getline (&fcpwwpnsysfs, &len, fd); if (rc == -1) { g_set_error (&l_error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Failed to read value from %s", wwpn_path); + g_free (fcpwwpnsysfs); g_free (wwpn_path); g_free (fcphbasysfs); g_free (fcpsysfs); g_free (scsidev); fclose (fd); @@ rc = getline (&fcplunsysfs, &len, fd); if (rc == -1) { g_set_error (&l_error, BD_S390_ERROR, BD_S390_ERROR_DEVICE, "Failed to read value from %s", lun_path); fclose (fd); + g_free (fcplunsysfs); g_free (lun_path); g_free (fcpwwpnsysfs); g_free (fcphbasysfs); g_free (fcpsysfs); g_free (scsidev);Also applies to: 841-854, 878-892
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/s390.c` around lines 806 - 818, The getline() failure branches leak the per-read buffers (fcphbasysfs, fcpwwpnsysfs, fcplunsysfs); update each error path (the block that handles rc == -1 after getline calls in this function) to free any of those allocated buffers before returning: call g_free on fcphbasysfs, fcpwwpnsysfs, and fcplunsysfs as applicable, in addition to the existing frees (hba_path, fcpsysfs, scsidev, line) and fclose calls, and then propagate the error and return FALSE; apply the same fix to the other getline failure blocks in this function (the similar branches handling read errors).src/lib/blockdev.c.in (1)
498-506:⚠️ Potential issue | 🟡 Minor
bd_ensure_init(NULL, …)still misses the default-plugin exception.Off s390(x),
load_plugins()intentionally skipsBD_PLUGIN_S390for the default "load everything" case, but this fast-path check still requires it. That meansbd_ensure_init(NULL, …)re-entersload_plugins()on every call instead of returning early on already-initialized non-s390 builds.Suggested fix
- for (plugin=BD_PLUGIN_LVM; !missing && plugin != BD_PLUGIN_UNDEF; plugin++) - missing = !bd_is_plugin_available(plugin); + for (plugin = BD_PLUGIN_LVM; !missing && plugin != BD_PLUGIN_UNDEF; plugin++) { +#if !defined(__s390__) && !defined(__s390x__) + if (plugin == BD_PLUGIN_S390) + continue; +#endif + missing = !bd_is_plugin_available(plugin); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/blockdev.c.in` around lines 498 - 506, The fast-path in bd_ensure_init (checking 'initialized' and iterating plugins when require_plugins is NULL) incorrectly treats BD_PLUGIN_S390 as required on non-s390 builds, causing repeated re-entry to load_plugins; update the default-plugin loop (the branch that iterates plugin from BD_PLUGIN_LVM to BD_PLUGIN_UNDEF) to skip BD_PLUGIN_S390 the same way load_plugins does (e.g., if (plugin == BD_PLUGIN_S390) continue;) so the check matches load_plugins' behavior and doesn’t mark S390 as missing on builds where it is intentionally omitted.src/plugins/lvm/lvm.c (1)
1594-1614:⚠️ Potential issue | 🟡 MinorUse
%uforpv_list_len.On line 1595,
pv_list_lenis declared asguint(unsigned int). On line 1613,g_strdup_printf("%d", pv_list_len)uses the signed integer format specifier%d, which is a varargs type mismatch and undefined behavior. Use%uinstead.Suggested fix
- type_str = g_strdup_printf ("%d", pv_list_len); + type_str = g_strdup_printf ("%u", pv_list_len);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/lvm/lvm.c` around lines 1594 - 1614, In bd_lvm_lvcreate, the g_strdup_printf call that builds type_str uses the signed "%d" for pv_list_len (a guint), causing a varargs mismatch; update the format specifier to "%u" (or use G_GUINT32_FORMAT/G_GUINT64_FORMAT as appropriate) in the g_strdup_printf call that creates type_str so it matches the guint pv_list_len.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/blockdev.c.in`:
- Around line 343-347: The loop over require_plugins currently only skips
plugin_name >= BD_PLUGIN_UNDEF but still allows negative values and silently
ignores out-of-range positives; change the guard in the loop that builds
required_plugins_mask to explicitly validate plugin_name is within the valid
enum range (0 <= plugin_name && plugin_name < BD_PLUGIN_UNDEF) before using it
for array indexing or bit shifts, and if any plugin_name is invalid immediately
treat the request as failed (propagate an error/return FALSE from bd_init or the
caller path used here) so invalid plugin IDs reject initialization instead of
being half-handled; update the same validation logic in the other two locations
noted (the blocks around the other two loops at 765-767 and 781-783) to use the
same bounds check and failure handling.
In `@src/plugins/part.c`:
- Around line 310-312: bd_part_close currently calls freelocale(c_locale)
unconditionally which is UB if newlocale failed and c_locale is (locale_t)0;
update bd_part_close to check that c_locale is non-zero/NULL before calling
freelocale and only reset c_locale to (locale_t)0 after a successful free. Refer
to the c_locale variable and bd_part_close function (and newlocale
initialization sites) when adding the simple guard around freelocale to safely
handle init failures and repeated closes.
---
Outside diff comments:
In `@src/lib/blockdev.c.in`:
- Around line 498-506: The fast-path in bd_ensure_init (checking 'initialized'
and iterating plugins when require_plugins is NULL) incorrectly treats
BD_PLUGIN_S390 as required on non-s390 builds, causing repeated re-entry to
load_plugins; update the default-plugin loop (the branch that iterates plugin
from BD_PLUGIN_LVM to BD_PLUGIN_UNDEF) to skip BD_PLUGIN_S390 the same way
load_plugins does (e.g., if (plugin == BD_PLUGIN_S390) continue;) so the check
matches load_plugins' behavior and doesn’t mark S390 as missing on builds where
it is intentionally omitted.
In `@src/plugins/fs/btrfs.c`:
- Around line 369-372: The error string passed to g_set_error in the Btrfs code
(g_set_error with BD_FS_ERROR/BD_FS_ERROR_FAIL) concatenates two literals
without a space so callers see ").Filesystem"; update the string literal that
includes mpoint and num_devices (the one beginning "Btrfs filesystem mounted on
%s spans multiple devices...") to include a space between the closing
parenthesis and "Filesystem" (e.g., add " " or include the space at the start of
the second literal) so the rendered message reads "). Filesystem plugin..." and
keep mpoint and num_devices placement unchanged.
- Around line 321-325: The regex stored in the pattern variable currently
hardcodes "devid 1" which fails for single-device btrfs filesystems whose device
id may be >1; update the pattern in the declaration of pattern (gchar const *
const pattern) to replace "devid\\s+1" with "devid\\s+\\d+" so the regex accepts
any numeric devid (keep the rest of the pattern unchanged).
In `@src/plugins/lvm/lvm.c`:
- Around line 1594-1614: In bd_lvm_lvcreate, the g_strdup_printf call that
builds type_str uses the signed "%d" for pv_list_len (a guint), causing a
varargs mismatch; update the format specifier to "%u" (or use
G_GUINT32_FORMAT/G_GUINT64_FORMAT as appropriate) in the g_strdup_printf call
that creates type_str so it matches the guint pv_list_len.
In `@src/plugins/s390.c`:
- Around line 806-818: The getline() failure branches leak the per-read buffers
(fcphbasysfs, fcpwwpnsysfs, fcplunsysfs); update each error path (the block that
handles rc == -1 after getline calls in this function) to free any of those
allocated buffers before returning: call g_free on fcphbasysfs, fcpwwpnsysfs,
and fcplunsysfs as applicable, in addition to the existing frees (hba_path,
fcpsysfs, scsidev, line) and fclose calls, and then propagate the error and
return FALSE; apply the same fix to the other getline failure blocks in this
function (the similar branches handling read errors).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ae725d9-95a0-42da-9895-eea965773b78
📒 Files selected for processing (22)
scripts/boilerplate_generator.pysrc/lib/blockdev.c.insrc/lib/plugin_apis/crypto.apisrc/plugins/crypto.csrc/plugins/dm.csrc/plugins/fs/btrfs.csrc/plugins/fs/common.csrc/plugins/fs/mount.csrc/plugins/fs/ntfs.csrc/plugins/loop.csrc/plugins/lvm/lvm-common.csrc/plugins/lvm/lvm.csrc/plugins/mdraid.csrc/plugins/mpath.csrc/plugins/nvme/nvme-fabrics.csrc/plugins/nvme/nvme-info.csrc/plugins/part.csrc/plugins/s390.csrc/plugins/swap.csrc/utils/dbus.csrc/utils/exec.ctests/_lvm_cases.py
Comment on lines
343
to
+347
| for (i=0; *(require_plugins + i); i++) { | ||
| plugin_name = require_plugins[i]->name; | ||
| required_plugins_mask |= (1 << plugin_name); | ||
| if (plugin_name >= BD_PLUGIN_UNDEF) | ||
| continue; | ||
| required_plugins_mask |= ((guint64) 1 << plugin_name); |
There was a problem hiding this comment.
Reject invalid plugin IDs instead of half-handling them.
These guards only cover values >= BD_PLUGIN_UNDEF. Negative BDPlugin values can still reach array indexing and bit shifts, while out-of-range positive values are silently skipped. In the all-invalid case, bd_init() can return TRUE while leaving the library uninitialized. Please treat any out-of-range plugin as a failed request.
Minimal guard shape
- if (plugin_name >= BD_PLUGIN_UNDEF)
- continue;
+ if ((gint) plugin_name < 0 || plugin_name >= BD_PLUGIN_UNDEF) {
+ requested_loaded = FALSE;
+ continue;
+ }
...
- if (plugin >= BD_PLUGIN_UNDEF)
+ if ((gint) plugin < 0 || plugin >= BD_PLUGIN_UNDEF)
return NULL;
...
- if (plugin >= BD_PLUGIN_UNDEF)
+ if ((gint) plugin < 0 || plugin >= BD_PLUGIN_UNDEF)
return NULL;Also applies to: 765-767, 781-783
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/blockdev.c.in` around lines 343 - 347, The loop over require_plugins
currently only skips plugin_name >= BD_PLUGIN_UNDEF but still allows negative
values and silently ignores out-of-range positives; change the guard in the loop
that builds required_plugins_mask to explicitly validate plugin_name is within
the valid enum range (0 <= plugin_name && plugin_name < BD_PLUGIN_UNDEF) before
using it for array indexing or bit shifts, and if any plugin_name is invalid
immediately treat the request as failed (propagate an error/return FALSE from
bd_init or the caller path used here) so invalid plugin IDs reject
initialization instead of being half-handled; update the same validation logic
in the other two locations noted (the blocks around the other two loops at
765-767 and 781-783) to use the same bounds check and failure handling.
Comment on lines
310
to
312
| void bd_part_close (void) { | ||
| freelocale (c_locale); | ||
| c_locale = (locale_t) 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the relevant code sections in src/plugins/part.c
cat -n src/plugins/part.c | sed -n '290,320p'Repository: storaged-project/libblockdev
Length of output: 1108
🏁 Script executed:
# Check for c_locale declaration
rg -n "static.*locale_t.*c_locale" src/plugins/part.cRepository: storaged-project/libblockdev
Length of output: 116
🏁 Script executed:
# Look for all newlocale and freelocale calls
rg -n "newlocale|freelocale" src/plugins/part.cRepository: storaged-project/libblockdev
Length of output: 161
🌐 Web query:
POSIX freelocale() behavior with null or zero locale_t parameter
💡 Result:
POSIX specifies that freelocale(locale_t locobj) may only be used on a valid locale object handle returned by newlocale() or duplocale(). The standard explicitly says:
- “The behavior is undefined if … [locobj] is not a valid locale object handle” (and also if it is
LC_GLOBAL_LOCALE). [1] - It also defines no error return/reporting for
freelocale(), so an implementation is not required to treat invalid values as a harmless no-op. [1]
Therefore, passing NULL (or (locale_t)0) to freelocale() is undefined behavior under POSIX, because it is not a valid locale handle. This matches typical Linux documentation, which likewise says results are undefined if locobj is LC_GLOBAL_LOCALE or “not [a] valid locale object handle”. [2]
Practical rule: only call freelocale(loc) if loc is a handle you successfully obtained from newlocale()/duplocale(), and it is not LC_GLOBAL_LOCALE. [1][2]
Sources: [1] POSIX freelocale(3p) text (as reproduced by ManKier), [2] Linux man-pages newlocale(3)/freelocale(3) (man7.org).
Guard freelocale() against the zero sentinel.
If newlocale() ever fails, c_locale stays (locale_t) 0, so this unconditional freelocale(c_locale) invokes undefined behavior according to POSIX, which requires a valid locale handle. A simple guard ensures safe shutdown on init failure and repeated close calls.
Suggested fix
void bd_part_close (void) {
- freelocale (c_locale);
+ if (c_locale != (locale_t) 0)
+ freelocale (c_locale);
c_locale = (locale_t) 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void bd_part_close (void) { | |
| freelocale (c_locale); | |
| c_locale = (locale_t) 0; | |
| void bd_part_close (void) { | |
| if (c_locale != (locale_t) 0) | |
| freelocale (c_locale); | |
| c_locale = (locale_t) 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/part.c` around lines 310 - 312, bd_part_close currently calls
freelocale(c_locale) unconditionally which is UB if newlocale failed and
c_locale is (locale_t)0; update bd_part_close to check that c_locale is
non-zero/NULL before calling freelocale and only reset c_locale to (locale_t)0
after a successful free. Refer to the c_locale variable and bd_part_close
function (and newlocale initialization sites) when adding the simple guard
around freelocale to safely handle init failures and repeated closes.
tbzatek
approved these changes
Mar 6, 2026
| @@ -2260,7 +2260,7 @@ gboolean bd_crypto_luks_set_label (const gchar *device, const gchar *label, cons | |||
|
|
|||
| if (g_strcmp0 (crypt_get_type (cd), CRYPT_LUKS2) != 0) { | |||
| g_set_error (error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_TECH_UNAVAIL, | |||
Member
There was a problem hiding this comment.
You should be using g_set_error_literal() instead as g_set_error() expects a format string.
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Next batch of issues discovered by Claude.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements