From 828ff42848cb06cf0a9c94fcfd1ff47fd5617b22 Mon Sep 17 00:00:00 2001 From: Matej Kenda Date: Fri, 12 Jun 2026 12:51:57 +0200 Subject: [PATCH 1/2] fix: reads of uninitialized values on error paths Four sites used a value that may never have been set: - tape_get_pews leaves *pews unset on -LTFS_UNSUPPORTED, which the caller treats as non-fatal before computing pews + 10. - ltfs_profiler_set left ret unset when the volume had neither an iosched nor a device handle, then tested it. - _get_dump derived a transfer length from cap_buf without checking the READ BUFFER result, so a failed read produced a garbage length. - the ICU normalize helpers leave their output pointer unset on error, but the callers compared that pointer against the input (to decide whether to free a no-op result) before checking the return code, reading uninitialized memory and potentially leaking the input buffer. Initialize pews and ret, check the READ BUFFER result, and check the normalize return code before the pointer comparison in all five callers. --- src/libltfs/ltfs.c | 4 +++- src/libltfs/pathname.c | 34 ++++++++++++++++++++--------- src/libltfs/tape.c | 5 ++++- src/tape_drivers/linux/sg/sg_tape.c | 8 ++++++- 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/libltfs/ltfs.c b/src/libltfs/ltfs.c index c0c624b3..6bd807f7 100644 --- a/src/libltfs/ltfs.c +++ b/src/libltfs/ltfs.c @@ -4404,7 +4404,9 @@ void ltfs_enable_livelink_mode(struct ltfs_volume *vol) */ int ltfs_profiler_set(uint64_t source, struct ltfs_volume *vol) { - int ret, ret_save = 0; + /* ret stays unset if neither the iosched nor device handle is present; + * the final "if (!ret && ret_save)" would then read garbage. */ + int ret = 0, ret_save = 0; if (vol->iosched_handle) { if (source & PROF_IOSCHED) { diff --git a/src/libltfs/pathname.c b/src/libltfs/pathname.c index 19abc321..6eba0b1a 100644 --- a/src/libltfs/pathname.c +++ b/src/libltfs/pathname.c @@ -197,10 +197,14 @@ int pathname_prepare_caseless(const char *name, UChar **new_name, bool use_nfc) /* Convert to NFD if needed, then case fold the name. */ if (need_initial_nfd) { ret = _pathname_normalize_nfd_icu(icu_name, &icu_nfd); - if (icu_name != icu_nfd) + if (ret < 0) { + /* icu_nfd is unset on error; free the input and bail before + * comparing the (garbage) output pointer. */ free(icu_name); - if (ret < 0) return ret; + } + if (icu_name != icu_nfd) + free(icu_name); ret = _pathname_foldcase_icu(icu_nfd, &icu_fold); free(icu_nfd); if (ret < 0) @@ -217,10 +221,14 @@ int pathname_prepare_caseless(const char *name, UChar **new_name, bool use_nfc) ret = _pathname_normalize_nfc_icu(icu_fold, new_name); else ret = _pathname_normalize_nfd_icu(icu_fold, new_name); - if (icu_fold != *new_name) + if (ret < 0) { + /* *new_name is unset on error; free the input and bail before + * comparing the (garbage) output pointer. */ free(icu_fold); - if (ret < 0) return ret; + } + if (icu_fold != *new_name) + free(icu_fold); return 0; } @@ -475,10 +483,12 @@ int _pathname_format_icu(const char *src, char **dest, bool validate, bool allow /* normalize */ ret = _pathname_normalize_nfc_icu(utf16_name, &utf16_name_norm); - if (utf16_name != utf16_name_norm) + if (ret < 0) { free(utf16_name); - if (ret < 0) return ret; + } + if (utf16_name != utf16_name_norm) + free(utf16_name); /* convert to UTF-8 */ ret = _pathname_utf16_to_utf8_icu(utf16_name_norm, dest); @@ -534,10 +544,12 @@ int _pathname_normalize_utf8_nfd_icu(const char *src, char **dest) return ret; ret = _pathname_normalize_nfd_icu(icu_str, &icu_str_norm); - if (icu_str != icu_str_norm) + if (ret < 0) { free(icu_str); - if (ret < 0) return ret; + } + if (icu_str != icu_str_norm) + free(icu_str); ret = _pathname_utf16_to_utf8_icu(icu_str_norm, dest); free(icu_str_norm); @@ -602,10 +614,12 @@ int _pathname_normalize_utf8_icu(const char *src, char **dest) return ret; ret = _pathname_normalize_nfc_icu(icu_str, &icu_str_norm); - if (icu_str != icu_str_norm) + if (ret < 0) { free(icu_str); - if (ret < 0) return ret; + } + if (icu_str != icu_str_norm) + free(icu_str); ret = _pathname_utf16_to_utf8_icu(icu_str_norm, dest); free(icu_str_norm); diff --git a/src/libltfs/tape.c b/src/libltfs/tape.c index 255ca2a2..58c5bfe4 100644 --- a/src/libltfs/tape.c +++ b/src/libltfs/tape.c @@ -373,7 +373,10 @@ int tape_load_tape(struct device_data *dev, void * const kmi_handle, bool force) int ret; struct tc_drive_param param; struct tc_remaining_cap cap; - uint16_t pews; + /* tape_get_pews leaves this unset when it returns -LTFS_UNSUPPORTED, + * which the caller treats as non-fatal and then reads pews; default to 0 + * so the fallback (pews + 10) is deterministic. */ + uint16_t pews = 0; CHECK_ARG_NULL(dev, -LTFS_NULL_ARG); CHECK_ARG_NULL(dev->backend, -LTFS_NULL_ARG); diff --git a/src/tape_drivers/linux/sg/sg_tape.c b/src/tape_drivers/linux/sg/sg_tape.c index 19f2beb8..96c6ccd1 100644 --- a/src/tape_drivers/linux/sg/sg_tape.c +++ b/src/tape_drivers/linux/sg/sg_tape.c @@ -311,7 +311,13 @@ static int _get_dump(struct sg_data *priv, char *fname) } /* Get buffer capacity */ - _cdb_read_buffer(priv, buf_id, cap_buf, 0, sizeof(cap_buf), 0x03); + ret = _cdb_read_buffer(priv, buf_id, cap_buf, 0, sizeof(cap_buf), 0x03); + if (ret < 0) { + /* cap_buf is indeterminate on failure; do not derive a transfer + * length from it. */ + free(dump_buf); + return ret; + } data_length = (cap_buf[1] << 16) + (cap_buf[2] << 8) + (int)cap_buf[3]; /* Open dump file for write only*/ From 418c89828b4cc03908bb30483f31cb361ee8ee05 Mon Sep 17 00:00:00 2001 From: Matej Kenda Date: Thu, 11 Jun 2026 00:18:18 +0200 Subject: [PATCH 2/2] fix: leak of the key list on realloc failure in the simple KMI realloc was assigned back to priv.dk_list, so a failure overwrote the only pointer to the existing buffer with NULL and leaked it. Use a temporary and free the original on failure. --- src/kmi/simple.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/kmi/simple.c b/src/kmi/simple.c index 41c83cf9..9aa90751 100644 --- a/src/kmi/simple.c +++ b/src/kmi/simple.c @@ -192,13 +192,20 @@ int simple_parse_opts(void *opt_args) const size_t dk_list_len = (priv.dk_list ? strlen((char *) priv.dk_list) + strlen("/") : 0) + strlen((char *) key[i].dk) + strlen(":") + strlen((char *) key[i].dki) + 1; - if (priv.dk_list) - priv.dk_list = (unsigned char*)realloc(priv.dk_list, dk_list_len); - else + if (priv.dk_list) { + unsigned char *expanded = (unsigned char*)realloc(priv.dk_list, dk_list_len); + if (expanded == NULL) { + ltfsmsg(LTFS_ERR, 10001E, __FUNCTION__); + free(priv.dk_list); + return -LTFS_NO_MEMORY; + } + priv.dk_list = expanded; + } else { priv.dk_list = (unsigned char*)calloc(dk_list_len, sizeof(unsigned char)); - if (priv.dk_list == NULL) { - ltfsmsg(LTFS_ERR, 10001E, __FUNCTION__); - return -LTFS_NO_MEMORY; + if (priv.dk_list == NULL) { + ltfsmsg(LTFS_ERR, 10001E, __FUNCTION__); + return -LTFS_NO_MEMORY; + } } *(priv.dk_list + original_dk_list_len) = '\0';