Skip to content

Memory-safety fixes: uninitialized reads and a realloc leak #613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
matejk wants to merge 2 commits into
base: main
Choose a base branch
from
+51 −19
Open

Memory-safety fixes: uninitialized reads and a realloc leak #613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
828ff42
fix: reads of uninitialized values on error paths
matejk Jun 12, 2026
418c898
fix: leak of the key list on realloc failure in the simple KMI
matejk Jun 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions src/kmi/simple.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment on lines +195 to +208

@XV02 XV02 Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to add more unnecessary complexity and repeated code. Maybe adding the brackets for clarity can be a good change. The added free on the original error path should be enough.

}
*(priv.dk_list + original_dk_list_len) = '\0';

Expand Down
4 changes: 3 additions & 1 deletion src/libltfs/ltfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Comment on lines +4407 to +4408

@XV02 XV02 Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is out of scope for documentation, there's no need to over explain the initialization of the variable. Was this AI generated?

int ret = 0, ret_save = 0;

if (vol->iosched_handle) {
if (source & PROF_IOSCHED) {
Expand Down
34 changes: 24 additions & 10 deletions src/libltfs/pathname.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Comment on lines +201 to +202

@XV02 XV02 Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto about the comment.

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)
Expand All @@ -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. */
Comment on lines +225 to +226

@XV02 XV02 Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto about the comment.

free(icu_fold);
if (ret < 0)
return ret;
}
if (icu_fold != *new_name)
free(icu_fold);

return 0;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
5 changes: 4 additions & 1 deletion src/libltfs/tape.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Comment on lines +376 to +378

@XV02 XV02 Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto about the comment.

uint16_t pews = 0;

CHECK_ARG_NULL(dev, -LTFS_NULL_ARG);
CHECK_ARG_NULL(dev->backend, -LTFS_NULL_ARG);
Expand Down
8 changes: 7 additions & 1 deletion src/tape_drivers/linux/sg/sg_tape.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Comment on lines +316 to +317

@XV02 XV02 Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto about the comment.

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*/
Expand Down