Re: [PATCH v11 02/10] btrfs-progs: receive: dynamically allocate sctx->read_buf
From: Nikolay Borisov <hidden>
Date: 2021-10-20 14:09:23
Also in:
linux-btrfs, linux-fsdevel
On 1.09.21 г. 20:01, Omar Sandoval wrote:
From: Boris Burkov <redacted> In send stream v2, write commands can now be an arbitrary size. For that reason, we can no longer allocate a fixed array in sctx for read_cmd. Instead, read_cmd dynamically allocates sctx->read_buf. To avoid needless reallocations, we reuse read_buf between read_cmd calls by also keeping track of the size of the allocated buffer in sctx->read_buf_sz. We do the first allocation of the old default size at the start of processing the stream, and we only reallocate if we encounter a command that needs a larger buffer. Signed-off-by: Boris Burkov <redacted> --- common/send-stream.c | 55 ++++++++++++++++++++++++++++---------------- send.h | 2 +- 2 files changed, 36 insertions(+), 21 deletions(-)
<snip>
quoted hunk ↗ jump to hunk
@@ -124,18 +125,22 @@ static int read_cmd(struct btrfs_send_stream *sctx) goto out; } - sctx->cmd_hdr = (struct btrfs_cmd_header *)sctx->read_buf; - cmd = le16_to_cpu(sctx->cmd_hdr->cmd); - cmd_len = le32_to_cpu(sctx->cmd_hdr->len); - - if (cmd_len + sizeof(*sctx->cmd_hdr) >= sizeof(sctx->read_buf)) { - ret = -EINVAL; - error("command length %u too big for buffer %zu", - cmd_len, sizeof(sctx->read_buf)); - goto out; + cmd_hdr = (struct btrfs_cmd_header *)sctx->read_buf; + cmd_len = le32_to_cpu(cmd_hdr->len); + cmd = le16_to_cpu(cmd_hdr->cmd); + buf_len = sizeof(*cmd_hdr) + cmd_len; + if (sctx->read_buf_sz < buf_len) { + sctx->read_buf = realloc(sctx->read_buf, buf_len); + if (!sctx->read_buf) {
nit: This is prone to a memory leak, because according to https://en.cppreference.com/w/c/memory/realloc If there is not enough memory, the old memory block is not freed and null pointer is returned. This means if realloc fails it will overwrite sctx->read_buf with NULL, yet the old memory won't be freed which will cause a memory leak. It can be argued that's not critical since we'll very quickly terminate the program afterwards but still. <snip>