From 8dd2abf0828cba8a052111502923414743197e3f Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Tue, 7 May 2024 14:50:37 -0500 Subject: [PATCH 01/28] Prep. for new release Signed-off-by: Tony Asleson --- NEWS | 8 ++++++++ configure.ac | 2 +- packaging/libstoragemgmt.spec.in | 2 ++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index f10fc310..ec98d5e3 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,13 @@ News for libStorageMgmt +1.10.0: May 7 2024 + * Fix hashlib.md5 usage + * Fix megaraid plugin for hba-mode + * Use ledmon library for controlling LED + * Add LED API interface for LED slot identifiers + * Fix megaraid plugin when no disks are attached + * smi-s plugin fix error message + 1.9.8: Apr 17 2023 * FIPS correction https://github.com/libstorage/libstoragemgmt/pull/528 diff --git a/configure.ac b/configure.ac index 9e55d0d5..ac9c7ede 100644 --- a/configure.ac +++ b/configure.ac @@ -2,7 +2,7 @@ dnl Process this file with autoconf to produce a configure script. dnl Copyright (C) 2011-2023 Red Hat, Inc. dnl See COPYING.LIB for the License of this software -AC_INIT([libstoragemgmt],[1.9.8],[libstoragemgmt-devel@lists.fedorahosted.org],[],[https://github.com/libstorage/libstoragemgmt/]) +AC_INIT([libstoragemgmt],[1.10.0],[libstoragemgmt-devel@lists.fedorahosted.org],[],[https://github.com/libstorage/libstoragemgmt/]) AC_CONFIG_SRCDIR([configure.ac]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) diff --git a/packaging/libstoragemgmt.spec.in b/packaging/libstoragemgmt.spec.in index 805fc8e1..db91721d 100644 --- a/packaging/libstoragemgmt.spec.in +++ b/packaging/libstoragemgmt.spec.in @@ -16,6 +16,7 @@ License: LGPLv2+ URL: https://github.com/libstorage/libstoragemgmt Source0: https://github.com/libstorage/libstoragemgmt/releases/download/%{version}/%{name}-%{version}.tar.gz Requires: python3-%{name} +Requires: ledmon-libs # Packages that have been removed Obsoletes: %{name}-netapp-plugin < %{version}-%{release} @@ -33,6 +34,7 @@ BuildRequires: bash-completion BuildRequires: libconfig-devel BuildRequires: systemd-devel BuildRequires: kernel-headers +BuildRequires: ledmon-devel %if %{with python2} BuildRequires: python2-six BuildRequires: python2-devel From c22bb3d875cb36a78f0c629473ed58bbe67f8ed0 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 09:39:56 -0500 Subject: [PATCH 02/28] Revert "ledmon-lib: correct ENUM" The enumeration change was done after 1.0.0 was release, thus it's an incompatible change. This reverts commit 42fdb156bc1e42739eff2bc21755bb7e73228374. --- c_binding/lsm_local_disk.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/c_binding/lsm_local_disk.cpp b/c_binding/lsm_local_disk.cpp index fc8c3ed0..c5706195 100644 --- a/c_binding/lsm_local_disk.cpp +++ b/c_binding/lsm_local_disk.cpp @@ -1124,7 +1124,7 @@ int ledlib_set(const char *disk_path, lsm_error **lsm_err, int led_state) { desired_state = current_state; switch (current_state) { - case LED_IBPI_PATTERN_LOCATE_AND_FAIL: + case LED_IBPI_PATTERN_LOCATE_AND_FAILED_DRIVE: // Both LEDs currently illuminated if (led_state == LSM_DISK_LED_STATUS_IDENT_OFF) { desired_state = LED_IBPI_PATTERN_FAILED_DRIVE; @@ -1142,7 +1142,7 @@ int ledlib_set(const char *disk_path, lsm_error **lsm_err, int led_state) { desired_state = LED_IBPI_PATTERN_NORMAL; break; case LSM_DISK_LED_STATUS_FAULT_ON: - desired_state = LED_IBPI_PATTERN_LOCATE_AND_FAIL; + desired_state = LED_IBPI_PATTERN_LOCATE_AND_FAILED_DRIVE; break; case LSM_DISK_LED_STATUS_FAULT_OFF: desired_state = current_state; @@ -1153,7 +1153,7 @@ int ledlib_set(const char *disk_path, lsm_error **lsm_err, int led_state) { // FAULT is currently on, IDENT off switch (led_state) { case LSM_DISK_LED_STATUS_IDENT_ON: - desired_state = LED_IBPI_PATTERN_LOCATE_AND_FAIL; + desired_state = LED_IBPI_PATTERN_LOCATE_AND_FAILED_DRIVE; break; case LSM_DISK_LED_STATUS_IDENT_OFF: desired_state = current_state; @@ -1302,7 +1302,7 @@ int LSM_DLL_EXPORT lsm_local_disk_led_status_get(const char *disk_path, *led_status = 0; led_state = led_slot_state(slot_entry); switch (led_state) { - case LED_IBPI_PATTERN_LOCATE_AND_FAIL: + case LED_IBPI_PATTERN_LOCATE_AND_FAILED_DRIVE: *led_status = LSM_DISK_LED_STATUS_IDENT_ON; *led_status |= LSM_DISK_LED_STATUS_FAULT_ON; break; @@ -1606,7 +1606,7 @@ uint32_t lsm_led_slot_status_get(lsm_led_slot *slot) { (LSM_DISK_LED_STATUS_FAULT_ON | LSM_DISK_LED_STATUS_IDENT_OFF); break; } - case (LED_IBPI_PATTERN_LOCATE_AND_FAIL): { + case (LED_IBPI_PATTERN_LOCATE_AND_FAILED_DRIVE): { translated = (LSM_DISK_LED_STATUS_IDENT_ON | LSM_DISK_LED_STATUS_FAULT_ON); } @@ -1643,7 +1643,7 @@ int lsm_led_slot_status_set(lsm_led_handle *handle, lsm_led_slot *slot, break; } case (LSM_DISK_LED_STATUS_FAULT_ON): { - state = LED_IBPI_PATTERN_LOCATE_AND_FAIL; + state = LED_IBPI_PATTERN_LOCATE_AND_FAILED_DRIVE; break; } case (LSM_DISK_LED_STATUS_IDENT_OFF): { @@ -1668,7 +1668,7 @@ int lsm_led_slot_status_set(lsm_led_handle *handle, lsm_led_slot *slot, break; } case (LSM_DISK_LED_STATUS_IDENT_ON | LSM_DISK_LED_STATUS_FAULT_ON): { - state = LED_IBPI_PATTERN_LOCATE_AND_FAIL; + state = LED_IBPI_PATTERN_LOCATE_AND_FAILED_DRIVE; break; } default: From 627526441c0974a40e188be3d1d9189e4d77897c Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 11:19:34 -0500 Subject: [PATCH 03/28] UNUSED_VALUE: dp assignment overwritten Signed-off-by: Tony Asleson --- c_binding/libsg.c | 1 - 1 file changed, 1 deletion(-) diff --git a/c_binding/libsg.c b/c_binding/libsg.c index 003bdf33..77b57a34 100644 --- a/c_binding/libsg.c +++ b/c_binding/libsg.c @@ -735,7 +735,6 @@ int _sg_parse_vpd_83(char *err_msg, uint8_t *vpd_data, (*dps)[*dp_count] = dp; ++*dp_count; - dp = NULL; } out: From 6ec1c05d50f3be8cd40d20c008c9394781695320 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 11:24:05 -0500 Subject: [PATCH 04/28] UNUSED_VALUE: wwpn assignment overwritten Signed-off-by: Tony Asleson --- c_binding/lsm_datatypes.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/c_binding/lsm_datatypes.cpp b/c_binding/lsm_datatypes.cpp index 2f26b99a..0d6bec90 100644 --- a/c_binding/lsm_datatypes.cpp +++ b/c_binding/lsm_datatypes.cpp @@ -971,7 +971,6 @@ static lsm_string_list *standardize_init_list(lsm_string_list *initiators) { break; } free(wwpn); - wwpn = NULL; } } } From f4c1c568dbc304aea30b70c2d3c74cad569cef3c Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 12:18:33 -0500 Subject: [PATCH 05/28] lsm_ipc: Check for overflow on send libstoragemgmt-1.10.0/c_binding/lsm_ipc.cpp:53:13: overflow_sink: "msg_size - written", which might have underflowed, is passed to "send(this->s, data.c_str() + written, msg_size - written, MSG_NOSIGNAL)". # 51| # 52| while (written < msg_size) { # 53|-> int wrote = send(s, data.c_str() + written, (msg_size - written), # 54| MSG_NOSIGNAL); // Prevent SIGPIPE on write # 55| if (wrote != -1) { Signed-off-by: Tony Asleson --- c_binding/lsm_ipc.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/c_binding/lsm_ipc.cpp b/c_binding/lsm_ipc.cpp index 22dd39c6..d6034bbc 100644 --- a/c_binding/lsm_ipc.cpp +++ b/c_binding/lsm_ipc.cpp @@ -50,10 +50,15 @@ int Transport::msg_send(const std::string &msg, int &error_code) { ssize_t msg_size = data.size(); while (written < msg_size) { - int wrote = send(s, data.c_str() + written, (msg_size - written), - MSG_NOSIGNAL); // Prevent SIGPIPE on write + ssize_t wrote = + send(s, data.c_str() + written, (msg_size - written), + MSG_NOSIGNAL); // Prevent SIGPIPE on write if (wrote != -1) { - written += wrote; + ssize_t t = written; + if (__builtin_add_overflow(t, wrote, &written)) { + error_code = EOVERFLOW; + break; + } } else { error_code = errno; break; From 82507d25a667e2ba2c613640e6d25e8a5ceb3e6f Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 12:30:13 -0500 Subject: [PATCH 06/28] Check for overflow in string_read libstoragemgmt-1.10.0/c_binding/lsm_ipc.cpp:79:60: warning[cpp/IntegerOverflow]: Unsanitized input from a remote resource flows into a subtraction operator (-), where it is used in integer arithmetic. This may result in an integer overflow vulnerability. # 77| while (amount_read < count) { # 78| ssize_t rd = # 79|-> recv(fd, buff, std::min(sizeof(buff), (count - amount_read)), # 80| MSG_WAITALL); # 81| if (rd > 0) { Signed-off-by: Tony Asleson --- c_binding/lsm_ipc.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/c_binding/lsm_ipc.cpp b/c_binding/lsm_ipc.cpp index d6034bbc..aebca678 100644 --- a/c_binding/lsm_ipc.cpp +++ b/c_binding/lsm_ipc.cpp @@ -72,19 +72,23 @@ int Transport::msg_send(const std::string &msg, int &error_code) { return rc; } -static std::string string_read(int fd, size_t count, int &error_code) { +static std::string string_read(int fd, ssize_t count, int &error_code) { char buff[4096]; - size_t amount_read = 0; + ssize_t amount_read = 0; std::string rc = ""; error_code = 0; while (amount_read < count) { - ssize_t rd = - recv(fd, buff, std::min(sizeof(buff), (count - amount_read)), - MSG_WAITALL); + ssize_t rd = recv( + fd, buff, std::min((ssize_t)(sizeof(buff)), (count - amount_read)), + MSG_WAITALL); if (rd > 0) { - amount_read += rd; + ssize_t t = amount_read; + if (__builtin_add_overflow(t, rd, &amount_read)) { + error_code = EOVERFLOW; + break; + } rc += std::string(buff, rd); } else { error_code = errno; @@ -106,7 +110,10 @@ std::string Transport::msg_recv(int &error_code) { if (len.size() && error_code == 0) { payload_len = strtoul(len.c_str(), NULL, 10); if (payload_len < 0x80000000) { /* Should be big enough */ - msg = string_read(s, payload_len, error_code); + ssize_t len = payload_len; + msg = string_read(s, len, error_code); + } else { + error_code = EOVERFLOW; } // fprintf(stderr, "<<< %s\n", msg.c_str()); } From cd4acfb40834846cccde2d434f441be1611a0e6a Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 15:35:10 -0500 Subject: [PATCH 07/28] lsm_local_disk: Ensure sas_addr is nul terminated Signed-off-by: Tony Asleson --- c_binding/lsm_local_disk.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/c_binding/lsm_local_disk.cpp b/c_binding/lsm_local_disk.cpp index c5706195..faac44b6 100644 --- a/c_binding/lsm_local_disk.cpp +++ b/c_binding/lsm_local_disk.cpp @@ -1244,6 +1244,8 @@ static void _sysfs_sas_addr_get(const char *blk_name, char *tp_sas_addr) { _SG_T10_SPL_SAS_ADDR_LEN); out: + /* ensure sas addr is always nul terminated */ + tp_sas_addr[_SG_T10_SPL_SAS_ADDR_LEN - 1] = '\0'; free(sysfs_sas_path); } @@ -1271,6 +1273,8 @@ static int _sas_addr_get(char *err_msg, const char *disk_path, } out: + /* regardless of how to finish, ensure string is always null terminated */ + tp_sas_addr[_SG_T10_SPL_SAS_ADDR_LEN - 1] = '\0'; if (fd >= 0) close(fd); return rc; From 87c155ada95df618f6d95f0664c5baa225f6b733 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 15:43:47 -0500 Subject: [PATCH 08/28] lsm_led_slot_iterator_get: fix memleak c_binding/lsm_local_disk.cpp:1527:5: alloc_fn: Storage is returned from allocation function "calloc". c_binding/lsm_local_disk.cpp:1527:5: var_assign: Assigning: "itr" = storage returned from "calloc(1UL, 32UL)". c_binding/lsm_local_disk.cpp:1539:9: leaked_storage: Variable "itr" going out of scope leaks the storage it points to. # 1537| *lsm_err = translate_led_to_lsm( # 1538| led_rc, "Unexpected return for 'led_slots_get'"); # 1539|-> return LSM_ERR_LIB_BUG; # 1540| } # 1541| Signed-off-by: Tony Asleson --- c_binding/lsm_local_disk.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/c_binding/lsm_local_disk.cpp b/c_binding/lsm_local_disk.cpp index faac44b6..3f2a0846 100644 --- a/c_binding/lsm_local_disk.cpp +++ b/c_binding/lsm_local_disk.cpp @@ -1540,6 +1540,7 @@ int lsm_led_slot_iterator_get(lsm_led_handle *handle, if (led_rc != LED_STATUS_SUCCESS) { *lsm_err = translate_led_to_lsm( led_rc, "Unexpected return for 'led_slots_get'"); + free(itr); return LSM_ERR_LIB_BUG; } From 09a9d71b1f5f3c2e2c0be1d7e78d529118b1cd38 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 15:55:48 -0500 Subject: [PATCH 09/28] lsm_plugin_info_get: reduce to 1 free location c_binding/lsm_mgmt.cpp:371:14: warning[cpp/DoubleFree]: Potential double call to free. _ may have already been freed by call to free. # 369| } # 370| } catch (const ValueException &ve) { # 371|-> free(*desc); # 372| *desc = NULL; # 373| free(*version); c_binding/lsm_mgmt.cpp:373:14: warning[cpp/DoubleFree]: Potential double call to free. _ may have already been freed by call to free. # 371| free(*desc); # 372| *desc = NULL; # 373|-> free(*version); # 374| *version = NULL; # 375| rc = log_exception(c, LSM_ERR_PLUGIN_BUG, "Unexpected type", ve.what()); Signed-off-by: Tony Asleson --- c_binding/lsm_mgmt.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/c_binding/lsm_mgmt.cpp b/c_binding/lsm_mgmt.cpp index cccc8ae0..0fa22eca 100644 --- a/c_binding/lsm_mgmt.cpp +++ b/c_binding/lsm_mgmt.cpp @@ -363,16 +363,17 @@ int lsm_plugin_info_get(lsm_connect *c, char **desc, char **version, if (!*desc || !*version) { rc = LSM_ERR_NO_MEMORY; - free(*desc); - free(*version); } } } catch (const ValueException &ve) { + rc = log_exception(c, LSM_ERR_PLUGIN_BUG, "Unexpected type", ve.what()); + } + + if (rc != LSM_ERR_OK) { free(*desc); *desc = NULL; free(*version); *version = NULL; - rc = log_exception(c, LSM_ERR_PLUGIN_BUG, "Unexpected type", ve.what()); } return rc; From d2bd19c18614a1ddcae5855a032dc0f3028b43c1 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 16:26:34 -0500 Subject: [PATCH 10/28] lsm_mgmt: Use std::move c_binding/lsm_mgmt.cpp:1067:58: copy_constructor_call: "response" is passed-by-value as parameter to "parse_job_response" when it could be moved instead. c_binding/lsm_ipc.hpp:165:21: moveable_type: Type "Value" is move-constructible. c_binding/lsm_mgmt.cpp:1067:58: remediation: Use "std::move"("response") instead of "response". # 1065| int rc = rpc(c, "volume_create", parameters, response); # 1066| if (LSM_ERR_OK == rc) { # 1067|-> *newVolume = (lsm_volume *)parse_job_response(c, response, rc, job, # 1068| (convert)value_to_volume); # 1069| } Signed-off-by: Tony Asleson --- c_binding/lsm_mgmt.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/c_binding/lsm_mgmt.cpp b/c_binding/lsm_mgmt.cpp index 0fa22eca..04e7c483 100644 --- a/c_binding/lsm_mgmt.cpp +++ b/c_binding/lsm_mgmt.cpp @@ -1065,8 +1065,8 @@ int lsm_volume_create(lsm_connect *c, lsm_pool *pool, const char *volumeName, int rc = rpc(c, "volume_create", parameters, response); if (LSM_ERR_OK == rc) { - *newVolume = (lsm_volume *)parse_job_response(c, response, rc, job, - (convert)value_to_volume); + *newVolume = (lsm_volume *)parse_job_response( + c, std::move(response), rc, job, (convert)value_to_volume); } return rc; } @@ -1095,7 +1095,7 @@ int lsm_volume_resize(lsm_connect *c, lsm_volume *volume, uint64_t newSize, int rc = rpc(c, "volume_resize", parameters, response); if (LSM_ERR_OK == rc) { *resizedVolume = (lsm_volume *)parse_job_response( - c, response, rc, job, (convert)value_to_volume); + c, std::move(response), rc, job, (convert)value_to_volume); } return rc; } @@ -1128,7 +1128,7 @@ int lsm_volume_replicate(lsm_connect *c, lsm_pool *pool, int rc = rpc(c, "volume_replicate", parameters, response); if (LSM_ERR_OK == rc) { *newReplicant = (lsm_volume *)parse_job_response( - c, response, rc, job, (convert)value_to_volume); + c, std::move(response), rc, job, (convert)value_to_volume); } return rc; } @@ -1364,7 +1364,7 @@ int lsm_access_group_create(lsm_connect *c, const char *name, std::map p; p["name"] = Value(name); - p["init_id"] = id; + p["init_id"] = std::move(id); p["init_type"] = Value((int32_t)init_type); p["system"] = system_to_value(system); p["flags"] = Value(flags); @@ -1429,7 +1429,7 @@ static int _lsm_ag_add_delete(lsm_connect *c, lsm_access_group *access_group, std::map p; p["access_group"] = access_group_to_value(access_group); - p["init_id"] = id; + p["init_id"] = std::move(id); p["init_type"] = Value((int32_t)init_type); p["flags"] = Value(flags); @@ -1797,7 +1797,7 @@ int lsm_fs_create(lsm_connect *c, lsm_pool *pool, const char *name, int rc = rpc(c, "fs_create", parameters, response); if (LSM_ERR_OK == rc) { - *fs = (lsm_fs *)parse_job_response(c, response, rc, job, + *fs = (lsm_fs *)parse_job_response(c, std::move(response), rc, job, (convert)value_to_fs); } return rc; @@ -1843,7 +1843,7 @@ int lsm_fs_resize(lsm_connect *c, lsm_fs *fs, uint64_t new_size_bytes, int rc = rpc(c, "fs_resize", parameters, response); if (LSM_ERR_OK == rc) { - *rfs = (lsm_fs *)parse_job_response(c, response, rc, job, + *rfs = (lsm_fs *)parse_job_response(c, std::move(response), rc, job, (convert)value_to_fs); } return rc; @@ -1870,8 +1870,8 @@ int lsm_fs_clone(lsm_connect *c, lsm_fs *src_fs, const char *name, int rc = rpc(c, "fs_clone", parameters, response); if (LSM_ERR_OK == rc) { - *cloned_fs = (lsm_fs *)parse_job_response(c, response, rc, job, - (convert)value_to_fs); + *cloned_fs = (lsm_fs *)parse_job_response(c, std::move(response), rc, + job, (convert)value_to_fs); } return rc; } @@ -2047,8 +2047,8 @@ int lsm_fs_ss_create(lsm_connect *c, lsm_fs *fs, const char *name, int rc = rpc(c, "fs_snapshot_create", parameters, response); if (LSM_ERR_OK == rc) { - *snapshot = (lsm_fs_ss *)parse_job_response(c, response, rc, job, - (convert)value_to_ss); + *snapshot = (lsm_fs_ss *)parse_job_response(c, std::move(response), rc, + job, (convert)value_to_ss); } return rc; } From 37be12ed0a1c4a496d0dc727fc8562e833c298fc Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Wed, 8 May 2024 16:40:51 -0500 Subject: [PATCH 11/28] Payload::deserialize: ensure tok null Signed-off-by: Tony Asleson --- c_binding/lsm_value_jsmn.hpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/c_binding/lsm_value_jsmn.hpp b/c_binding/lsm_value_jsmn.hpp index 272bc0de..cda7dc09 100644 --- a/c_binding/lsm_value_jsmn.hpp +++ b/c_binding/lsm_value_jsmn.hpp @@ -219,6 +219,11 @@ static int inc_token(int current, int amount, int max) { Value lsm_parse(jsmntok_t *tok, int start_tok, int end_tok, const char *j, int *consumed) { + if (!tok) { + throw ValueException( + "Invalid parameter: lsm_parse param 'tok' is null"); + } + int i = start_tok; int len = tok[i].end - tok[i].start; @@ -293,10 +298,12 @@ Value Payload::deserialize(const std::string &json_str) { if (rc < 0) { if (JSMN_ERROR_NOMEM == rc) { free(tok); + tok = NULL; num_tokens *= 2; continue; } else { free(tok); + tok = NULL; throw ValueException("In-valid json"); } } From af218ed691f7b71b4a2873724dde85576ef4cb75 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 08:47:10 -0500 Subject: [PATCH 12/28] uri_parser: Use std::move Signed-off-by: Tony Asleson --- c_binding/uri_parser.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/c_binding/uri_parser.hpp b/c_binding/uri_parser.hpp index b23652b2..21a85b6b 100644 --- a/c_binding/uri_parser.hpp +++ b/c_binding/uri_parser.hpp @@ -113,7 +113,7 @@ inline struct uri parse(std::string uri) { return rc; } - rc.scheme = scheme_tmp; + rc.scheme = std::move(scheme_tmp); auto remainder = uri.substr(req + 3); @@ -157,7 +157,7 @@ inline struct uri parse(std::string uri) { } // Only part left should be host, which is a ipv4, hostname, or ipv6 - rc.host = remainder; + rc.host = std::move(remainder); rc.valid = true; return rc; } From 44e1304aa44c52c94437a6dfc77a866104a00eac Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 09:31:09 -0500 Subject: [PATCH 13/28] utils.c: Re-work _file_exists Instead of using open, we'll leverage access which prevents us from potentially leaking a file descriptor. Signed-off-by: Tony Asleson --- c_binding/utils.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/c_binding/utils.c b/c_binding/utils.c index 1b2be5c2..e5806026 100644 --- a/c_binding/utils.c +++ b/c_binding/utils.c @@ -68,18 +68,12 @@ void _be_raw_to_hex(uint8_t *raw, size_t len, char *out) { } bool _file_exists(const char *path) { - int fd = -1; - assert(path != NULL); - fd = open(path, O_RDONLY); - if ((fd == -1) && (errno == ENOENT)) - return false; - - if (fd >= 0) { - close(fd); + if (access(path, F_OK) == 0) { + return true; } - return true; + return false; } int _read_file(const char *path, uint8_t *buff, ssize_t *size, From 93805fffd14dbb03bbcfb75a1372b9d7be9bd20b Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 09:48:56 -0500 Subject: [PATCH 14/28] lsm_daemon: Add close on fd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit daemon/lsm_daemon.c: scope_hint: In function ‘process_plugin’ daemon/lsm_daemon.c:551:9: warning[-Wanalyzer-fd-leak]: leak of file descriptor daemon/lsm_daemon.c:604:5: note: in expansion of macro ‘info’ daemon/lsm_daemon.c:604:5: note: in expansion of macro ‘info’ daemon/lsm_daemon.c:29: included_from: Included from here. daemon/lsm_daemon.c:18: included_from: Included from here. daemon/lsm_daemon.c:10: included_from: Included from here. # 549| /* The only real way to get here is failed strdup as # 550| setup_socket will exit on error. */ # 551|-> free(item); # 552| item = NULL; # 553| log_and_exit("strdup failed %s\n", full_name); The comment in correct, if the setup function encounters an error the daemon will exit, so adding an explicit close of the FD is redundant, but the static code analysis is unable to determine that. Signed-off-by: Tony Asleson --- daemon/lsm_daemon.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/daemon/lsm_daemon.c b/daemon/lsm_daemon.c index c7332ee7..09f14763 100644 --- a/daemon/lsm_daemon.c +++ b/daemon/lsm_daemon.c @@ -548,6 +548,11 @@ int process_plugin(void *p, char *full_name) { } else { /* The only real way to get here is failed strdup as setup_socket will exit on error. */ + + /* static code checkers incorrectly flag a failure to close fd */ + if (item->fd >= 0) { + close(item->fd); + } free(item); item = NULL; log_and_exit("strdup failed %s\n", full_name); From d10a358a62578de81d7f802bb2effc4c231ffaac Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 09:53:33 -0500 Subject: [PATCH 15/28] nfs.py: fix typo Signed-off-by: Tony Asleson --- plugin/nfs_plugin/nfs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/nfs_plugin/nfs.py b/plugin/nfs_plugin/nfs.py index 06bc20d2..db4790ea 100644 --- a/plugin/nfs_plugin/nfs.py +++ b/plugin/nfs_plugin/nfs.py @@ -62,7 +62,7 @@ def _export_id(path, auth_type, anon_uid, anon_gid, options): @staticmethod def _parse_options(optionstring): - """ Parse a comma separted option list into a dict """ + """ Parse a comma separated option list into a dict """ options = {} if optionstring is None: From f6ff3358730078d0cb919ecf5966a6cee2287e0c Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 12:34:30 -0500 Subject: [PATCH 16/28] Ensure string index in bounds plugin/simc/db.c:722:47: warning[-Wanalyzer-out-of-bounds]: stack-based buffer underwrite # 720| # 721| /* Remove the trailing ", " */ # 722|-> keys_str[keys_printed - strlen(", ") + 1] = '\0'; # 723|-> values_str[values_printed - strlen(", ") + 1] = '\0'; # 724| Signed-off-by: Tony Asleson --- plugin/simc/db.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/plugin/simc/db.c b/plugin/simc/db.c index 66685b09..3edde7eb 100644 --- a/plugin/simc/db.c +++ b/plugin/simc/db.c @@ -678,6 +678,15 @@ void _db_sql_trans_rollback(sqlite3 *db) { NULL /* don't parse output */); } +static void remove_trail_sep(int items_printed, char *s) { + int nul_pos = items_printed - strlen(", ") + 1; + if (nul_pos >= 0 && nul_pos < _BUFF_SIZE) { + s[nul_pos] = '\0'; + } else { + s[0] = '\0'; + } +} + int _db_data_add(char *err_msg, sqlite3 *db, const char *table_name, ...) { int rc = LSM_ERR_OK; char sql_cmd[_BUFF_SIZE * 4]; @@ -719,8 +728,8 @@ int _db_data_add(char *err_msg, sqlite3 *db, const char *table_name, ...) { va_end(arg); /* Remove the trailing ", " */ - keys_str[keys_printed - strlen(", ") + 1] = '\0'; - values_str[values_printed - strlen(", ") + 1] = '\0'; + remove_trail_sep(keys_printed, keys_str); + remove_trail_sep(values_printed, values_str); _snprintf_buff(err_msg, rc, out, sql_cmd, "INSERT INTO %s (%s) VALUES (%s);", table_name, keys_str, From 7a1fe927d7d06d4a03912f6e918aab53e3ee0a92 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 12:43:08 -0500 Subject: [PATCH 17/28] utils.c: Handle overflow on add and underflow sub plugin/simc/utils.c:164:9: overflow_sink: "needed - got", which might have underflowed, is passed to "read(fd, raw_data + got, needed - got)". [Note: The source code implementation of the function has been overridden by a builtin model.] # 162| # 163| while (got < needed) { # 164|-> cur_got = read(fd, raw_data + got, needed - got); # 165| if (cur_got < 0) { # 166| close(fd); and plugin/simc/utils.c:165:9: overflow_sink: "needed - got", which might have underflowed, is passed to "read(fd, raw_data + got, needed - got)". Signed-off-by: Tony Asleson --- plugin/simc/utils.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/plugin/simc/utils.c b/plugin/simc/utils.c index 6db5dcb5..5db654be 100644 --- a/plugin/simc/utils.c +++ b/plugin/simc/utils.c @@ -146,6 +146,7 @@ const char *_random_vpd(char *buff) { ssize_t cur_got = 0; size_t got = 0; size_t i = 0; + size_t tmp = 0; size_t needed = (_VPD_83_LEN - 1) / 2 - 1; /* Skip the first two digits. We will use '50'. */ /* Assuming _VPD_83_LEN is odd number */ @@ -161,12 +162,27 @@ const char *_random_vpd(char *buff) { return buff; while (got < needed) { - cur_got = read(fd, raw_data + got, needed - got); + + size_t remaining = 0; + if (__builtin_sub_overflow(needed, got, &remaining)) { + close(fd); + buff[0] = '\0'; + return buff; + } + + cur_got = read(fd, raw_data + got, remaining); if (cur_got < 0) { close(fd); + buff[0] = '\0'; + return buff; + } + + tmp = got; + if (__builtin_add_overflow(tmp, cur_got, &got)) { + close(fd); + buff[0] = '\0'; return buff; } - got += cur_got; } close(fd); From 9b487b4c0cd08908d6707e34e3f499db5fdf19a6 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 12:55:06 -0500 Subject: [PATCH 18/28] Fix memleak in led_slot_handle_get python_binding/lsm/_clib.c:739:9: leaked_storage: Variable "handle" going out of scope leaks the storage it points to. # 737| Py_XDECREF(err_msg_obj); # 738| Py_XDECREF(rc_obj); # 739|-> return PyErr_NoMemory(); # 740| } # 741| PyList_SET_ITEM(rc_list, 0, rc_obj); Signed-off-by: Tony Asleson --- python_binding/lsm/_clib.c | 1 + 1 file changed, 1 insertion(+) diff --git a/python_binding/lsm/_clib.c b/python_binding/lsm/_clib.c index ca42bc4c..8d7de08e 100644 --- a/python_binding/lsm/_clib.c +++ b/python_binding/lsm/_clib.c @@ -732,6 +732,7 @@ static PyObject *led_slot_handle_get(PyObject *self, PyObject *args, out: if (flag_no_mem == true) { + lsm_led_handle_free(handle); Py_XDECREF(rc_list); Py_XDECREF(err_no_obj); Py_XDECREF(err_msg_obj); From ae11dd0fd40cba75d3aa99444cacf021b5f3d1f9 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 12:58:55 -0500 Subject: [PATCH 19/28] Fix memory leak in led_slot_iterator_get python_binding/lsm/_clib.c:814:9: leaked_storage: Variable "lsm_err" going out of scope leaks the storage it points to. # 812| Py_XDECREF(err_msg_obj); # 813| Py_XDECREF(rc_obj); # 814|-> return PyErr_NoMemory(); # 815| } # 816| PyList_SET_ITEM(rc_list, 0, rc_obj); Signed-off-by: Tony Asleson --- python_binding/lsm/_clib.c | 1 + 1 file changed, 1 insertion(+) diff --git a/python_binding/lsm/_clib.c b/python_binding/lsm/_clib.c index 8d7de08e..2c6fa9f4 100644 --- a/python_binding/lsm/_clib.c +++ b/python_binding/lsm/_clib.c @@ -808,6 +808,7 @@ static PyObject *led_slot_iterator_get(PyObject *self, PyObject *args, out: if (flag_no_mem == true) { + lsm_error_free(lsm_err); Py_XDECREF(rc_list); Py_XDECREF(err_no_obj); Py_XDECREF(err_msg_obj); From 2d781e1baedc16a83965d14d6a4cd7b43bd5219f Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 13:09:05 -0500 Subject: [PATCH 20/28] Fix integer overflow /test/tester.c:114:9: overflow_sink: "len - got", which might have underflowed, is passed to "read(fd, raw_data + got, len - got)". [Note: The source code implementation of the function has been overridden by a builtin model.] # 112| # 113| while (got < len) { # 114|-> cur_got = read(fd, raw_data + got, len - got); # 115| if (cur_got < 0) { # 116| goto out; Signed-off-by: Tony Asleson --- test/tester.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/tester.c b/test/tester.c index bdd5fce3..52a0ea74 100644 --- a/test/tester.c +++ b/test/tester.c @@ -93,10 +93,11 @@ void generate_random(char *buff, uint32_t len) { uint32_t i = 0; ssize_t cur_got = 0; size_t got = 0; + size_t tmp = 0; int fd = -1; uint8_t *raw_data; - if (buff == NULL) + if (buff == NULL || len == 0) return; raw_data = (uint8_t *)malloc(len * sizeof(uint8_t)); @@ -115,7 +116,13 @@ void generate_random(char *buff, uint32_t len) { if (cur_got < 0) { goto out; } - got += cur_got; + + tmp = got; + if (__builtin_add_overflow(tmp, cur_got, &got)) { + close(fd); + buff[0] = '\0'; + return; + } } for (i = 0; i < (len - 1); ++i) { From 765a4e6459a3c58f3cc196a76ed6bfeaf908ec86 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 13:13:39 -0500 Subject: [PATCH 21/28] Change unused value test/tester.c:753:21: assigned_pointer: Assigning value "NULL" to "init_list" here, but that stored value is overwritten before it can be used. # 751| updated = NULL; # 752| } # 753|-> init_list = NULL; # 754| } # 755| Signed-off-by: Tony Asleson --- test/tester.c | 1 - 1 file changed, 1 deletion(-) diff --git a/test/tester.c b/test/tester.c index 52a0ea74..7ec63844 100644 --- a/test/tester.c +++ b/test/tester.c @@ -757,7 +757,6 @@ START_TEST(test_access_groups) { lsm_access_group_record_free(updated); updated = NULL; } - init_list = NULL; } if (group) { From 0217bfd361992000c6e6a9134f46ff2b5015743c Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 13:16:55 -0500 Subject: [PATCH 22/28] Clear pointer so we avoid double free Trying to work around static code analysis. Signed-off-by: Tony Asleson --- test/tester.c | 1 + 1 file changed, 1 insertion(+) diff --git a/test/tester.c b/test/tester.c index 7ec63844..df07b5ea 100644 --- a/test/tester.c +++ b/test/tester.c @@ -3603,6 +3603,7 @@ START_TEST(test_local_disk_serial_num_get) { if (lsm_err != NULL) lsm_error_free(lsm_err); free(serial_num); + serial_num = NULL; /* Test nonexistent disk */ rc = From 2bfc3130f2b9056beae28f67e40d742aa58fca8f Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Thu, 9 May 2024 13:25:18 -0500 Subject: [PATCH 23/28] Fix potential double free test/tester.c:3683:10: note[cpp/DoubleFree/test]: Potential double call to free. vpd83 may have already been freed by call to free. # 3681| "been set with non-NULL error message when disk not exist"); # 3682| lsm_error_free(lsm_err); # 3683|-> free(vpd83); # 3684| } # 3685| END_TEST Signed-off-by: Tony Asleson --- test/tester.c | 1 + 1 file changed, 1 insertion(+) diff --git a/test/tester.c b/test/tester.c index df07b5ea..7018390e 100644 --- a/test/tester.c +++ b/test/tester.c @@ -3670,6 +3670,7 @@ START_TEST(test_local_disk_vpd83_get) { if (lsm_err != NULL) lsm_error_free(lsm_err); free(vpd83); + vpd83 = NULL; /* Test non-exist disk */ rc = lsm_local_disk_vpd83_get(NOT_EXIST_SD_PATH, &vpd83, &lsm_err); From 2d22083446a7ab307813efba95d9aa23c713328c Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Fri, 10 May 2024 10:29:41 -0500 Subject: [PATCH 24/28] utils.c: Re-work _file_exists Instead of using open, we'll leverage access which prevents us from potentially leaking a file descriptor. Note: This is a copy from c_binding/utils.c, would be good to consolidate at some point. Signed-off-by: Tony Asleson --- plugin/simc/utils.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/plugin/simc/utils.c b/plugin/simc/utils.c index 5db654be..34bca982 100644 --- a/plugin/simc/utils.c +++ b/plugin/simc/utils.c @@ -43,18 +43,12 @@ int _get_db_from_plugin_ptr(char *err_msg, lsm_plugin_ptr c, sqlite3 **db) { * Copy from c_binding/utils.c, will remove if that was exposed out. */ bool _file_exists(const char *path) { - int fd = -1; - assert(path != NULL); - fd = open(path, O_RDONLY); - if ((fd == -1) && (errno == ENOENT)) - return false; - - if (fd >= 0) { - close(fd); + if (access(path, F_OK) == 0) { + return true; } - return true; + return false; } /* From 92c52a4cbd8722aed8cc8187a391120ff70e5fcd Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Fri, 10 May 2024 11:16:41 -0500 Subject: [PATCH 25/28] generate_random: fix memleak Signed-off-by: Tony Asleson --- test/tester.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/tester.c b/test/tester.c index 7018390e..6a5aaba5 100644 --- a/test/tester.c +++ b/test/tester.c @@ -119,9 +119,8 @@ void generate_random(char *buff, uint32_t len) { tmp = got; if (__builtin_add_overflow(tmp, cur_got, &got)) { - close(fd); buff[0] = '\0'; - return; + goto out; } } From b385829c19ee3d56fd6328f05bfbf135aa97ff3c Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Fri, 10 May 2024 11:17:52 -0500 Subject: [PATCH 26/28] Remove un-needed null assign Signed-off-by: Tony Asleson --- test/tester.c | 1 - 1 file changed, 1 deletion(-) diff --git a/test/tester.c b/test/tester.c index 6a5aaba5..29d1dc28 100644 --- a/test/tester.c +++ b/test/tester.c @@ -3827,7 +3827,6 @@ START_TEST(test_batteries) { ck_assert_msg(compare_battery(bs[i], b_copy) == 0, "src copy miss-match"); lsm_battery_record_free(b_copy); - b_copy = NULL; id = lsm_battery_id_get(bs[i]); ck_assert_msg(id != NULL && strlen(id) > 0, "NULL"); From 268465a0604651303d2a334a326b3d0e35d0b18b Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Fri, 10 May 2024 11:30:38 -0500 Subject: [PATCH 27/28] scan-scsi-target: Close FD in error path Code scanning tools show this as a FD leak even though in this case it doesn't matter as the utility is exiting soon. Signed-off-by: Tony Asleson --- tools/udev/scan-scsi-target.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/udev/scan-scsi-target.c b/tools/udev/scan-scsi-target.c index cd8cfbc0..430d729f 100644 --- a/tools/udev/scan-scsi-target.c +++ b/tools/udev/scan-scsi-target.c @@ -211,6 +211,7 @@ int main(int argc, char **argv) { usage(argv, 1); } if (write(fd, sysfs_data, strlen(sysfs_data)) < 0) { + close(fd); fprintf(stderr, "Cannot write '%s': %s\n", sysfs_path, strerror(errno)); usage(argv, 1); } From 90aaf7f8ece110adbc5ac50697a531b4bc7c5f98 Mon Sep 17 00:00:00 2001 From: Tony Asleson Date: Fri, 10 May 2024 12:31:19 -0500 Subject: [PATCH 28/28] Remove el7 builds Signed-off-by: Tony Asleson --- .circleci/config.yml | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index bef1aaaf..6301002c 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -19,25 +19,6 @@ jobs: - run: command: cd libstoragemgmt && git checkout $GHBR && ./test/docker_ci_test.sh no_output_timeout: 20m - el7: - docker: - - image: centos:7 - environment: - GHURL: << pipeline.project.git_url >> - GHBR: << pipeline.git.branch >> - steps: - - run: - command: yum install -y git yum-plugin-copr - no_output_timeout: 20m - - run: - command: yum copr enable -y tasleson/ledmon-upstream - no_output_timeout: 20m - - run: - command: git clone $GHURL - no_output_timeout: 5m - - run: - command: cd libstoragemgmt && git checkout $GHBR && ./test/docker_ci_test.sh - no_output_timeout: 20m el8: docker: - image: oraclelinux:8 @@ -63,5 +44,4 @@ workflows: workflow: jobs: - fedora - - el7 - el8