[PATCH v1 0/8] fio error handling fixes

[PATCH v1 0/8] fio error handling fixes

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Hi Jens,

This series contains patches that were dropped from my previous thread:

  https://lore.kernel.org/fio/20220428203954.222165-1-ammarfaizi2@gnuweeb.org

Plus, I have more patches in this series. This series contains error
handling fixes, mostly about ENOMEM. There are 8 patches in this
series. All of them are ENOMEM handling stuff except that patch #4 has
extra fixes and a bit of refactoring.

Please review, thanks!

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (8):
  cgroup: Add ENOMEM handling on a `malloc()` call
  stat: Add ENOMEM handling on `malloc()` / `calloc()` calls
  engines/net: Add ENOMEM handling on a `malloc()` call
  blktrace: Fix broken error handling in `merge_blktrace_iologs()`
  blktrace: Add ENOMEM handling when allocating @ipo
  blktrace: Add ENOMEM handling in `trace_add_open_close_event()` and its callers
  client: Add ENOMEM handling on `realloc()` calls
  client: Add ENOMEM handling on `malloc()`, `calloc()` and `strdup()` calls

 blktrace.c    | 116 +++++++++++++++++++++++++++++++-------------
 cgroup.c      |   4 ++
 client.c      | 131 +++++++++++++++++++++++++++++++++++++++++---------
 engines/net.c |   9 ++--
 stat.c        |  40 +++++++++++----
 5 files changed, 231 insertions(+), 69 deletions(-)


base-commit: 5f2d43188c2d65674aaba6280e2a87107e5d7099
-- 
Ammar Faizi

