From patchwork Mon Apr 5 01:44:33 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 26744 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id B5D7344982B for ; Mon, 5 Apr 2021 04:45:42 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 9FACD6880A8; Mon, 5 Apr 2021 04:45:42 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from EUR06-VI1-obe.outbound.protection.outlook.com (mail-vi1eur06olkn2092.outbound.protection.outlook.com [40.92.17.92]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id AB697680946 for ; Mon, 5 Apr 2021 04:45:35 +0300 (EEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=DLHkGhDsvSV5g3rXsduo1V4UendyHHwf5gKEvCJbtWDsSb2XXBrm1tD7Y4tVSCUpvIIOYhqGVAq+bEPBo4B7wrMWsgwC8OfwsveFFcsCrPRVNekCALamzGzX2WKafC3o2YRu1xn63/vBQPwj9H+xOZQQ/Hriul/6gYSDmAfv2YhKjtnuvYZofBeICIqIcUmob8p7zLCeHcI5I9FTJXlj+MuefDkuoZ4ZCJ63PWxKObPMAtw826wwYtqEjLWcEfFJb+sc88lOTt7Vvqe3jJdXrYtBxD34QYGyn0bOL1oun8DYN7olmju7i29G1Th1mFOpRCAD+Ofd58FrVYr1x0qw5w== 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-SenderADCheck; bh=CaFoSkCMzI+fLhneTk0lqGBQL7DpoeyO/XTkdiGZQc8=; b=fG2bzNd9z+vsOXJLVjGFd6sLQRZtFDxPOFqinbIzMlqPGH7m9eJ4ljr+AYrk3KMsmn8K/Uk++pFSkRHzAZrI58jBzIID70HqVUhplAkTJTruSzmpOTnAQSnGKC4GFpq6ZByIx/5GDjusppzwK32Xb+nrKk2fWKiWdWHcBvXCinm+4jL9pchVb1ufR+eN1w3exLsHAXrJ6DrhiVE5EWg8IWcp1Ga4Ef65aqzoYh+/5LUYg95kq5YxsrAx7UM9FS5tuzerFzfF6RtK04cd0xCmgAiuVTo9yJCMjZrs47dCYc5vUDueCzwyARNce2GL+VMUrxZMZUf/fPmLBfcnnAkPfw== 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=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=CaFoSkCMzI+fLhneTk0lqGBQL7DpoeyO/XTkdiGZQc8=; b=rQBLqV5P+G4I6A/a8CExgs0K1GINSZTJMaahVzBNS+3C+v854Ptzsf6sLe1EfbfJ2yy38Tt2OQdb5p3QoKuAEC9oMCyTOJdxTCm6Y86VYcwCUE2skf2TjJv8FNHkj/K16qX60SKQ6ZDBZn15XQ3Mze7oZyIj0JTX1QFDrDmKQC080j7r+rmgxl/1rWfc67rZtnJlsqI/XhSdorNuPIalfKM/gaxGoDQj8CnSfAU2LflUSzzfx6qoD7PRbo9fXec4F2bYZfR1yE+j5XjugXRRvdmkl93AIuTvy2JXvdSWmvI6FxLDckJPzt3pWT8dJ9sVZy2Erd6OKZKu2bacwpG+AA== Received: from AM7EUR06FT063.eop-eur06.prod.protection.outlook.com (2a01:111:e400:fc36::51) by AM7EUR06HT210.eop-eur06.prod.protection.outlook.com (2a01:111:e400:fc36::476) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3999.28; Mon, 5 Apr 2021 01:45:35 +0000 Received: from HE1PR0301MB2154.eurprd03.prod.outlook.com (2a01:111:e400:fc36::52) by AM7EUR06FT063.mail.protection.outlook.com (2a01:111:e400:fc36::403) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3999.28 via Frontend Transport; Mon, 5 Apr 2021 01:45:35 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:E00E218BDE35C2FDA11FFB5C32E9F3A0FE5D7CFA5DFF5B19788F58A043305578; UpperCasedChecksum:37EFA4220534A571948BDBD6A87062066ADED31EA0CDA4B16CCCDF5E8BCB261C; SizeAsReceived:7608; Count:48 Received: from HE1PR0301MB2154.eurprd03.prod.outlook.com ([fe80::8128:5de5:4e94:9a21]) by HE1PR0301MB2154.eurprd03.prod.outlook.com ([fe80::8128:5de5:4e94:9a21%3]) with mapi id 15.20.3999.032; Mon, 5 Apr 2021 01:45:35 +0000 From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Mon, 5 Apr 2021 03:44:33 +0200 Message-ID: X-Mailer: git-send-email 2.27.0 In-Reply-To: References: X-TMN: [lMeVIt+JNzR2RdC1D2Mw+Lm3W8VptA45] X-ClientProxiedBy: AM0PR10CA0039.EURPRD10.PROD.OUTLOOK.COM (2603:10a6:20b:150::19) To HE1PR0301MB2154.eurprd03.prod.outlook.com (2603:10a6:3:2a::22) X-Microsoft-Original-Message-ID: <20210405014434.3973535-4-andreas.rheinhardt@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from sblaptop.fritz.box (188.192.137.96) by AM0PR10CA0039.EURPRD10.PROD.OUTLOOK.COM (2603:10a6:20b:150::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3999.26 via Frontend Transport; Mon, 5 Apr 2021 01:45:34 +0000 X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 48 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: 1423a9a4-eb98-4a44-9cde-08d8f7d48152 X-MS-Exchange-SLBlob-MailProps: gjx25WM8ZNUT6BysHa+24QDXCmt+/ofHIH4lvje3dxUDOsJiYL6lQV5xq+3rkg9xHa19Y6Gm57k78aO8s7uZHF4w9w9OTNcb4X0ZaBqOBe6/ud0WGTcJ4r2Lifry9CEfXwuQcBE2PgnpYUHvJcQYA8UezufVdsS1WeUksjVtcnAOKFRFTKnL0tn1NE4cA5ti3as7RT8HXroiI2/fdHK44t5vHzIHCs6zOZinvNzsrxopqh2tm1YSv++tNYgg+Oe01+Bg/ToZ+sdh42QH8lPxX/ayCD5xPjLP2OqzSdk+UMeA8v5SS3GSYlKOde0UHx5N6o7AWJIXupJj71Ol6xrG9QIiYIIs7+shwZT9i/qoj0l6gonmCyXZjafxwigWav2DjuJ0CQnCyvBt410ooLR718/k7yYBjcYWq4GAAqUVvMkdHXnLzolxV6APINoH6qHw9/o1gyjn51q+O8xoah9D+HRfq8z+NVQOgjEV2y1KCYK2bPnlJFufwbZP8yHgDrsJrJgSDZdEglRzcWYZwnsYYMQ8nSCeH5pmbAfhI22OSzydw2EVx3+Mdoyara4yW59w9vX4szx7TaRdXEn3ySd40CAivP0oj8Gh/dYNcPVvyWUrIrRDopWLvksEMq0U/xgZuKk7PvJmp0ic5sDgXJ3p+1SAepMOgCqEvIdfbC+ArQnS7zUknOOxkmRxH5+2qgHBMxsB6EsQ/dMeEOec1lMuq7j3ZgpgM/zsFC6txCDULBI= X-MS-TrafficTypeDiagnostic: AM7EUR06HT210: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: IyDOqCN1fopSk+WPxg8Lh0LyzSGriGnSFCgMmH67h5ZjOH903r5nJHnUCyLu+A4M8ZpeRVKEfdM8ooF/zvtB9uCFTENEEifxfaYrj9Rx7Qg2tcF/V00Rvg2pOOQadi3IpWOtiTUIDRUEP6ZeUwxhAWa40OmFcDX+o19+alG4NfK9kFvz8O4TQ3c6tEm2OlKXHS6wlAW9IO+vblvsC+xd5WPJYKraUtTcJDAf7/abCVYIIFEuRojSK25n6Z9FXKhVcxQcgzYlYI/X+4Eh+nl5bgEUc/bGffVCKKC0KYvCu8x8C3w1cJTJAYg+A/Qac/sHeXcumMBIK0Z3Ebd0HBjNc6zgpFCqoD0VpC0jeQsHB9eUxA5Pb8nuTOYSGMQcIisnb8+avIVyvz8dqya9JPYAbA== X-MS-Exchange-AntiSpam-MessageData: hGO9GM613gNhpT8T70E72xAJ9P2wm4HcT2ib+lYAIdsSPCf0v9HjFMR74/jOJPzGxXsPqVJGWuPOaLQMaT2v6o9DZiA7Nvh2KA4fQehu7NxxnZVaTRW1sRLDcXzT14Se6BHC16Xg0sb91nt96IIgdA== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1423a9a4-eb98-4a44-9cde-08d8f7d48152 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2021 01:45:34.9386 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-AuthSource: AM7EUR06FT063.eop-eur06.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: Internet X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7EUR06HT210 Subject: [FFmpeg-devel] [PATCH 5/6] avcodec/rv34, mpegvideo: Fix segfault upon frame size change error X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 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 Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" The RealVideo 3.0 and 4.0 decoders call ff_mpv_common_init() only during their init function and not during decode_frame(); when the size of the frame changes, they call ff_mpv_common_frame_size_change(). Yet upon error, said function calls ff_mpv_common_end() which frees the whole MpegEncContext and not only those parts that ff_mpv_common_frame_size_change() reinits. As a result, the context will never be usable again; worse, because decode_frame() contains no check for whether the context is initialized or not, it is presumed that it is initialized, leading to segfaults. Basically the same happens if rv34_decoder_realloc() fails. This commit fixes this by only resetting the parts that ff_mpv_common_frame_size_change() changes upon error and by actually checking whether the context is in need of reinitialization in ff_rv34_decode_frame(). Signed-off-by: Andreas Rheinhardt --- I actually don't like that we have two flags that indicate whether a MpegEncContext is usable or not; how about we always call ff_mpv_common_init() during init (and never lateron) and make it unconditionally allocate the stuff that does not depend upon resolution etc. and add a parameter to said function to also allocate the latter. The decode_frame functions would then be modified to always use ff_mpv_common_frame_size_change(). ff_rv34_decode_update_thread_context() currently checks twice whether the source MpegEncContext is initialized; the second is trivially true, because of the first check. But the first is also always true now, because ff_mpv_common_end() is only called when closing the decoder (and said check also made no sense before this patch because a noninitialized MpegEncContext would have led to a segfault pretty soon). Should this be changed to check for context_reinit instead? libavcodec/mpegvideo.c | 6 ++++-- libavcodec/rv34.c | 13 +++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c index 7eddbdcc37..5de0719f83 100644 --- a/libavcodec/mpegvideo.c +++ b/libavcodec/mpegvideo.c @@ -555,7 +555,6 @@ int ff_mpeg_update_thread_context(AVCodecContext *dst, } if (s->height != s1->height || s->width != s1->width || s->context_reinit) { - s->context_reinit = 0; s->height = s1->height; s->width = s1->width; if ((ret = ff_mpv_common_frame_size_change(s)) < 0) @@ -1099,10 +1098,12 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s) if (err < 0) goto fail; } + s->context_reinit = 0; return 0; fail: - ff_mpv_common_end(s); + free_context_frame(s); + s->context_reinit = 1; return err; } @@ -1149,6 +1150,7 @@ void ff_mpv_common_end(MpegEncContext *s) av_frame_free(&s->new_picture.f); s->context_initialized = 0; + s->context_reinit = 0; s->last_picture_ptr = s->next_picture_ptr = s->current_picture_ptr = NULL; diff --git a/libavcodec/rv34.c b/libavcodec/rv34.c index 7e5bfe0e22..99e580a09a 100644 --- a/libavcodec/rv34.c +++ b/libavcodec/rv34.c @@ -1383,6 +1383,7 @@ static int rv34_decoder_alloc(RV34DecContext *r) if (!(r->cbp_chroma && r->cbp_luma && r->deblock_coefs && r->intra_types_hist && r->mb_type)) { + r->s.context_reinit = 1; rv34_decoder_free(r); return AVERROR(ENOMEM); } @@ -1530,7 +1531,7 @@ int ff_rv34_decode_update_thread_context(AVCodecContext *dst, const AVCodecConte if (dst == src || !s1->context_initialized) return 0; - if (s->height != s1->height || s->width != s1->width) { + if (s->height != s1->height || s->width != s1->width || s->context_reinit) { s->height = s1->height; s->width = s1->width; if ((err = ff_mpv_common_frame_size_change(s)) < 0) @@ -1667,11 +1668,12 @@ int ff_rv34_decode_frame(AVCodecContext *avctx, if (s->mb_num_left > 0 && s->current_picture_ptr) { av_log(avctx, AV_LOG_ERROR, "New frame but still %d MB left.\n", s->mb_num_left); - ff_er_frame_end(&s->er); + if (!s->context_reinit) + ff_er_frame_end(&s->er); ff_mpv_frame_end(s); } - if (s->width != si.width || s->height != si.height) { + if (s->width != si.width || s->height != si.height || s->context_reinit) { int err; av_log(s->avctx, AV_LOG_WARNING, "Changing dimensions to %dx%d\n", @@ -1689,7 +1691,6 @@ int ff_rv34_decode_frame(AVCodecContext *avctx, err = ff_set_dimensions(s->avctx, s->width, s->height); if (err < 0) return err; - if ((err = ff_mpv_common_frame_size_change(s)) < 0) return err; if ((err = rv34_decoder_realloc(r)) < 0) @@ -1744,6 +1745,10 @@ int ff_rv34_decode_frame(AVCodecContext *avctx, } s->mb_x = s->mb_y = 0; ff_thread_finish_setup(s->avctx); + } else if (s->context_reinit) { + av_log(s->avctx, AV_LOG_ERROR, "Decoder needs full frames to " + "reinitialize (start MB is %d).\n", si.start); + return AVERROR_INVALIDDATA; } else if (HAVE_THREADS && (s->avctx->active_thread_type & FF_THREAD_FRAME)) { av_log(s->avctx, AV_LOG_ERROR, "Decoder needs full frames in frame "