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 3039640C02 for ; Sun, 8 May 2022 02:27:59 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 33F4A68B2D5; Sun, 8 May 2022 05:27:57 +0300 (EEST) Received: from NAM12-MW2-obe.outbound.protection.outlook.com (mail-mw2nam12olkn2010.outbound.protection.outlook.com [40.92.23.10]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id DB15068AF97 for ; Sun, 8 May 2022 05:27:49 +0300 (EEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Hma0pnaLhsiizb21zrv0m+Ie5GJbO36REAJDVlsn/t3oSUpxg8bx/R4TePCP5isXRzKE1IWjKtNYTiHA8JeyitI1h8BCyTbsWpSknZvVAjq6Rbk+TQEnUcusx6khVNb5/+Sy34Cn3Q+O22bNv+o4BbmG3S1gtzl65C2f6DG7IVTEkxGTYjVZZAjFIQEHFZIj+8LJG3L/EWWuiVu0Mplt/BukHPMcyr9MUpopDdrS2ifm1E/BeFw8QhgEslzXeAAA2ik/LFnPZStrRst62o4Or/AgL6MihVHJla1mvB7n2RXHEd27K1OahK6TqGuGv5LQ/jy5v9oWcHR6yuh7ym1d1g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=0tr+CNdIojXkd1DOGDaWybZ78/U2KXBxm8SRaptDAGM=; b=Oolkcuxf2/qkubo4soyQy0gfbbsHYEbdjHtrjb+QTljnes6P/rDRBFBVj1w+8qpqZ67uzcJtvntOS/Vt1zXB/RlvK/w6vJQ5gLIrbp2uTH0pR7WibbeQZ2HVtTp5ky313R0oWma5pJQdpxcfgWJpURhLeFbvb+p63z7FOI69DuG+vS/S9lv8mzVtGZkaGy+N3N4/Ms/TOM5xePG+LjzlTfV4f/zI01pkS5Qgc9M+7aoP55yd0LIcyRZ4KmPppuweyY6JfQmaJe4ooa0Cu1G9cBs9sEycS6Zhcz5rh4B1QwAYeHz8Z27wMncj/A0JfOsVaNGl+ko4WkCiHNSqVJxxcQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=hotmail.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=0tr+CNdIojXkd1DOGDaWybZ78/U2KXBxm8SRaptDAGM=; b=jtH+61on/H2hq3joVolyUg6uawSS3KoO/+dUE7j9hfKvdxpRGptwQluZLVenNeR1oJS/fVcgPnoLqIn6V5u++i2eXpyt/jEq0YEzE51WAkPnGR0rHVomqc7k66J2p+vM18CRC44d4e6D6G/ecgVZoQOq2DRdfD2W1rlfOj+WS4t0MOuziGTKM212ixMlY1IXGbmlb1ljRwlXoBPnFFYx4H8JCblFwiTA+HV+YbplgK/ah1x08MZLxW9lNvqS6crGShbou/FzSBysy2a9Y86+j+3EYA26QzcI+M/fEJn19iKOvbq7d4crHhq+NZF+CAkFaLGqtvdN8NLB1eES5TYDPg== Received: from DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM (2603:10b6:8:b::20) by BL3P223MB0212.NAMP223.PROD.OUTLOOK.COM (2603:10b6:208:34e::12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5227.20; Sun, 8 May 2022 02:27:46 +0000 Received: from DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM ([fe80::7472:6f83:eeb:45e3]) by DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM ([fe80::7472:6f83:eeb:45e3%9]) with mapi id 15.20.5227.022; Sun, 8 May 2022 02:27:46 +0000 From: Soft Works To: FFmpeg development discussions and patches Thread-Topic: [FFmpeg-devel] [PATCH v2 01/11] libavformat/asf: fix handling of byte array length values Thread-Index: AQHYYfX5GQMJdqeAXEqTFZuwdbGY1q0Twi6AgABan0A= Date: Sun, 8 May 2022 02:27:46 +0000 Message-ID: References: <0056a93a347829e72cd6d09d48062978ca4ac6e0.1651916204.git.ffmpegagent@gmail.com> <20220507184833.GI396728@pb2> In-Reply-To: <20220507184833.GI396728@pb2> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-tmn: [DPpvqNCvmQdxxqW0BYvbiCBbkgMzGLxp] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 2cb18d06-11e1-461f-4a1e-08da309a5683 x-ms-traffictypediagnostic: BL3P223MB0212:EE_ x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: niABjBXJAkfp2Yedw2htUG1qxBeZcG8rMzgx8WMhvBFfurKpSrEn+xeHa2hwm9IXaAv3SxNOOdoo0czDpDcBUxc0nyGoTEM+aVMXkQ7qH5hxdVTjSpB5RxaEsocYbDkMCb+Tjhxk/7wqhnKFoZmEMuRfSbOeUbcQ+qKs3O6Y/8PIoquglryiwYd8HLKK+9/wPDN8QvDnyTiCo9gy3s0PSdzAtte+oqE/hiQ7v2wdjaJGRS1qkTGGvpljqlglnCDCaOpIiXohJma3wNIzMsciPJKoiVi/rtnUgr0Op2xkDsTpCFDgZCdsDJrni5iHn48nFNC2OhpicLETN1WiPQvUHrJdjlZuKNlRq2kBB01O/Mq3oAgFo38eSqF0GdlzngwBVYgyyG1iB2WohlT5gevZ2LqvQLklK0y+7MHtJh4soSdkL9S+toZGoRvGH8S7dBdtYL19KjigEeSsyrmLM9aELTrwnnzg+ZU9Il+tDTb5shtTYFzD9x1oeJSFQSWQx80t3XAvN6cpsW2VcN99/Gmp+ZZzr4iRcwwyqlN3U+ZZJSIFXelfHmmWBsG3YNLmMfQu562Mr8yu6SkRW9HXt9yf+HmOM8fKSrXPgzkxjSW2urLbTsRVqKC0SL+Bs9Oh7k7Vr/fglDwFEaLWlBjlgA3aAg== x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?utf-8?B?U2JYSC9tYVhlM05CQmxjcTRnbG9RNzZXak5WVHRzT3FQaFVPditBYjdkc3hs?= =?utf-8?B?YW5GUEtTaDdNZVYyVTQrODhzNXNPYStuU1hTaGNOckZxT1pvK0NlL3cyZmVB?= =?utf-8?B?ejEyWkk3ZHNsbitwZXRoZ1BzdFR0bkNId0EyREZHbFk4eDZHMUdxLzZuV2tt?= =?utf-8?B?RlFycmV0Y1l6Y2pzV0Y2WEJtZnFrL2hIamdWZEtNU0JoK0lKWS8yTkN0OWp3?= =?utf-8?B?TWduZ1FOeWFxdzB4UGFTUEJod1llV2lXSHovNVJFZE0vbUVwR1M3VmZ1L2Qw?= =?utf-8?B?KzRCUlZHMXpUbWptbS82Z1hYUFRpS0pObkR0eldZWEhwREp3SVBwYjBRMi94?= =?utf-8?B?cHhjOHQ3T0xMZ3dvbFhxZUdVTjQ1OWs2V0M1dDlZV3J0V0g4aFM2ekFCcFFK?= =?utf-8?B?Q1BqSDl6Mkp3dXlseVFzU3dLS3BITTNOYStMaXNhK1FKRi8zcWdFeW9JOWxm?= =?utf-8?B?QnR2WnppSDVLc3BJYU1QbGtuVWhkTUZlNVEvY3FobkVuRlNmS0FXZ05TcDlP?= =?utf-8?B?cjRRTDNDKzkrRUllTXlWdm95UzRGNWdiTnAvMFZTOVhSYVpzN3l4RDVXVFgx?= =?utf-8?B?cW0rNHJtN2VQK2ZNbjB6V01yOW92NEV1YWMrMWJCb2JyV1hrZXIzYVN4ZXVh?= =?utf-8?B?UC9TbzNCTlEwWUYrbCt1VERkdVM5UFFGYkQ0V0UrNkcyb2o5L1ptSzA3ckxp?= =?utf-8?B?Tk5Ubm9LVDQzNHdoeVVSUGdERGt5OUNMZWNTam9sdlQ4UzBKbjI1YTJJRXBr?= =?utf-8?B?UkxrTmZmZU5rTjBONWdTZEFGemxNaHVwaTNDSmhtMGMrTm1aTVRZcnhHSDM3?= =?utf-8?B?VGVGVXZSMVZiaktTMG1qbTdxY2hRUW44NjlLSVJYK2N4djU2dnpId0Jnbmpt?= =?utf-8?B?azY2RWplMUdneWVvTUZDZThRdUVvZjJLYWQydEZleXR3UGlNaVRUZEREYjVH?= =?utf-8?B?dzNYNE9OQWhaUC9nbTcrNXVaZjRhUXl2VElXOEtKcmxQbHpYdmI2WFRuQ0NY?= =?utf-8?B?MW4xdzhhUmlSeVJ0YUxJU2RUMVJTcCtvcnFyck12a3hBVDQvZU92QUYyWXZi?= =?utf-8?B?RTYvQXBXV1VNZG5QUTk0RndCSS8rMHAzV29UeEU1eGgzRHhMU3FxZ0h2ZVZ3?= =?utf-8?B?Sm5zRGo0ZzFQMW9rTTd1NHBGcHQrZk5qQUZpck1OSkxPMVRSNWFrQXU5NXBo?= =?utf-8?B?OHB3cnVvTkNteXFJK2NXMW5xUjd1bmtYLzhONldkWjZXakVxWmJLMHlyQk92?= =?utf-8?B?TzdFVDRkNHJtQVp2aVQ2YlhMTjVPRWY1OXc5Y2F4M1ZKUGlMN3ZJaUo1bktZ?= =?utf-8?B?aFZMYnZWVGlWNW9pK2IyWE11Z3l1UTV0cUVpTytRTlFUcytDYUxJYy9DbTVu?= =?utf-8?B?ZG1peHF3OUdzVy9GQUxnOUIyZ0NwRGhxeTJ1NHpPT0dlK0dqZW9sY0VBNTJn?= =?utf-8?B?dEo4cE1iYjRadXBRUEpnclhaTW96eWdCZXUyZm1VUG1yYkM1dkJicDh2Q3Nh?= =?utf-8?B?MUdSSnpJMTVPRytwbCt5ajZIN0VJWDl0VCtVSGhOTlkzMkZKWUd1RUpmallJ?= =?utf-8?B?bG1sQlZzN3Vod0lJVTRKUDlGMkJpZTRwR0ZLbWJLTjVVcnZzbmF4R3lKdzR5?= =?utf-8?B?djA0TXl4QUhlS2V4cFQ0VGhKWmhSdGQrN1hqdFc1cnczTkQyWmtpVzFQRjBx?= =?utf-8?B?VXJRNzlOa2lqMkpmcmNRVWJ6NGRnZ3BaMm1HU0dVbkt2N1F1SklMdFBhaG02?= =?utf-8?B?cnhFeW9SL0FkeVhlSTI5cW1KdVhpTnRtRDlGU3RpN2FiYUVGS3FDQ3ZUd2xJ?= =?utf-8?B?bUlvRGg1TGRMaURrVUVwc0RsYVUxbXZxaEtYS2Vpa2hqOVBjdHVNOUpQdkVh?= =?utf-8?B?V3NvcWFVVmlkOTEvdnA5bmZjaTJWdWhLTU9FQnBmbUMwT29rVVJFTTJQeTVL?= =?utf-8?Q?JBdkVJy784Q=3D?= MIME-Version: 1.0 X-OriginatorOrg: sct-15-20-4755-11-msonline-outlook-1ff67.templateTenant X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-CrossTenant-Network-Message-Id: 2cb18d06-11e1-461f-4a1e-08da309a5683 X-MS-Exchange-CrossTenant-originalarrivaltime: 08 May 2022 02:27:46.0419 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3P223MB0212 Subject: Re: [FFmpeg-devel] [PATCH v2 01/11] libavformat/asf: fix handling of byte array length values 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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: > -----Original Message----- > From: ffmpeg-devel On Behalf Of > Michael Niedermayer > Sent: Saturday, May 7, 2022 8:49 PM > To: FFmpeg development discussions and patches devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v2 01/11] libavformat/asf: fix > handling of byte array length values > > On Sat, May 07, 2022 at 09:36:34AM +0000, softworkz wrote: > > From: softworkz > > > > The spec allows attachment sizes of up to UINT32_MAX while > > we can handle only sizes up to INT32_MAX (in downstream > > code) > > > > The debug.assert in get_tag didn't really address this, > > and truncating the value_len in calling methods cannot > > be used because the length value is required in order to > > continue parsing. This adds a check with log message in > > ff_asf_handle_byte_array to handle those (rare) cases. > > > > Signed-off-by: softworkz > > --- > > libavformat/asf.c | 12 +++++++++--- > > libavformat/asf.h | 2 +- > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/libavformat/asf.c b/libavformat/asf.c > > index 1ac8b5f078..179b66a2b4 100644 > > --- a/libavformat/asf.c > > +++ b/libavformat/asf.c > > @@ -267,12 +267,18 @@ static int get_id3_tag(AVFormatContext *s, int > len) > > } > > > > int ff_asf_handle_byte_array(AVFormatContext *s, const char *name, > > - int val_len) > > + uint32_t val_len) > > { > > + if (val_len > INT32_MAX) { > > + av_log(s, AV_LOG_VERBOSE, "Unable to handle byte arrays > > INT32_MAX in tag %s.\n", name); > > + return 1; > > + } > > + > > > if (!strcmp(name, "WM/Picture")) // handle cover art > > - return asf_read_picture(s, val_len); > > + return asf_read_picture(s, (int)val_len); > > else if (!strcmp(name, "ID3")) // handle ID3 tag > > - return get_id3_tag(s, val_len); > > + return get_id3_tag(s, (int)val_len); > > unneeded Hi Michael, thanks a lot for reviewing! I think we had talked about this a while ago. From my point of view, that explicit cast to int, tells me, every other reader of the code as well as any static analysis or linting tools that the developer has been aware of the data type mismatch between supplied variable and the parameter type and that the conversion is intended rather than accidental. But I don't want to insist - I have removed it. > > > > + av_log(s, AV_LOG_VERBOSE, "Unsupported byte array in tag > %s.\n", name); > > Probably this should be DEBUG The problem with DEBUG is that some components are spitting out so many lines with log level DEBUG that you'd hardly ever see it unless you'd explicitly search for that exact line, while that line is rather of the kind that you don't expect it. I've changed it to DEBUG, though. > > if (!strcmp(name, "AspectRatioX")){ > > - int aspect_x = get_value(s->pb, value_type, 16); > > + const uint64_t aspect_x = get_value(s->pb, value_type, > 16); > > + if (aspect_x > INT32_MAX) { > > + av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioX > value: %"PRIu64"\n", aspect_x); > > + return AVERROR(ENOTSUP); > > + } > > if(stream_num < 128) > > - asf->dar[stream_num].num = aspect_x; > > + asf->dar[stream_num].num = (int)aspect_x; > > } else if(!strcmp(name, "AspectRatioY")){ > > - int aspect_y = get_value(s->pb, value_type, 16); > > + const uint64_t aspect_y = get_value(s->pb, value_type, > 16); > > + if (aspect_y > INT32_MAX) { > > + av_log(s, AV_LOG_DEBUG, "Unsupported AspectRatioY > value: %"PRIu64"\n", aspect_y); > > + return AVERROR(ENOTSUP); > > + } > > if(stream_num < 128) > > - asf->dar[stream_num].den = aspect_y; > > + asf->dar[stream_num].den = (int)aspect_y; > > } else { > > If you go to the length to do something with oddly huge aspect > components > maybe change dar to 2 uint64_t and check it in one place instead of 2 > also the av_reduce() can handle a wider range than int32 Good idea, didn't know that av_reduce() can take larger numbers. Done. Thanks again, softworkz _______________________________________________ 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".