axboe / fio

Flexible I/O Tester
GNU General Public License v2.0
5.01k stars 1.23k forks source link

scalloc doesn't clear the buffer #1772

Closed vincentkfu closed 1 month ago

vincentkfu commented 1 month ago

smalloc.c:scalloc() is defined in this way:

void *scalloc(size_t nmemb, size_t size)
{
        return smalloc(nmemb * size);
}

void *smalloc(size_t size)
{
        unsigned int i, end_pool;

        if (size != (unsigned int) size)
                return NULL;

        i = last_pool;
        end_pool = nr_pools;

        do {
                for (; i < end_pool; i++) {
                        void *ptr = smalloc_pool(&mp[i], size);

                        if (ptr) {
                                last_pool = i;
                                return ptr;
                        }
                }
                if (last_pool) {
                        end_pool = last_pool;
                        last_pool = i = 0;
                        continue;
                }

                break;
        } while (1);

        log_err("smalloc: OOM. Consider using --alloc-size to increase the "
                "shared memory available.\n");
        smalloc_debug(size);
        return NULL;
}

scalloc() does not actually clear the buffer returned to the caller. I know of no reports of this actually causing problems but we should fix this.

axboe commented 1 month ago

IIRC, smalloc() used to always clear the allocated region, which is why scalloc() didn't do anything outside of that. Either my memory is wrong, or it just got lost down the line.

I think it was this:

commit 9c3e13e3314da394698ca32f21cc46d46b7cfe47
Author: Jens Axboe <axboe@fb.com>
Date:   Sat Nov 7 17:33:38 2015 -0700

    smalloc: only clear the bitmap, not the whole pool
axboe commented 1 month ago

Just add the memset() to calloc on successful alloc?

Edit: heh, I think you can just do:

$ git revert a640ed36829f3be6d9dd8c7974dba41b9b59e6a5

and be done with it.

vincentkfu commented 1 month ago

Reverted. Thanks for the history!

vincentkfu commented 3 weeks ago

This turned out not to be a bug:

https://git.kernel.dk/cgit/fio/commit/?id=f956fed4d181d41a5b1c49bba9dce46d8197d428

axboe commented 3 weeks ago

Should probably have a comment in there, since we've now done it, reverted it, and reverted the revert.

vincentkfu commented 3 weeks ago

Done: https://git.kernel.dk/cgit/fio/commit/?id=bd7492a47a5bb0ec66a05ae0cff0c9065afb0f88