On Wed, Apr 27, 2022 at 04:11:22PM +0700, Ammar Faizi wrote:
> This handles memory allocation failure and several improvements.
>
> 1) Change `malloc(n * size)` to `calloc(n, size)`. This is to avoid
> multiplication on `malloc()` because it doesn't do overflow check.
> Also, `calloc()` zeroes the allocated memory, so we can omit the
> zeroing elements of the array inside the loop.
>
> 2) Make sure we are calling `free()` properly if we fail. Use goto to
> do this.
>
> Signed-off-by: Ammar Faizi <ammarfaizi2@xxxxxxxxxxx>
> ---
> stat.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/stat.c b/stat.c
> index 949af5ed..cc9be02e 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -2429,7 +2429,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 = calloc(groupid + 1, sizeof(*runstats));
Hello Ammar,
here you allocate runstats with calloc.
> + if (!runstats) {
> + log_err("fio: failed to allocate runstats\n");
> + return;
> + }
>
> for (i = 0; i < groupid + 1; i++)
> init_group_run_stat(&runstats[i]);
Here you call init_group_run_stat() on each runstats,
which calls memset(). Seems a bit excessive to clear
the memory to zero twice.
If you intend to modify init_group_run_stat(), be careful,
as it is also called by client.c
Kind regards,
Niklas
> @@ -2454,14 +2458,21 @@ 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 = calloc(nr_ts, sizeof(*threadstats));
> + if (!threadstats) {
> + log_err("fio: failed to allocate threadstats\n");
> + goto out_free_runstats;
> + }
>
> - for (i = 0; i < nr_ts; i++) {
> - init_thread_stat(&threadstats[i]);
> - opt_lists[i] = NULL;
> + 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++)
> + init_thread_stat(&threadstats[i]);
> +
> init_per_prio_stats(threadstats, nr_ts);
>
> j = 0;
> @@ -2709,15 +2720,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)
> --
> Ammar Faizi
>