[PATCH v1 1/8] cgroup: Add ENOMEM handling on a `malloc()` call

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Avoid a NULL pointer dereference bug when `ENOMEM`.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 cgroup.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cgroup.c b/cgroup.c
index 77e31a4d..b559b70f 100644
--- a/cgroup.c
+++ b/cgroup.c
@@ -114,6 +114,8 @@ void cgroup_kill(struct flist_head *clist)
 static char *get_cgroup_root(struct thread_data *td, struct cgroup_mnt *mnt)
 {
 	char *str = malloc(64);
+	if (!str)
+		return NULL;
 
 	if (td->o.cgroup)
 		sprintf(str, "%s/%s", mnt->path, td->o.cgroup);
@@ -178,6 +180,8 @@ int cgroup_setup(struct thread_data *td, struct flist_head *clist, struct cgroup
 	 * Create container, if it doesn't exist
 	 */
 	root = get_cgroup_root(td, *mnt);
+	if (!root)
+		return 1;
 	if (mkdir(root, 0755) < 0) {
 		int __e = errno;
 
-- 
Ammar Faizi

[PATCH v1 2/8] stat: Add ENOMEM handling on `malloc()` / `calloc()` calls

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Avoid a NULL pointer dereference bug when `ENOMEM`.

This adds missing `ENOMEM` handling in these function:
  - __show_run_stats()
  - __show_running_run_stats()
  - add_clat_sample()
  - calc_block_percentiles()

While in there, extra changes in `__show_run_stats()`:
  - Replace `malloc()` + set NULL with `calloc()` for simplicity.
  - Call `free()` properly when allocation fails to avoid memory leak.
  - Use `sizeof(*var)` instead of `sizeof(struct x)` for simplicity.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 stat.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/stat.c b/stat.c
index 949af5ed..29b58606 100644
--- a/stat.c
+++ b/stat.c
@@ -858,6 +858,8 @@ static int calc_block_percentiles(int nr_block_infos, uint32_t *block_infos,
 		return 0;
 
 	*percentiles = calloc(len, sizeof(**percentiles));
+	if (!*percentiles)
+		return 0;
 
 	for (i = 0; i < len; i++) {
 		int idx = (plist[i].u.f * (nr_block_infos - nr_uninit) / 100)
@@ -2429,7 +2431,11 @@ void __show_run_stats(void)
 	struct buf_output output[FIO_OUTPUT_NR];
 	struct flist_head **opt_lists;
 
-	runstats = malloc(sizeof(struct group_run_stats) * (groupid + 1));
+	runstats = malloc((groupid + 1) * sizeof(*runstats));
+	if (!runstats) {
+		log_err("fio: failed to allocate runstats\n");
+		return;
+	}
 
 	for (i = 0; i < groupid + 1; i++)
 		init_group_run_stat(&runstats[i]);
@@ -2454,13 +2460,20 @@ void __show_run_stats(void)
 		nr_ts++;
 	}
 
-	threadstats = malloc(nr_ts * sizeof(struct thread_stat));
-	opt_lists = malloc(nr_ts * sizeof(struct flist_head *));
+	threadstats = malloc(nr_ts * sizeof(*threadstats));
+	if (!threadstats) {
+		log_err("fio: failed to allocate threadstats\n");
+		goto out_free_runstats;
+	}
+
+	opt_lists = calloc(nr_ts, sizeof(*opt_lists));
+	if (!opt_lists) {
+		log_err("fio: failed to allocate opt_lists\n");
+		goto out_free_threadstats;
+	}
 
-	for (i = 0; i < nr_ts; i++) {
+	for (i = 0; i < nr_ts; i++)
 		init_thread_stat(&threadstats[i]);
-		opt_lists[i] = NULL;
-	}
 
 	init_per_prio_stats(threadstats, nr_ts);
 
@@ -2709,15 +2722,18 @@ void __show_run_stats(void)
 	fio_idle_prof_cleanup();
 
 	log_info_flush();
-	free(runstats);
 
 	/* free arrays allocated by sum_thread_stats(), if any */
 	for (i = 0; i < nr_ts; i++) {
 		ts = &threadstats[i];
 		free_clat_prio_stats(ts);
 	}
-	free(threadstats);
+
 	free(opt_lists);
+out_free_threadstats:
+	free(threadstats);
+out_free_runstats:
+	free(runstats);
 }
 
 int __show_running_run_stats(void)
@@ -2729,7 +2745,10 @@ int __show_running_run_stats(void)
 
 	fio_sem_down(stat_sem);
 
-	rt = malloc(thread_number * sizeof(unsigned long long));
+	rt = malloc(thread_number * sizeof(*rt));
+	if (!rt)
+		return 1;
+
 	fio_gettime(&ts, NULL);
 
 	for_each_td(td, i) {
@@ -3331,6 +3350,8 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
 			 */
 			io_u_plat = (uint64_t *) td->ts.io_u_plat[FIO_CLAT][ddir];
 			dst = malloc(sizeof(struct io_u_plat_entry));
+			if (!dst)
+				goto out;
 			memcpy(&(dst->io_u_plat), io_u_plat,
 				FIO_IO_U_PLAT_NR * sizeof(uint64_t));
 			flist_add(&dst->list, &hw->list);
@@ -3347,6 +3368,7 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
 		}
 	}
 
+out:
 	if (needs_lock)
 		__td_io_u_unlock(td);
 }
-- 
Ammar Faizi

[PATCH v1 3/8] engines/net: Add ENOMEM handling on a `malloc()` call

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Replace `malloc()` + `memset()` with `calloc()` to simplify the call.
`calloc()` zeroes the allocated memory, so we can avoid `memset()`.
Also, handle the `ENOMEM` case.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 engines/net.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/engines/net.c b/engines/net.c
index c6cec584..24c1463d 100644
--- a/engines/net.c
+++ b/engines/net.c
@@ -1370,9 +1370,9 @@ static int fio_netio_setup(struct thread_data *td)
 	}
 
 	if (!td->io_ops_data) {
-		nd = malloc(sizeof(*nd));
-
-		memset(nd, 0, sizeof(*nd));
+		nd = calloc(1, sizeof(*nd));
+		if (!nd)
+			return 1;
 		nd->listenfd = -1;
 		nd->pipes[0] = nd->pipes[1] = -1;
 		td->io_ops_data = nd;
@@ -1391,7 +1391,8 @@ static int fio_netio_setup_splice(struct thread_data *td)
 {
 	struct netio_data *nd;
 
-	fio_netio_setup(td);
+	if (fio_netio_setup(td))
+		return 1;
 
 	nd = td->io_ops_data;
 	if (nd) {
-- 
Ammar Faizi

[PATCH v1 4/8] blktrace: Fix broken error handling in `merge_blktrace_iologs()`

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

There are several problems in this functions:

1) The error is handled inconsistently. (e.g. we have `ret = -errno`,
   but we don't alter the `ret` when memory allocation fails).

2) Missing ENOMEM handling when allocating @bcs.

3) Wrong `fclose()` on cleanup. We have:

     for (i = 0; i < nr_logs; i++)
         fclose(bcs[i].f);

   but at that point, the @nr_logs is already zero. Recall that
   @nr_logs is zeroed inside the `while (nr_logs)` loop. The
   merge_finish_file() decrements @nr_logs every loop cycle.

Fix and refactor them all.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 blktrace.c | 73 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/blktrace.c b/blktrace.c
index 619121c7..776bd4ab 100644
--- a/blktrace.c
+++ b/blktrace.c
@@ -700,21 +700,25 @@ static int write_trace(FILE *fp, struct blk_io_trace *t)
 
 int merge_blktrace_iologs(struct thread_data *td)
 {
+	int nr_logs_iter;
 	int nr_logs = get_max_str_idx(td->o.read_iolog_file);
-	struct blktrace_cursor *bcs = malloc(sizeof(struct blktrace_cursor) *
-					     nr_logs);
+	struct blktrace_cursor *bcs;
 	struct blktrace_cursor *bc;
 	FILE *merge_fp;
 	char *str, *ptr, *name, *merge_buf;
 	int i, ret;
 
+	bcs = calloc(nr_logs, sizeof(struct blktrace_cursor));
+	if (!bcs)
+		return -ENOMEM;
+
 	ret = init_merge_param_list(td->o.merge_blktrace_scalars, bcs, nr_logs,
 				    100, offsetof(struct blktrace_cursor,
 						  scalar));
 	if (ret) {
 		log_err("fio: merge_blktrace_scalars(%d) != nr_logs(%d)\n",
 			ret, nr_logs);
-		goto err_param;
+		goto out_free_bcs;
 	}
 
 	ret = init_merge_param_list(td->o.merge_blktrace_iters, bcs, nr_logs,
@@ -723,83 +727,96 @@ int merge_blktrace_iologs(struct thread_data *td)
 	if (ret) {
 		log_err("fio: merge_blktrace_iters(%d) != nr_logs(%d)\n",
 			ret, nr_logs);
-		goto err_param;
+		goto out_free_bcs;
 	}
 
 	/* setup output file */
 	merge_fp = fopen(td->o.merge_blktrace_file, "w");
+	if (!merge_fp) {
+		ret = -errno;
+		goto out_free_bcs;
+	}
+
 	merge_buf = malloc(128 * 1024);
-	if (!merge_buf)
-		goto err_out_file;
+	if (!merge_buf) {
+		ret = -ENOMEM;
+		goto out_close_merge_fp;
+	}
+
 	ret = setvbuf(merge_fp, merge_buf, _IOFBF, 128 * 1024);
-	if (ret)
-		goto err_merge_buf;
+	if (ret) {
+		ret = -errno;
+		goto out_free_merge_buf;
+	}
 
 	/* setup input files */
 	str = ptr = strdup(td->o.read_iolog_file);
+	if (!str) {
+		ret = -ENOMEM;
+		goto out_free_merge_buf;
+	}
+
 	nr_logs = 0;
 	for (i = 0; (name = get_next_str(&ptr)) != NULL; i++) {
 		bcs[i].f = fopen(name, "rb");
 		if (!bcs[i].f) {
 			log_err("fio: could not open file: %s\n", name);
 			ret = -errno;
-			free(str);
-			goto err_file;
+			goto out_close_bcs_files;
 		}
 		nr_logs++;
 
 		if (!is_blktrace(name, &bcs[i].swap)) {
 			log_err("fio: file is not a blktrace: %s\n", name);
-			free(str);
-			goto err_file;
+			goto out_close_bcs_files;
 		}
 
 		ret = read_trace(td, &bcs[i]);
 		if (ret < 0) {
-			free(str);
-			goto err_file;
+			goto out_close_bcs_files;
 		} else if (!ret) {
 			merge_finish_file(bcs, i, &nr_logs);
 			i--;
 		}
 	}
-	free(str);
 
 	/* merge files */
-	while (nr_logs) {
-		i = find_earliest_io(bcs, nr_logs);
+	nr_logs_iter = nr_logs;
+	while (nr_logs_iter) {
+		i = find_earliest_io(bcs, nr_logs_iter);
 		bc = &bcs[i];
 		/* skip over the pdu */
 		ret = discard_pdu(bc->f, &bc->t);
 		if (ret < 0) {
 			td_verror(td, -ret, "blktrace lseek");
-			goto err_file;
+			goto out_close_bcs_files;
 		}
 
 		ret = write_trace(merge_fp, &bc->t);
 		ret = read_trace(td, bc);
 		if (ret < 0)
-			goto err_file;
+			goto out_close_bcs_files;
 		else if (!ret)
-			merge_finish_file(bcs, i, &nr_logs);
+			merge_finish_file(bcs, i, &nr_logs_iter);
 	}
 
 	/* set iolog file to read from the newly merged file */
 	td->o.read_iolog_file = td->o.merge_blktrace_file;
 	ret = 0;
 
-err_file:
-	/* cleanup */
-	for (i = 0; i < nr_logs; i++) {
+out_close_bcs_files:
+	for (i = 0; i < nr_logs; i++)
 		fclose(bcs[i].f);
-	}
-err_merge_buf:
+	free(str);
+
+out_free_merge_buf:
 	free(merge_buf);
-err_out_file:
+
+out_close_merge_fp:
 	fflush(merge_fp);
 	fclose(merge_fp);
-err_param:
-	free(bcs);
 
+out_free_bcs:
+	free(bcs);
 	return ret;
 }
-- 
Ammar Faizi

[PATCH v1 5/8] blktrace: Add ENOMEM handling when allocating @ipo

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

We have many places that do `ipo = calloc(1, sizeof(*ipo))` but don't
handle the ENOMEM case. Add it.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 blktrace.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/blktrace.c b/blktrace.c
index 776bd4ab..95adf698 100644
--- a/blktrace.c
+++ b/blktrace.c
@@ -80,11 +80,15 @@ bool is_blktrace(const char *filename, int *need_swap)
 #define FMAJOR(dev)	((unsigned int) ((dev) >> FMINORBITS))
 #define FMINOR(dev)	((unsigned int) ((dev) & FMINORMASK))
 
-static void trace_add_open_close_event(struct thread_data *td, int fileno, enum file_log_act action)
+static void trace_add_open_close_event(struct thread_data *td, int fileno,
+				       enum file_log_act action)
 {
 	struct io_piece *ipo;
 
 	ipo = calloc(1, sizeof(*ipo));
+	if (!ipo)
+		return;
+
 	init_ipo(ipo);
 
 	ipo->ddir = DDIR_INVAL;
@@ -158,6 +162,9 @@ static void store_ipo(struct thread_data *td, unsigned long long offset,
 	struct io_piece *ipo;
 
 	ipo = calloc(1, sizeof(*ipo));
+	if (!ipo)
+		return;
+
 	init_ipo(ipo);
 
 	ipo->offset = offset * 512;
@@ -211,6 +218,9 @@ static bool handle_trace_discard(struct thread_data *td,
 		return false;
 
 	ipo = calloc(1, sizeof(*ipo));
+	if (!ipo)
+		return false;
+
 	init_ipo(ipo);
 	fileno = trace_add_file(td, t->device, cache);
 
@@ -288,6 +298,9 @@ static bool handle_trace_flush(struct thread_data *td, struct blk_io_trace *t,
 		return false;
 
 	ipo = calloc(1, sizeof(*ipo));
+	if (!ipo)
+		return false;
+
 	init_ipo(ipo);
 	fileno = trace_add_file(td, t->device, cache);
 
-- 
Ammar Faizi

[PATCH v1 6/8] blktrace: Add ENOMEM handling in `trace_add_open_close_event()` and its callers

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

When this function hits ENOMEM, there is no way to notify the callers
that we fail to allocate the memory. This changes the return value of
`trace_add_open_close_event()` to `int` so we can tell the caller if
it hits ENOMEM. Also, fix the callers to handle this error case.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 blktrace.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/blktrace.c b/blktrace.c
index 95adf698..98ba25d1 100644
--- a/blktrace.c
+++ b/blktrace.c
@@ -80,14 +80,14 @@ bool is_blktrace(const char *filename, int *need_swap)
 #define FMAJOR(dev)	((unsigned int) ((dev) >> FMINORBITS))
 #define FMINOR(dev)	((unsigned int) ((dev) & FMINORMASK))
 
-static void trace_add_open_close_event(struct thread_data *td, int fileno,
-				       enum file_log_act action)
+static int trace_add_open_close_event(struct thread_data *td, int fileno,
+				      enum file_log_act action)
 {
 	struct io_piece *ipo;
 
 	ipo = calloc(1, sizeof(*ipo));
 	if (!ipo)
-		return;
+		return -ENOMEM;
 
 	init_ipo(ipo);
 
@@ -95,6 +95,7 @@ static void trace_add_open_close_event(struct thread_data *td, int fileno,
 	ipo->fileno = fileno;
 	ipo->file_action = action;
 	flist_add_tail(&ipo->list, &td->io_log_list);
+	return 0;
 }
 
 static int trace_add_file(struct thread_data *td, __u32 device,
@@ -124,6 +125,7 @@ static int trace_add_file(struct thread_data *td, __u32 device,
 	strcpy(dev, "/dev");
 	if (blktrace_lookup_device(td->o.replay_redirect, dev, maj, min)) {
 		int fileno;
+		int err;
 
 		if (td->o.replay_redirect)
 			dprint(FD_BLKTRACE, "device lookup: %d/%d\n overridden"
@@ -137,7 +139,9 @@ static int trace_add_file(struct thread_data *td, __u32 device,
 		td->o.open_files++;
 		td->files[fileno]->major = maj;
 		td->files[fileno]->minor = min;
-		trace_add_open_close_event(td, fileno, FIO_LOG_OPEN_FILE);
+		err = trace_add_open_close_event(td, fileno, FIO_LOG_OPEN_FILE);
+		if (err < 0)
+			return err;
 		cache->fileno = fileno;
 	}
 
@@ -217,12 +221,15 @@ static bool handle_trace_discard(struct thread_data *td,
 	if (td->o.replay_skip & (1u << DDIR_TRIM))
 		return false;
 
+	fileno = trace_add_file(td, t->device, cache);
+	if (fileno < 0)
+		return false;
+
 	ipo = calloc(1, sizeof(*ipo));
 	if (!ipo)
 		return false;
 
 	init_ipo(ipo);
-	fileno = trace_add_file(td, t->device, cache);
 
 	ios[DDIR_TRIM]++;
 	if (t->bytes > bs[DDIR_TRIM])
@@ -261,6 +268,8 @@ static bool handle_trace_fs(struct thread_data *td, struct blk_io_trace *t,
 	int fileno;
 
 	fileno = trace_add_file(td, t->device, cache);
+	if (fileno < 0)
+		return false;
 
 	rw = (t->action & BLK_TC_ACT(BLK_TC_WRITE)) != 0;
 
@@ -297,12 +306,15 @@ static bool handle_trace_flush(struct thread_data *td, struct blk_io_trace *t,
 	if (td->o.replay_skip & (1u << DDIR_SYNC))
 		return false;
 
+	fileno = trace_add_file(td, t->device, cache);
+	if (fileno < 0)
+		return false;
+
 	ipo = calloc(1, sizeof(*ipo));
 	if (!ipo)
 		return false;
 
 	init_ipo(ipo);
-	fileno = trace_add_file(td, t->device, cache);
 
 	ipo->delay = ttime / 1000;
 	ipo->ddir = DDIR_SYNC;
@@ -560,8 +572,14 @@ bool read_blktrace(struct thread_data* td)
 		return true;
 	}
 
-	for_each_file(td, fiof, i)
-		trace_add_open_close_event(td, fiof->fileno, FIO_LOG_CLOSE_FILE);
+	for_each_file(td, fiof, i) {
+		int err;
+
+		err = trace_add_open_close_event(td, fiof->fileno,
+						 FIO_LOG_CLOSE_FILE);
+		if (err < 0)
+			goto err;
+	}
 
 	fclose(td->io_log_rfile);
 	td->io_log_rfile = NULL;
-- 
Ammar Faizi

[PATCH v1 7/8] client: Add ENOMEM handling on `realloc()` calls

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

We need to use a temporary variable when calling `realloc()`. If we do:

    ptr = realloc(ptr, new_size);

the `ptr` will be NULL when the `realloc()` hits ENOMEM and the old
pointer will be leaked. This fixes several places that do the above
`realloc()` call.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 client.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/client.c b/client.c
index 605a3ce5..c93c90eb 100644
--- a/client.c
+++ b/client.c
@@ -338,9 +338,16 @@ static void __fio_client_add_cmd_option(struct fio_client *client,
 					const char *opt)
 {
 	int index;
+	void *tmp;
 
 	index = client->argc++;
-	client->argv = realloc(client->argv, sizeof(char *) * client->argc);
+	tmp = realloc(client->argv, sizeof(char *) * client->argc);
+	if (!tmp) {
+		log_err("fio: cannot reallocate client->argv in %s\n", __func__);
+		return;
+	}
+
+	client->argv = tmp;
 	client->argv[index] = strdup(opt);
 	dprint(FD_NET, "client: add cmd %d: %s\n", index, opt);
 }
@@ -1535,12 +1542,17 @@ static void handle_start(struct fio_client *client, struct fio_net_cmd *cmd)
 	client->nr_stat = le32_to_cpu(pdu->stat_outputs);
 
 	if (client->jobs) {
+		void *tmp;
 		int i;
 
-		if (client->opt_lists)
+		tmp = realloc(client->opt_lists,
+			      client->jobs * sizeof(struct flist_head));
+		if (!tmp) {
 			free(client->opt_lists);
+			return;
+		}
+		client->opt_lists = tmp;
 
-		client->opt_lists = malloc(client->jobs * sizeof(struct flist_head));
 		for (i = 0; i < client->jobs; i++)
 			INIT_FLIST_HEAD(&client->opt_lists[i]);
 	}
@@ -1750,7 +1762,11 @@ fail:
 	}
 
 	size += sb.st_size;
-	rep = realloc(rep, size);
+	tmp = realloc(rep, size);
+	if (!tmp)
+		goto fail;
+
+	rep = tmp;
 	rep->size = cpu_to_le32((uint32_t) sb.st_size);
 
 	fd = open((char *)pdu->path, O_RDONLY);
-- 
Ammar Faizi

[PATCH v1 8/8] client: Add ENOMEM handling on `malloc()`, `calloc()` and `strdup()` calls

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Avoid a NULL pointer dereference bug. There are many places that don't
handle the ENOMEM case. Add it.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 client.c | 107 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 89 insertions(+), 18 deletions(-)

diff --git a/client.c b/client.c
index c93c90eb..da292334 100644
--- a/client.c
+++ b/client.c
@@ -376,8 +376,11 @@ static struct fio_client *get_new_client(void)
 {
 	struct fio_client *client;
 
-	client = malloc(sizeof(*client));
-	memset(client, 0, sizeof(*client));
+	client = calloc(1, sizeof(*client));
+	if (!client) {
+		log_err("fio: cannot allocate client in %s\n", __func__);
+		return NULL;
+	}
 
 	INIT_FLIST_HEAD(&client->list);
 	INIT_FLIST_HEAD(&client->hash_list);
@@ -397,6 +400,8 @@ struct fio_client *fio_client_add_explicit(struct client_ops *ops,
 	struct fio_client *client;
 
 	client = get_new_client();
+	if (!client)
+		return NULL;
 
 	if (type == Fio_client_socket)
 		client->is_sock = true;
@@ -473,6 +478,8 @@ int fio_client_add(struct client_ops *ops, const char *hostname, void **cookie)
 	}
 
 	client = get_new_client();
+	if (!client)
+		return -ENOMEM;
 
 	if (fio_server_parse_string(hostname, &client->hostname,
 					&client->is_sock, &client->port,
@@ -695,6 +702,10 @@ static int send_client_cmd_line(struct fio_client *client)
 	dprint(FD_NET, "client: send cmdline %d\n", client->argc);
 
 	lens = malloc(client->argc * sizeof(unsigned int));
+	if (!lens) {
+		log_err("fio: cannot allocate lens in %s\n", __func__);
+		return -ENOMEM;
+	}
 
 	/*
 	 * Find out how much mem we need
@@ -708,8 +719,13 @@ static int send_client_cmd_line(struct fio_client *client)
 	 * We need one cmd_line_pdu, and argc number of cmd_single_line_pdu
 	 */
 	mem += sizeof(*clp) + (client->argc * sizeof(*cslp));
-
 	pdu = malloc(mem);
+	if (!pdu) {
+		log_err("fio: cannot allocate mem in %s\n", __func__);
+		ret = -ENOMEM;
+		goto out_free_lens;
+	}
+
 	clp = pdu;
 	offset = sizeof(*clp);
 
@@ -722,11 +738,12 @@ static int send_client_cmd_line(struct fio_client *client)
 		offset += sizeof(*cslp) + arg_len;
 	}
 
-	free(lens);
 	clp->lines = cpu_to_le16(client->argc);
 	clp->client_type = __cpu_to_le16(client->type);
 	ret = fio_net_send_cmd(client->fd, FIO_NET_CMD_JOBLINE, pdu, mem, NULL, NULL);
 	free(pdu);
+out_free_lens:
+	free(lens);
 	return ret;
 }
 
@@ -800,8 +817,12 @@ static int __fio_client_send_remote_ini(struct fio_client *client,
 	dprint(FD_NET, "send remote ini %s to %s\n", filename, client->hostname);
 
 	p_size = sizeof(*pdu) + strlen(filename) + 1;
-	pdu = malloc(p_size);
-	memset(pdu, 0, p_size);
+	pdu = calloc(1, p_size);
+	if (!pdu) {
+		log_err("fio: cannot allocate pdu in %s\n", __func__);
+		return -ENOMEM;
+	}
+
 	pdu->name_len = strlen(filename);
 	strcpy((char *) pdu->file, filename);
 	pdu->client_type = cpu_to_le16((uint16_t) client->type);
@@ -839,8 +860,7 @@ static int __fio_client_send_local_ini(struct fio_client *client,
 	if (fstat(fd, &sb) < 0) {
 		ret = -errno;
 		log_err("fio: job file stat: %s\n", strerror(errno));
-		close(fd);
-		return ret;
+		goto out_close_fd;
 	}
 
 	/*
@@ -849,15 +869,20 @@ static int __fio_client_send_local_ini(struct fio_client *client,
 	sb.st_size += OPT_LEN_MAX;
 	p_size = sb.st_size + sizeof(*pdu);
 	pdu = malloc(p_size);
+	if (!pdu) {
+		log_err("fio: cannot allocate pdu in %s\n", __func__);
+		ret = -ENOMEM;
+		goto out_close_fd;
+	}
 	buf = pdu->buf;
 
 	len = sb.st_size;
 	p = buf;
-	if (read_ini_data(fd, p, len)) {
+	ret = read_ini_data(fd, p, len);
+	if (ret) {
 		log_err("fio: failed reading job file %s\n", filename);
-		close(fd);
-		free(pdu);
-		return 1;
+		ret = -ret;
+		goto out_free_pdu;
 	}
 
 	pdu->buf_len = __cpu_to_le32(sb.st_size);
@@ -865,7 +890,11 @@ static int __fio_client_send_local_ini(struct fio_client *client,
 
 	client->sent_job = true;
 	ret = fio_net_send_cmd(client->fd, FIO_NET_CMD_JOB, pdu, p_size, NULL, NULL);
+
+out_free_pdu:
 	free(pdu);
+
+out_close_fd:
 	close(fd);
 	return ret;
 }
@@ -1171,11 +1200,30 @@ static void handle_job_opt(struct fio_client *client, struct fio_net_cmd *cmd)
 	} else if (client->opt_lists) {
 		struct flist_head *opt_list = &client->opt_lists[pdu->groupid];
 		struct print_option *p;
+		char *v;
 
 		p = malloc(sizeof(*p));
+		if (!p)
+			return;
+
 		p->name = strdup((const char *)pdu->name);
-		p->value = pdu->value[0] ? strdup((const char *)pdu->value) :
-			NULL;
+		if (!p->name) {
+			free(p);
+			return;
+		}
+
+		if (pdu->value[0]) {
+			v = strdup((const char *)pdu->value);
+			if (!v) {
+				free(p->name);
+				free(p);
+				return;
+			}
+		} else {
+			v = NULL;
+		}
+
+		p->value = v;
 		flist_add_tail(&p->list, opt_list);
 	}
 }
@@ -1512,6 +1560,14 @@ static void handle_probe(struct fio_client *client, struct fio_net_cmd *cmd)
 	const char *os, *arch;
 	char bit[16];
 
+	if (!client->name) {
+		client->name = strdup((char *) probe->hostname);
+		if (!client->name) {
+			log_err("fio: cannot allocate client->name in %s\n", __func__);
+			return;
+		}
+	}
+
 	os = fio_get_os_string(probe->os);
 	if (!os)
 		os = "unknown";
@@ -1528,9 +1584,6 @@ static void handle_probe(struct fio_client *client, struct fio_net_cmd *cmd)
 			probe->hostname, probe->bigendian, bit, os, arch,
 			probe->fio_version, (unsigned long) probe->flags);
 	}
-
-	if (!client->name)
-		client->name = strdup((char *) probe->hostname);
 }
 
 static void handle_start(struct fio_client *client, struct fio_net_cmd *cmd)
@@ -1613,6 +1666,10 @@ static struct cmd_iolog_pdu *convert_iolog_gz(struct fio_net_cmd *cmd,
 	else
 		total = nr_samples * __log_entry_sz(le32_to_cpu(pdu->log_offset));
 	ret = malloc(total + sizeof(*pdu));
+	if (!ret) {
+		log_err("fio: cannot allocate ret in %s\n", __func__);
+		return NULL;
+	}
 	ret->nr_samples = nr_samples;
 
 	memcpy(ret, pdu, sizeof(*pdu));
@@ -1745,13 +1802,17 @@ static void sendfile_reply(int fd, struct cmd_sendfile_reply *rep,
 static int fio_send_file(struct fio_client *client, struct cmd_sendfile *pdu,
 			 uint64_t tag)
 {
-	struct cmd_sendfile_reply *rep;
+	struct cmd_sendfile_reply *rep, *tmp;
 	struct stat sb;
 	size_t size;
 	int fd;
 
 	size = sizeof(*rep);
 	rep = malloc(size);
+	if (!rep) {
+		log_err("fio: cannot allocate rep in %s\n", __func__);
+		return 1;
+	}
 
 	if (stat((char *)pdu->path, &sb) < 0) {
 fail:
@@ -1956,6 +2017,10 @@ int fio_clients_send_trigger(const char *cmd)
 		client = flist_entry(entry, struct fio_client, list);
 
 		pdu = malloc(sizeof(*pdu) + slen);
+		if (!pdu) {
+			log_err("fio: cannot allocate pdu in %s\n", __func__);
+			return 1;
+		}
 		pdu->len = cpu_to_le16((uint16_t) slen);
 		if (slen)
 			memcpy(pdu->cmd, cmd, slen);
@@ -1980,6 +2045,10 @@ static void request_client_etas(struct client_ops *ops)
 	dprint(FD_NET, "client: request eta (%d)\n", nr_clients);
 
 	eta = calloc(1, sizeof(*eta) + __THREAD_RUNSTR_SZ(REAL_MAX_JOBS));
+	if (!eta) {
+		log_err("fio: cannot allocate eta in %s\n", __func__);
+		return;
+	}
 	eta->pending = nr_clients;
 
 	flist_for_each(entry, &client_list) {
@@ -2107,6 +2176,8 @@ int fio_handle_clients(struct client_ops *ops)
 	fio_gettime(&eta_ts, NULL);
 
 	pfds = malloc(nr_clients * sizeof(struct pollfd));
+	if (!pfds)
+		return -ENOMEM;
 
 	init_thread_stat(&client_ts);
 	init_group_run_stat(&client_gs);
-- 
Ammar Faizi

RE: [PATCH v1 1/8] cgroup: Add ENOMEM handling on a `malloc()` call

[Vincent Fu]

> -----Original Message-----
> From: Ammar Faizi [mailto:ammarfaizi2@gnuweeb.org]
> Sent: Thursday, April 28, 2022 8:47 PM
> To: Jens Axboe <axboe@kernel.dk>
> Cc: Ammar Faizi <ammarfaizi2@gnuweeb.org>; Alviro Iskandar Setiawan
> <alviro.iskandar@gnuweeb.org>; Niklas Cassel
> <Niklas.Cassel@wdc.com>; fio Mailing List <fio@vger.kernel.org>;
> GNU/Weeb Mailing List <gwml@vger.gnuweeb.org>
> Subject: [PATCH v1 1/8] cgroup: Add ENOMEM handling on a `malloc()`
> call
> 
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> Avoid a NULL pointer dereference bug when `ENOMEM`.
> 
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Looks good.

Reviewed-by: Vincent Fu <vincent.fu@samsung.net>

> ---
>  cgroup.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/cgroup.c b/cgroup.c
> index 77e31a4d..b559b70f 100644
> --- a/cgroup.c
> +++ b/cgroup.c
> @@ -114,6 +114,8 @@ void cgroup_kill(struct flist_head *clist)
>  static char *get_cgroup_root(struct thread_data *td, struct cgroup_mnt
> *mnt)
>  {
>  	char *str = malloc(64);
> +	if (!str)
> +		return NULL;
> 
>  	if (td->o.cgroup)
>  		sprintf(str, "%s/%s", mnt->path, td->o.cgroup);
> @@ -178,6 +180,8 @@ int cgroup_setup(struct thread_data *td, struct
> flist_head *clist, struct cgroup
>  	 * Create container, if it doesn't exist
>  	 */
>  	root = get_cgroup_root(td, *mnt);
> +	if (!root)
> +		return 1;
>  	if (mkdir(root, 0755) < 0) {
>  		int __e = errno;
> 
> --
> Ammar Faizi

RE: [PATCH v1 3/8] engines/net: Add ENOMEM handling on a `malloc()` call

[Vincent Fu]

> -----Original Message-----
> From: Ammar Faizi [mailto:ammarfaizi2@gnuweeb.org]
> Sent: Thursday, April 28, 2022 8:47 PM
> To: Jens Axboe <axboe@kernel.dk>
> Cc: Ammar Faizi <ammarfaizi2@gnuweeb.org>; Alviro Iskandar Setiawan
> <alviro.iskandar@gnuweeb.org>; Niklas Cassel
> <Niklas.Cassel@wdc.com>; fio Mailing List <fio@vger.kernel.org>;
> GNU/Weeb Mailing List <gwml@vger.gnuweeb.org>
> Subject: [PATCH v1 3/8] engines/net: Add ENOMEM handling on a
> `malloc()` call
> 
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> Replace `malloc()` + `memset()` with `calloc()` to simplify the call.
> `calloc()` zeroes the allocated memory, so we can avoid `memset()`.
> Also, handle the `ENOMEM` case.
> 
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
>  engines/net.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/engines/net.c b/engines/net.c
> index c6cec584..24c1463d 100644
> --- a/engines/net.c
> +++ b/engines/net.c
> @@ -1370,9 +1370,9 @@ static int fio_netio_setup(struct thread_data
> *td)
>  	}
> 
>  	if (!td->io_ops_data) {
> -		nd = malloc(sizeof(*nd));
> -
> -		memset(nd, 0, sizeof(*nd));
> +		nd = calloc(1, sizeof(*nd));
> +		if (!nd)
> +			return 1;
>  		nd->listenfd = -1;
>  		nd->pipes[0] = nd->pipes[1] = -1;
>  		td->io_ops_data = nd;
> @@ -1391,7 +1391,8 @@ static int fio_netio_setup_splice(struct
> thread_data *td)
>  {
>  	struct netio_data *nd;
> 
> -	fio_netio_setup(td);
> +	if (fio_netio_setup(td))
> +		return 1;
> 
>  	nd = td->io_ops_data;
>  	if (nd) {
> --
> Ammar Faizi


Looks good.

Reviewed-by: Vincent Fu <vincent.fu@samsung.net>

Re: [PATCH v1 0/8] fio error handling fixes

[Jens Axboe]

On 4/28/22 6:46 PM, Ammar Faizi wrote:
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> Hi Jens,
> 
> This series contains patches that were dropped from my previous thread:
> 
>   https://lore.kernel.org/fio/20220428203954.222165-1-ammarfaizi2@gnuweeb.org
> 
> Plus, I have more patches in this series. This series contains error
> handling fixes, mostly about ENOMEM. There are 8 patches in this
> series. All of them are ENOMEM handling stuff except that patch #4 has
> extra fixes and a bit of refactoring.

While I agree on good error handling in general, I'm a bit dubious at
malloc() related NULL returns. Have you ever seen malloc() return NULL?
On Linux, or somewhere else? Most systems overcommit themselves to
death, and things will death spiral before you ever get NULL.

I'm happy to be convinced otherwise, just naturally skeptical that
there's any real value in making changes like this.

-- 
Jens Axboe

Re: [PATCH v1 0/8] fio error handling fixes

[Ammar Faizi]

On Sat, Apr 30, 2022 at 1:21 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 4/28/22 6:46 PM, Ammar Faizi wrote:
> > From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> >
> > Hi Jens,
> >
> > This series contains patches that were dropped from my previous thread:
> >
> >   https://lore.kernel.org/fio/20220428203954.222165-1-ammarfaizi2@gnuweeb.org
> >
> > Plus, I have more patches in this series. This series contains error
> > handling fixes, mostly about ENOMEM. There are 8 patches in this
> > series. All of them are ENOMEM handling stuff except that patch #4 has
> > extra fixes and a bit of refactoring.
>
> While I agree on good error handling in general, I'm a bit dubious at
> malloc() related NULL returns. Have you ever seen malloc() return NULL?
> On Linux, or somewhere else? Most systems overcommit themselves to
> death, and things will death spiral before you ever get NULL.

Yes, you're right. malloc() returns NULL is a very rare case. I don't
think fio users will ever see that in general.

I have seen malloc() return NULL when a program is in jail (e.g.
inside cgroup, or whatever that puts a limit on the memory resource).
But obviously, it is not reasonable for fio. Although that's one of my
motivations, it's actually not the main one that brought me to do this
series.

> I'm happy to be convinced otherwise, just naturally skeptical that
> there's any real value in making changes like this.

I am a bit bothered seeing inconsistent error handling in the
codebase. From what I see in the fio codebase today, sometimes we
handle ENOMEM cases, sometimes we don't. I want to make it consistent
in the hope we can improve the quality of the overall error handling.

If you are fine with that, I can send more incremental patches for
review in the future to fix those inconsistencies. It may be tiring
and progressive work. But I am okay doing that.

However, if you NAK this series. I will start fixing other broken
things I can find, like in patch #4 for example. And that's fine.

-- 
Ammar Faizi

Re: [PATCH v1 0/8] fio error handling fixes

[Jens Axboe]

On 4/29/22 2:15 PM, Ammar Faizi wrote:
> On Sat, Apr 30, 2022 at 1:21 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 4/28/22 6:46 PM, Ammar Faizi wrote:
>>> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
>>>
>>> Hi Jens,
>>>
>>> This series contains patches that were dropped from my previous thread:
>>>
>>>   https://lore.kernel.org/fio/20220428203954.222165-1-ammarfaizi2@gnuweeb.org
>>>
>>> Plus, I have more patches in this series. This series contains error
>>> handling fixes, mostly about ENOMEM. There are 8 patches in this
>>> series. All of them are ENOMEM handling stuff except that patch #4 has
>>> extra fixes and a bit of refactoring.
>>
>> While I agree on good error handling in general, I'm a bit dubious at
>> malloc() related NULL returns. Have you ever seen malloc() return NULL?
>> On Linux, or somewhere else? Most systems overcommit themselves to
>> death, and things will death spiral before you ever get NULL.
> 
> Yes, you're right. malloc() returns NULL is a very rare case. I don't
> think fio users will ever see that in general.
> 
> I have seen malloc() return NULL when a program is in jail (e.g.
> inside cgroup, or whatever that puts a limit on the memory resource).
> But obviously, it is not reasonable for fio. Although that's one of my
> motivations, it's actually not the main one that brought me to do this
> series.
> 
>> I'm happy to be convinced otherwise, just naturally skeptical that
>> there's any real value in making changes like this.
> 
> I am a bit bothered seeing inconsistent error handling in the
> codebase. From what I see in the fio codebase today, sometimes we
> handle ENOMEM cases, sometimes we don't. I want to make it consistent
> in the hope we can improve the quality of the overall error handling.

Any internal allocator use case must handle errors, but malloc() and
friends most likely do not, or at least do so inconsistently I'm sure.

> If you are fine with that, I can send more incremental patches for
> review in the future to fix those inconsistencies. It may be tiring
> and progressive work. But I am okay doing that.
> 
> However, if you NAK this series. I will start fixing other broken
> things I can find, like in patch #4 for example. And that's fine.

It's not that I'm against cleaning up these bits, my only worry is that
it's quite easy to add errors when fixing these things up. And if the
choice is between some inconsistency in alloc error handling or
potentially adding error that users WILL actually hit, then I'd pick the
former any day.

I'll leave the decision to you. If you feel they are all trivial and
risk of introducing errors it close to zero, then I'm happy to apply
them. I just don't have the strongest motivation myself for doing
thorough reviews of it, just don't have time to do so.

-- 
Jens Axboe

Re: [PATCH v1 1/8] cgroup: Add ENOMEM handling on a `malloc()` call

[Alviro Iskandar Setiawan]

On Fri, Apr 29, 2022 at 7:46 AM Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
>
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
>
> Avoid a NULL pointer dereference bug when `ENOMEM`.
>
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
>  cgroup.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/cgroup.c b/cgroup.c
> index 77e31a4d..b559b70f 100644
> --- a/cgroup.c
> +++ b/cgroup.c
> @@ -114,6 +114,8 @@ void cgroup_kill(struct flist_head *clist)
>  static char *get_cgroup_root(struct thread_data *td, struct cgroup_mnt *mnt)
>  {
>         char *str = malloc(64);
> +       if (!str)
> +               return NULL;
>
>         if (td->o.cgroup)
>                 sprintf(str, "%s/%s", mnt->path, td->o.cgroup);
> @@ -178,6 +180,8 @@ int cgroup_setup(struct thread_data *td, struct flist_head *clist, struct cgroup
>          * Create container, if it doesn't exist
>          */
>         root = get_cgroup_root(td, *mnt);
> +       if (!root)
> +               return 1;
>         if (mkdir(root, 0755) < 0) {
>                 int __e = errno;
>

Reviewed-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>

tq

-- Viro

Re: [PATCH v1 8/8] client: Add ENOMEM handling on `malloc()`, `calloc()` and `strdup()` calls

[Jens Axboe]

On 4/28/22 6:47 PM, Ammar Faizi wrote:
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> Avoid a NULL pointer dereference bug. There are many places that don't
> handle the ENOMEM case. Add it.
> 
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
>  client.c | 107 +++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 89 insertions(+), 18 deletions(-)
> 
> @@ -473,6 +478,8 @@ int fio_client_add(struct client_ops *ops, const char *hostname, void **cookie)
>  	}
>  
>  	client = get_new_client();
> +	if (!client)
> +		return -ENOMEM;

I don't think we should be using -errno in fio if we can avoid it. Are
there cases where we need to discern one failure from another for alloc
fixes? Looking at this case, we really just want a non-zero return.

-- 
Jens Axboe