From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id E7D3B46750 for ; Fri, 16 Jun 2023 23:15:49 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 8C3B368C196; Sat, 17 Jun 2023 02:15:46 +0300 (EEST) Received: from mail-yb1-f175.google.com (mail-yb1-f175.google.com [209.85.219.175]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C235668C024 for ; Sat, 17 Jun 2023 02:15:40 +0300 (EEST) Received: by mail-yb1-f175.google.com with SMTP id 3f1490d57ef6-bad3013ed55so159116276.0 for ; Fri, 16 Jun 2023 16:15:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686957339; x=1689549339; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=tMUCjmgELaWku6Fq9VpH8h/ldEtNkSf0dNpWaU0+W8U=; b=qO9LGNoBzaQZcplgRQwZW3bF9ZWuVFR+SAkOYFPPxShR/JP+p3Rwq3lXlvTZlwNsWS OzIM/28kF4DtCyx7lhF3q3QwpLJyPoxK6K8AZWFnPEDtCKFMhApzP2vkQRooFzN1Nyq+ y11IEJy8uI1eLsJYdPQckcU7DAzxhy2HMVadNmuSJPNiX6COH8IgmcTrDSWKpj+IrV5V za4xaVsRj+OGAnqjCkjbaGlCQZ5a0gWjZILJohyJ5r89ue+abvtJNTAhiidUOSIkGctf ssWte/Xnycnvmb9EjCBq01BP/MoHdGQJT0sb3ZB54gqmwY1x/ivZeqQqOyzMz1e+rAeo B9DA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686957339; x=1689549339; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=tMUCjmgELaWku6Fq9VpH8h/ldEtNkSf0dNpWaU0+W8U=; b=DfhN/LRnhZ/+4mfG2r9/8CmFUhRAtkoLrvYFk2XW5opWf0o3Bmn44fEj0Io11WR7mq 2BuN1PoW3Qx4mw3n+hgyWZ5UDyxJrfS7fuFysb1pfzqQXJXBh8zuC1v2h/M73+nIB4Zq 4d7IckOl0II1K3C/ib/XdkKihlTFUcs5sbZUIq7j2nEWC593WDeYUWBpxxtr+5/mEncT RwYL+7Gv4/8JMy2T4Lzf1FC+cuNW/ZctOksbDds5l9dP08a2RSny6pHAJFefYBY1Ntsb 16VVQZs/qovxgavSVp9qUJxiXE3Ma+OIblelDGqKXiVbKAQaKrHJTrohSmV/XeEd13TR wcqw== X-Gm-Message-State: AC+VfDygZCCqSnUrPyPDPkJDyIpeDTNCix1yCLJ9kOK7Gqu9sAVHvRmZ LLiyKntP7gzDgMNc+Jte12Dt5EQ8png= X-Google-Smtp-Source: ACHHUZ4K0hFnhc68C/8dn4DrElZ3rXyFL9POC2qIwmaHjqwu42QFPYKJfxSa5FXcS/N8Y2CyZqHyeQ== X-Received: by 2002:a81:1709:0:b0:56c:e6a6:5bd0 with SMTP id 9-20020a811709000000b0056ce6a65bd0mr1808229ywx.3.1686957338802; Fri, 16 Jun 2023 16:15:38 -0700 (PDT) Received: from [192.168.1.35] (c-98-224-219-15.hsd1.mi.comcast.net. [98.224.219.15]) by smtp.gmail.com with ESMTPSA id w123-20020a0dd481000000b005616d72f435sm352357ywd.116.2023.06.16.16.15.37 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 16 Jun 2023 16:15:38 -0700 (PDT) Message-ID: <8863efa1-3363-6268-2468-1b7a0d3eec9f@gmail.com> Date: Fri, 16 Jun 2023 19:15:37 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Content-Language: en-US-large To: ffmpeg-devel@ffmpeg.org References: <20230612132256.84549-1-leo.izen@gmail.com> From: Leo Izen In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH v2] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 6/16/23 15:00, Andreas Rheinhardt wrote: > Leo Izen: >> This switches the jpegxl_collect_codestream_header function to use >> avcodec/bytestream2, which better enforces barriers, and should avoid >> overrunning buffers with jxlp boxes if the size is zero or if the size >> is so small the box is invalid. >> >> Signed-off-by: Leo Izen >> --- >> libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++---------------- >> 1 file changed, 30 insertions(+), 26 deletions(-) >> >> diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c >> index 6ea6c46d8f..ec400c955c 100644 >> --- a/libavformat/jpegxl_anim_dec.c >> +++ b/libavformat/jpegxl_anim_dec.c >> @@ -28,6 +28,7 @@ >> #include >> #include >> >> +#include "libavcodec/bytestream.h" >> #define BITSTREAM_READER_LE >> #include "libavcodec/get_bits.h" >> >> @@ -48,62 +49,65 @@ typedef struct JXLAnimDemuxContext { >> * returns the number of bytes consumed from input, may be greater than input_len >> * if the input doesn't end on an ISOBMFF-box boundary >> */ >> -static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, uint8_t *buffer, int buflen, int *copied) { >> - const uint8_t *b = input_buffer; >> +static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, >> + uint8_t *buffer, int buflen, int *copied) { >> + GetByteContext gb; >> *copied = 0; >> + bytestream2_init(&gb, input_buffer, input_len); >> >> while (1) { >> uint64_t size; >> uint32_t tag; >> int head_size = 8; >> >> - if (b - input_buffer >= input_len - 16) >> + if (bytestream2_get_bytes_left(&gb) < 16) >> break; >> >> - size = AV_RB32(b); >> - b += 4; >> + size = bytestream2_get_be32(&gb); >> if (size == 1) { >> - size = AV_RB64(b); >> - b += 8; >> + size = bytestream2_get_be64(&gb); >> head_size = 16; >> } >> /* invalid ISOBMFF size */ >> - if (size > 0 && size <= head_size) >> + if (size && size <= head_size) >> return AVERROR_INVALIDDATA; >> - if (size > 0) >> + if (size) >> size -= head_size; >> >> - tag = AV_RL32(b); >> - b += 4; >> + tag = bytestream2_get_le32(&gb); >> if (tag == MKTAG('j', 'x', 'l', 'p')) { >> - b += 4; >> - size -= 4; >> + if (bytestream2_get_bytes_left(&gb) < 4) >> + break; >> + bytestream2_skip(&gb, 4); >> + if (size) { >> + if (size <= 4) >> + return AVERROR_INVALIDDATA; >> + size -= 4; >> + } >> } >> + /* >> + * size = 0 means "until EOF". this is legal but uncommon >> + * here we just set it to the remaining size of the probe buffer >> + */ >> + if (!size) >> + size = bytestream2_get_bytes_left(&gb); >> >> if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) { >> - /* >> - * size = 0 means "until EOF". this is legal but uncommon >> - * here we just set it to the remaining size of the probe buffer >> - * which at this point should always be nonnegative >> - */ >> - if (size == 0 || size > input_len - (b - input_buffer)) >> - size = input_len - (b - input_buffer); >> - >> if (size > buflen - *copied) >> size = buflen - *copied; >> /* >> * arbitrary chunking of the payload makes this memcpy hard to avoid >> * in practice this will only be performed one or two times at most >> */ >> - memcpy(buffer + *copied, b, size); >> - *copied += size; >> + *copied += bytestream2_get_buffer(&gb, buffer + *copied, size); >> + } else { >> + bytestream2_skip(&gb, size); >> } >> - b += size; >> - if (b >= input_buffer + input_len || *copied >= buflen) >> + if (bytestream2_get_bytes_left(&gb) <= 0 || *copied >= buflen) >> break; >> } >> >> - return b - input_buffer; >> + return bytestream2_tell(&gb); >> } >> >> static int jpegxl_anim_probe(const AVProbeData *p) > > Is there an actual (potential) overrun or is this just a precaution? > > - Andreas > Yea, michaelni sent a cvslog email about it earlier, mail ID <20230608002033.GF870501@pb2>. This fixes that and switches to bytestream2 at Anton's recommendation. - Leo Izen _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".