On 20.10.21 г. 17:09, Nikolay Borisov wrote:
>
>
> On 1.09.21 г. 20:01, Omar Sandoval wrote:
>> From: Boris Burkov <boris@xxxxxx>
>>
>> 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).
>> 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@xxxxxx>
>> ---
>> 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.
>
>
> <snip>
>
