Fix: strdup memory check #523

XV02 · 2025-05-26T20:19:01Z

Summary of changes

This pull request includes following changes or fixes.

Added checks for strdup
Changed logic of delim strategy to avoid repeated code

Description

This change adds checks for memory allocations done by strdup which previously could lead to unexpected Segmentation faults when the malloc failed and returned null.
Also updated the log of one of the uses of strdup to avoid repeated code and stepping up maintainability.

Type of change

Please delete items that are not relevant.

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have confirmed my fix is effective or that my feature works

…loc)

vandelvan

Hey @XV02, thanks for your effort! Great work!

But I'm concerned by the amount of To-dos you added, have you properly tested each check before creating this PR? I am hoping you only forgot to delete those comments because otherwise we will not approve a PR that adds technical debt.

I left a comment in each of those To-dos to keep track of each of them and know which ones have been tested.
Thanks!

vandelvan · 2025-05-26T20:55:57Z

src/libltfs/ltfs_fsops.c

@@ -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.


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

vandelvan · 2025-05-26T20:58:27Z

src/libltfs/ltfslogging.c

@@ -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


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

vandelvan · 2025-05-26T20:59:07Z

src/tape_drivers/freebsd/cam/cam_cmn.c

@@ -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


Same as before

vandelvan · 2025-05-26T20:59:12Z

src/tape_drivers/freebsd/cam/cam_cmn.c

@@ -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


Same as before

vandelvan · 2025-05-26T20:59:18Z

src/tape_drivers/freebsd/cam/cam_cmn.h

