Skip to content

Fix: strdup memory check #523

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
wants to merge 15 commits into
base: v2.4-stable
Choose a base branch
from
Open

Fix: strdup memory check #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7a3d4ee
feat: Added checkups for correct memory allocation on all strdup (mal…
XV02 Apr 18, 2025
1908efa
feat: Return to old delim logic
XV02 Apr 18, 2025
46b5724
update: Changed to new delim strategy
XV02 Apr 18, 2025
7ea045c
docs: Updated copyright years of modified files
XV02 May 22, 2025
0bec91b
chore: Updated comment style to C++
XV02 Jun 25, 2025
5bc15ee
fix: Filename Handling, ret value fixed
XV02 Jun 25, 2025
7348d2f
fix: Change camel case for snake case
XV02 Jun 25, 2025
a2d4a0a
fix: Correct return error and minor identation fix
XV02 Jun 26, 2025
2ed7743
fix: Returned comment deleted by mistake
XV02 Jun 26, 2025
e94c944
fix: Returned code deleted by mistake
XV02 Jun 26, 2025
83ea0f2
fix: Use correct convention for error
XV02 Jun 26, 2025
3c37ee0
fix: Use correct convention for error
XV02 Jun 26, 2025
f0ae574
fix: Use correct convention for error
XV02 Jun 26, 2025
b95a9c5
fix: Malloc memory check
XV02 Jun 26, 2025
559fa1e
fix: Correct error message
XV02 Jun 26, 2025
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
10 changes: 8 additions & 2 deletions src/libltfs/arch/filename_handling.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -130,6 +130,7 @@ void update_platform_safe_name(struct dentry* dentry, bool handle_invalid_char,
}
}
#else
/* Checkup not needed here, memory error handling happens after function call. */
dentry->platform_safe_name = strdup(dentry->name.name);
#endif
}
Expand Down Expand Up @@ -242,7 +243,12 @@ char * _generate_target_file_name(const char *prefix, const char *extension, int
ret = asprintf(&target, "%s.%s", prefix, extension);
else {
target = strdup(prefix);
ret = target ? strlen(target) : -1;
if (!target) {
ltfsmsg(LTFS_ERR, 10001E, "_generate_target_file_name: target assign");
ret = -1;
} else {
ret = strlen(target);
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/libltfs/fs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -89,6 +89,7 @@ static char* generate_hash_key_name(const char *src_str, int *rc)
} else
free(uchar_name);
#else
/* Checkup not needed here, memory error handling happens after function call. */
key_name = strdup(src_str);
*rc = 0;
#endif
Expand Down
41 changes: 26 additions & 15 deletions src/libltfs/index_criteria.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -288,27 +288,38 @@ int index_criteria_parse_name(const char *criteria, size_t len, struct index_cri
/* Assign rules to the glob_patterns[] array */
rule = rule+5;
for (delim = rule; *delim; delim++) {
bool do_delim_assign = false;
bool do_rule_add = false;
if (*delim == ':') {
*delim = '\0';
rule_ptr->percent_encode = fs_is_percent_encode_required(rule);
rule_ptr->name = strdup(rule);
rule_ptr++;
rule = delim+1;
} else if (*delim == '/') {
*delim = '\0';
rule_ptr->percent_encode = fs_is_percent_encode_required(rule);
rule_ptr->name = strdup(rule);
rule_ptr++;
} else if (*(delim+1) == '\0') {
rule_ptr->percent_encode = fs_is_percent_encode_required(rule);
rule_ptr->name = strdup(rule);
rule_ptr++;
do_delim_assign = true;
do_rule_add = true;
}
else if (*delim == '/') {
Copy link
Member

Choose a reason for hiding this comment

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

Please follow the convention. You must not break a line before else.

It looks there are multiple points that is same style. Please correct all of them.

Copy link
Author

Choose a reason for hiding this comment

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

Will check them.

do_delim_assign = true;
}
else if (! (*(delim+1) == '\0')) {
continue;
}
if (do_delim_assign) *delim = '\0';
rule_ptr->percent_encode = fs_is_percent_encode_required(rule);
rule_ptr->name = strdup(rule);
if (! rule_ptr->name) {
ltfsmsg(LTFS_ERR, 10001E, "index_criteria_parse_name: rule assign");
free(rule_ptr->name);
return -LTFS_NO_MEMORY;
}
rule_ptr++;
if (do_rule_add) rule = delim+1;
}

if (ic->glob_patterns == rule_ptr) {
rule_ptr->percent_encode = fs_is_percent_encode_required(rule);
rule_ptr->name = strdup(rule);
if (! rule_ptr->name) {
ltfsmsg(LTFS_ERR, 10001E, "index_criteria_parse_name: glob_patterns assign");
free(rule_ptr->name);
return -EDEV_NO_MEMORY;
}
}

/* Validate rules */
Expand Down
14 changes: 4 additions & 10 deletions src/libltfs/ltfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1659,7 +1659,7 @@ int ltfs_mount(bool force_full, bool deep_recovery, bool recover_extra, bool rec
(unsigned long long)volume_change_ref);

/* Index of IP could be corrupted. So set skip flag to true */
ret = _ltfs_search_index_wp(recover_symlink, true, &seekpos, vol);
ret = _ltfs_search_index_wp(recover_symlink, false, &seekpos, vol);
Copy link
Member

Choose a reason for hiding this comment

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

Why this line is changed?

It looks you ruined the fix made before.

Copy link
Author

Choose a reason for hiding this comment

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

It looks a change from somewhere else got into the PR, fixing it, thanks for the catch.

if (ret < 0)
goto out_unlock;

Expand All @@ -1669,8 +1669,7 @@ int ltfs_mount(bool force_full, bool deep_recovery, bool recover_extra, bool rec
seekpos.block = vol->dp_coh.set_id;
}
} else {
if (vol->ip_coh.count > vol->dp_coh.count && vollock != PWE_MAM_DP && vollock != PWE_MAM) {
/*
if (vol->ip_coh.count > vol->dp_coh.count && vollock != PWE_MAM_DP && vollock != PWE_MAM) { /*
* The index on IP is newer but MAM shows write perm doesn't happen in DP.
* LTFS already have written an index on DP when it is writing an index on IP,
* so this condition wouldn't happen logically.
Expand All @@ -1688,13 +1687,8 @@ int ltfs_mount(bool force_full, bool deep_recovery, bool recover_extra, bool rec
(unsigned long long)vol->dp_coh.volume_change_ref,
(unsigned long long)volume_change_ref);

if (vollock == PWE_MAM_BOTH) {
/* Index of IP could be corrupted (because of double write perm). So set skip flag to true */
ret = _ltfs_search_index_wp(recover_symlink, true, &seekpos, vol);
} else {
/* Index of DP could be corrupted. So set skip flag to false */
ret = _ltfs_search_index_wp(recover_symlink, false, &seekpos, vol);
}
/* Index of IP could be corrupted. So set skip flag */
Copy link
Member

Choose a reason for hiding this comment

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

Why this part is changed?

It looks you ruined the fix made before.

Copy link
Author

Choose a reason for hiding this comment

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

It looks a change from somewhere else got into the PR, fixing it, thanks for the catch.
@piste-jp Can you also respond my other comments? Thanks, appreciate it.

ret = _ltfs_search_index_wp(recover_symlink, true, &seekpos, vol);
if (ret < 0)
goto out_unlock;

Expand Down
7 changes: 6 additions & 1 deletion src/libltfs/ltfs_fsops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2021 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -1912,7 +1912,12 @@ int ltfs_fsops_symlink_path(const char* to, const char* from, ltfs_file_id *id,

id->uid = d->uid;
id->ino = d->ino;
// TODO: Check if the return error is needed or can be skipped.
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid adding technical debt hehe, did you just forget to erase this TODO? Or you did not manage to check it?

Copy link
Member

Choose a reason for hiding this comment

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

Please use C++ style command only for comment out.
Use C style comment here /* */ if this comment is really needed.

Copy link
Author

Choose a reason for hiding this comment

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

I will do the testing so I can completely remove them, thanks.

d->target.name = strdup(to);
if (! d->target.name) {
ltfsmsg(LTFS_ERR, 10001E, "ltfs_fsops_symlink_path: d target name");
Copy link
Member

Choose a reason for hiding this comment

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

You need to remove the node created by tfs_fsops_create() in line 1909 at least.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @piste-jp using the following code is enough to remove the node, or do I need to do something else? Thanks

ret2 = ltfs_fsops_close(d, true, true, use_iosche, vol);
if ( ret==0 && ret2<0 ) {
	ret = ret2;
}

return -LTFS_NO_MEMORY;
}
d->target.percent_encode = fs_is_percent_encode_required(to);
d->isslink = true;

Expand Down
6 changes: 5 additions & 1 deletion src/libltfs/ltfs_internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -1313,6 +1313,10 @@ int ltfs_split_symlink( struct ltfs_volume *vol )
}
ret = ltfs_fsops_close( workd, true, true, use_iosche, vol);
path=strdup(lfdir);
if (! path) {
ltfsmsg(LTFS_ERR, 10001E, "ltfs_split_symlink: path assign");
Copy link
Member

Choose a reason for hiding this comment

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

You need to remove the node created by tfs_fsops_create() in line 1303 at least.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @piste-jp the node is closed just above those lines, in 1314, is something else needed? Thanks a lot

return -LTFS_NO_MEMORY;
}

/* loop for conflicted files */
for( i=0; i<(vol->index->symerr_count); i++ ){
Expand Down
7 changes: 6 additions & 1 deletion src/libltfs/ltfslogging.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -489,6 +489,11 @@ int ltfsmsg_internal(bool print_id, int level, char **msg_out, const char *_id,
vsprintf(msg_buf, output_buf, argp);
va_end(argp);
*msg_out = strdup(msg_buf);
// TODO: Check if the return -1 is needed, or the program can just continue
Copy link
Member

Choose a reason for hiding this comment

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

Same as before, please do not add more technical debt, I cannot approve a PR that has not been properly tested 😅

if (!(*msg_out)) {
ltfsmsg(LTFS_ERR, 10001E, "ltfsmsg_internal: msg_out assign");
Copy link
Member

Choose a reason for hiding this comment

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

It is a terrible idea to use ltfsmsg within this function because this function is used from ltfsmsg.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @piste-jp your insight would be really helpful here, it is crucial to stop the process and return an error in this section (For example using the goto internal_error) or just printing a message using the fprintf or syslog functions? Thanks

return -1;
}
}

#ifdef ENABLE_SNMP
Expand Down
6 changes: 5 additions & 1 deletion src/libltfs/ltfssnmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -122,6 +122,10 @@ int read_trap_def_file(char *deffile)
return -LTFS_NO_MEMORY;
}
entry->id = strdup(tok);
if (!entry->id) {
ltfsmsg(LTFS_ERR, 10001E, "read_trap_def_file: entry id assign");
return -LTFS_NO_MEMORY;
}
TAILQ_INSERT_TAIL(&trap_entries, entry, list);
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/libltfs/pathname.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -953,8 +953,10 @@ int _pathname_utf8_to_system_icu(const char *src, char **dest)
syslocale = ucnv_getDefaultName();
if (! strcmp(syslocale, "UTF-8")) {
*dest = strdup(src);
if (! *dest)
if (! (*dest)) {
ltfsmsg(LTFS_ERR, 10001E, __FUNCTION__);
return -LTFS_NO_MEMORY;
}
return 0;
}

Expand Down
8 changes: 6 additions & 2 deletions src/libltfs/tape.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -1895,6 +1895,10 @@ int tape_get_media_pool_info(struct ltfs_volume *vol, char **media_name, char **
name = strndup(vol->t_attr->media_pool, add_start);
}
info = strdup(&(vol->t_attr->media_pool[add_start+1]));
if (!info) {
ltfsmsg(LTFS_ERR, 10001E, __FUNCTION__);
return -LTFS_NO_MEMORY;
}
len = strlen(info);
info[len-1] = '\0';
}
Expand Down Expand Up @@ -3508,7 +3512,7 @@ int read_tape_attribute(struct ltfs_volume *vol, char **val, const char *name)
*val = strdup(vol->t_attr->media_pool);
}

if (!*val) {
if (!(*val)) {
ltfsmsg(LTFS_ERR, 10001E, "read_tape_attribute: *val");
return -LTFS_UNEXPECTED_VALUE;
}
Expand Down
6 changes: 5 additions & 1 deletion src/libltfs/xml_reader_libltfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -154,6 +154,10 @@ static int decode_entry_name(char **new_name, const char *name)
tmp_name[j] = '\0';

*new_name = strdup(tmp_name);
if (! (*new_name)) {
ltfsmsg(LTFS_ERR, 10001E, __FUNCTION__);
return -LTFS_NO_MEMORY;
}
free(tmp_name);

return 0;
Expand Down
10 changes: 9 additions & 1 deletion src/libltfs/xml_writer_libltfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OO_Copyright_BEGIN
**
**
** Copyright 2010, 2020 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
** modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -98,6 +98,10 @@ static int encode_entry_name(char **new_name, const char *name)
len = strlen(name);

tmp_name = malloc(len * 3 * sizeof(UChar));
if (! tmp_name) {
ltfsmsg(LTFS_ERR, 10001E, "encode_entry_name: tmp name assign");
return -LTFS_NO_MEMORY;
}
buf_encode[2] = '\0';

while (i < len) {
Expand Down Expand Up @@ -131,6 +135,10 @@ static int encode_entry_name(char **new_name, const char *name)
tmp_name[j] = '\0';

*new_name = strdup(tmp_name);
if (! (*new_name)) {
ltfsmsg(LTFS_ERR, 10001E, "encode_entry_name: new name assign");
return -LTFS_NO_MEMORY;
}
free(tmp_name);

return 0;
Expand Down
10 changes: 9 additions & 1 deletion src/tape_drivers/freebsd/cam/cam_cmn.c
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Did you really test this code? This code is only compiled under FreeBSD.

Copy link
Author

Choose a reason for hiding this comment

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

Working on it.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
** OS_Copyright_BEGIN
**
**
** Copyright 2010, 2018 IBM Corp. All rights reserved.
** Copyright 2010, 2025 IBM Corp. All rights reserved.
** Copyright (c) 2013-2018 Spectra Logic Corporation. All rights reserved.
**
** Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -189,6 +189,10 @@ int camtape_ioctlrc2err(void *device, int fd, struct scsi_sense_data *sense_data

if (msg) {
*msg = strdup("No Sense Information");
if (! (*msg)) {
ltfsmsg(LTFS_ERR, 10001E, "camtape_ioctlrc2err: msg assign");
// TODO: Check if error return is needed
Copy link
Member

Choose a reason for hiding this comment

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

Same as before

}
}
rc = -EDEV_NO_SENSE;
} else {
Expand Down Expand Up @@ -217,6 +221,10 @@ int camtape_ioctlrc2err(void *device, int fd, struct scsi_sense_data *sense_data
ltfsmsg(LTFS_INFO, 31212I, rc_sense);
if (msg)
*msg = strdup("Cannot get sense information");
if (! (*msg)) {
ltfsmsg(LTFS_ERR, 10001E, "camtape_ioctlrc2err: msg assign");
// TODO: Check if error return is needed
Copy link
Member

Choose a reason for hiding this comment

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

Same as before

}

rc = -EDEV_CANNOT_GET_SENSE;
}
Expand Down
Loading
Loading