Re: [PATCH v11 02/10] btrfs-progs: receive: dynamically allocate sctx->read_buf

[Omar Sandoval <osandov@osandov.com>]

On Wed, Oct 20, 2021 at 05:35:42PM +0300, Nikolay Borisov wrote:
> 
> 
> On 20.10.21 г. 17:09, Nikolay Borisov wrote:
> > 
> > 
> > On 1.09.21 г. 20:01, Omar Sandoval wrote:
> >> From: Boris Burkov <boris@bur.io>
> >>
> >> In send stream v2, write commands can now be an arbitrary size. For that
> 
> nit: Actually can't commands really be up-to BTRFS_MAX_COMPRESSED + 16K
> really  or are we going to leave this as an implementation detail? I'm
> fine either way but looking at the changelog of patch 12 in the kernel
> series doesn't really mention of arbitrary size, instead it explicitly
> is talking about sending the max compressed extent size (128K) + some
> space for metadata (the 16K above).

Patch 10 mentions it in the changelog: "It also documents two changes to the
send stream format in v2: the receiver shouldn't assume a maximum command size,
and the DATA attribute is encoded differently to allow for writes larger than
64k".

And in send.h:

-#define BTRFS_SEND_BUF_SIZE SZ_64K
+/*
+ * In send stream v1, no command is larger than 64k. In send stream v2, no limit
+ * should be assumed.
+ */
+#define BTRFS_SEND_BUF_SIZE_V1 SZ_64K

You're correct that right now the limit is BTRFS_MAX_COMPRESSED + 16k,
but I think it's better if userspace doesn't make any assumptions about
that in case we want to send larger commands in the future.

> >> 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 <boris@bur.io>
> >> ---
> >>  common/send-stream.c | 55 ++++++++++++++++++++++++++++----------------
> >>  send.h               |  2 +-
> >>  2 files changed, 36 insertions(+), 21 deletions(-)
> >>
> > 
> > <snip>
> > 
> >> @@ -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.

Good catch, I'll fix that.