@@ -515,6 +515,10 @@ static inline int _sense2errcode(uint32_t sense, struct error_table *table, char
 			rc  = table[i].err_code;
 			if (msg)
 				*msg = strdup(table[i].msg);
+				if (! (*msg)) {
+					ltfsmsg(LTFS_ERR, 10001E, "_sense2errcode: msg assign");
+					// TODO: Check if error return is needed


Same as before

vandelvan · 2025-05-26T21:00:55Z

src/tape_drivers/freebsd/cam/cam_tc.c

@@ -3946,6 +4002,10 @@ int camtape_set_lbp(void *device, bool enable)
 	entry = mt_status_entry_find(&mtinfo, tmpname);
 	if (entry == NULL) {
 		msg = strdup("Cannot find sa(4) protection.protection_supported parameter");
+		if (! (*msg)) {
+			ltfsmsg(LTFS_ERR, 10001E, "camtape_set_lbp: msg assign");
+			// TODO: Check if error return is needed


Same as before

vandelvan · 2025-05-26T21:01:00Z

src/tape_drivers/freebsd/cam/cam_tc.c

@@ -3965,6 +4025,10 @@ int camtape_set_lbp(void *device, bool enable)
 	prot_entry = mt_status_entry_find(&mtinfo, MT_PROTECTION_NAME);
 	if (prot_entry == NULL) {
 		msg = strdup("Cannot find sa(4) protection node!");
+		if (! (*msg)) {
+			ltfsmsg(LTFS_ERR, 10001E, "camtape_set_lbp: msg assign");
+			// TODO: Check if error return is needed


Same as before

vandelvan · 2025-05-26T21:01:05Z

src/tape_drivers/freebsd/cam/cam_tc.c

@@ -3998,6 +4062,10 @@ int camtape_set_lbp(void *device, bool enable)
 		entry = mt_entry_find(prot_entry, __DECONST(char *, protect_list[i].name));
 		if (entry == NULL) {
 			msg = strdup("Cannot find all protection information entries");
+			if (! (*msg)) {
+				ltfsmsg(LTFS_ERR, 10001E, "camtape_set_lbp: msg assign");
+				// TODO: Check if error return is needed


Same as before

vandelvan · 2025-05-26T21:01:10Z

src/tape_drivers/freebsd/cam/cam_tc.c

@@ -4020,6 +4088,10 @@ int camtape_set_lbp(void *device, bool enable)
 		snprintf(tmpstr, sizeof(tmpstr), "Error returned from MTIOCSETLIST ioctl to set "
 			"protection parameters: %s", strerror(errno));
 		msg = strdup(tmpstr);
+		if (! (*msg)) {
+			ltfsmsg(LTFS_ERR, 10001E, "camtape_set_lbp: msg assign");
+			// TODO: Check if error return is needed


Same as before

vandelvan · 2025-05-26T21:01:47Z

src/utils/ltfsck.c

@@ -294,10 +294,15 @@ int main(int argc, char **argv)
 	while (true) {
 		int option_index = 0;
 		int c = getopt_long(argc, argv, short_options, long_options, &option_index);
+		// TODO: Check for optarg to not be null before using it


Same as before

Working on it for all of them, thanks.

XV02 · 2025-05-26T21:27:56Z

Hi @vandelvan thanks for the reminder, I need to remove the ones checked already, only a couple where left for review, I will update it and come back with the full code checked and ready.

piste-jp

Please consider how to handle memory allocation error correctly.

The important thing is not detecting the memory allocation error, how to handle it.

Memory allocation error might be one shot ? or permanent ?
LTFS can keep routing the tape after memory error ?
No data corruption after memory allocation error handling ?
No memory leak after memory allocation error ?
No cleaning resources after memory allocation error ?

src/libltfs/arch/filename_handling.c

src/libltfs/fs.c

src/libltfs/index_criteria.c

piste-jp · 2025-05-27T01:18:00Z

src/tape_drivers/freebsd/cam/cam_cmn.c

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

Working on it.

piste-jp · 2025-05-27T01:19:37Z

src/tape_drivers/freebsd/cam/cam_cmn.h

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

Working on it.

piste-jp · 2025-05-27T01:20:03Z

src/tape_drivers/freebsd/cam/cam_tc.c

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

Working on it.

src/tape_drivers/generic/file/filedebug_tc.c

src/utils/mkltfs.c

- Also removed now unused code

piste-jp

It looks a few critical updates are included. I cannot accept this PR.

piste-jp · 2025-07-09T14:19:40Z

src/libltfs/index_criteria.c

 		}
+		else if (*delim == '/') {


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.

Will check them.

piste-jp · 2025-07-09T14:21:30Z

src/libltfs/ltfs.c

@@ -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);


Why this line is changed?

It looks you ruined the fix made before.

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

piste-jp · 2025-07-09T14:22:42Z

src/libltfs/ltfs.c

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


Why this part is changed?

It looks you ruined the fix made before.

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.

XV02 added 4 commits April 18, 2025 10:31

feat: Added checkups for correct memory allocation on all strdup (mal…

7a3d4ee

…loc)

feat: Return to old delim logic

1908efa

update: Changed to new delim strategy

46b5724

docs: Updated copyright years of modified files

7ea045c

XV02 requested review from amissael95, piste-jp, juliocelon, vandelvan and chukero May 26, 2025 20:19

XV02 self-assigned this May 26, 2025

XV02 requested review from piste-jp and removed request for piste-jp May 26, 2025 20:27

vandelvan requested changes May 26, 2025

View reviewed changes

piste-jp requested changes May 27, 2025

View reviewed changes

XV02 added 11 commits June 25, 2025 16:54

chore: Updated comment style to C++

0bec91b

fix: Filename Handling, ret value fixed

5bc15ee

fix: Change camel case for snake case

7348d2f

- Also removed now unused code

fix: Correct return error and minor identation fix

a2d4a0a

fix: Returned comment deleted by mistake

2ed7743

fix: Returned code deleted by mistake

e94c944

fix: Use correct convention for error

83ea0f2

fix: Use correct convention for error

3c37ee0

fix: Use correct convention for error

f0ae574

fix: Malloc memory check

b95a9c5

fix: Correct error message

559fa1e

piste-jp requested changes Jul 9, 2025

View reviewed changes

Fix: strdup memory check #523

Are you sure you want to change the base?

Fix: strdup memory check #523

Uh oh!

Conversation

XV02 commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Description

Type of change

Checklist:

Uh oh!

vandelvan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XV02 commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piste-jp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

piste-jp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

XV02 commented May 26, 2025 •

edited

Loading

XV02 commented May 26, 2025 •

edited

Loading