From patchwork Wed Apr 29 22:21:55 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 19387 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 BEF0744B57C for ; Thu, 30 Apr 2020 01:22:37 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A8FC668C248; Thu, 30 Apr 2020 01:22:37 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 7C09E68C1EB for ; Thu, 30 Apr 2020 01:22:29 +0300 (EEST) Received: by mail-wm1-f65.google.com with SMTP id r26so3810302wmh.0 for ; Wed, 29 Apr 2020 15:22:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=V6iIP84atg9BOwxbmtshMptzIPzxjAB0e6P+TeyQqZg=; b=qV2MPTRsgmU7QIX86uh6/UHyTokpQHd6f3wpU0pfe/pT/ClpYEeqbDT/9P64Hachw3 +O90JUs0gFEOVWrzpKVjF/+XvJfpjXPtcqo8CA5xT3RSPhGK8GSUZUAA8vVdEP+3SX9i +6lAF4AMOSEJlK++XEUsGz2YtDyUmS+QYTTseyKpmWfRnNvOZqLpSqXQkveAd/Emkgrr WIAtZzM8cJlSzAp9F/9LVSQYxtRZxglxIjtK1AadEGQOp/kUE2lIMT+leBPQjeBJcpTB /BDXHBxEMHbLt8liGx6Qw1e8wUBB9UixPYdm5r7xDOfrAlBmhy5mcRkScdMUAdMJ4Y2l ZWKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=V6iIP84atg9BOwxbmtshMptzIPzxjAB0e6P+TeyQqZg=; b=BPe74ec/9jpUt6DJqeB195eWI7qrr4UA/GgfIcgmIrz97xWJgeflyW1EyLs7eVSJVG 2peYnjHVn+vbqo9y8x/FP2BcnR/mQSfPzpd0Q4NEtkd2s710/4S2bom+p03Z2XQZlgMx cYOQIcUN03c7Yt8N3kUen9jPqpOHFH92Yfyc0ZKkSJrNbybJnW/cxK97OIKeBCZZjiNJ ggjamWt3ZKPjxYaL5yk681Wb5YZ8CtZSItTRww+zEs3SQBc6hskreL9yxEBDVspcW6eV qEdjW8dEWMjGMq5jIFXpkfgVf4kZMXeix3s3/As8o3kUqpnCr5KwncestDejoPLQ+RTi Gn3Q== X-Gm-Message-State: AGi0PuZN5+VVKTx0eiUTc3uVomN9Yd8ix2eJeeCeB7SiRUq6CC50r0ht BUpuVIvDMFY3pS6axU60DlhZFYBa X-Google-Smtp-Source: APiQypJqEX1h5g19i1g2I2ODIfMNbHcs5NAQaUBcE9/anZV/r1E2WHBPnAVCJn5wRIFiRaTzafRQBQ== X-Received: by 2002:a05:600c:24cf:: with SMTP id 15mr58976wmu.94.1588198948223; Wed, 29 Apr 2020 15:22:28 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc1ab57.dynamic.kabel-deutschland.de. [188.193.171.87]) by smtp.gmail.com with ESMTPSA id x6sm1071555wrg.58.2020.04.29.15.22.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 29 Apr 2020 15:22:27 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Thu, 30 Apr 2020 00:21:55 +0200 Message-Id: <20200429222156.29129-5-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200429222156.29129-1-andreas.rheinhardt@gmail.com> References: <20200429222156.29129-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 5/6] avformat/matroskaenc: Check allocations implicit in dynamic buffers 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" Failures of the allocations that happen under the hood when using dynamic buffers are usually completely unchecked and the Matroska muxer is no exception to this. The API has its part in this, because there is no documented way to actually check for errors: The return value of both avio_get_dyn_buf() as well as avio_close_dyn_buf() is only documented as "the length of the byte buffer", so that using this to return errors would be an API break. Therefore this commit uses the only reliable way to check for errors with avio_get_dyn_buf(): The AVIOContext's error flag. (This is one of the advantages of avio_get_dyn_buf(): By not destroying the AVIOContext it is possible to inspect this value.) Checking whether the size or the pointer vanishes is not enough as it does not check for truncated output (the dynamic buffer API is int based and so has to truncate the buffer even when enough memory would be available; it's current actual limit is even way below INT_MAX). Signed-off-by: Andreas Rheinhardt --- libavformat/matroskaenc.c | 113 ++++++++++++++++++++++++++------------ 1 file changed, 77 insertions(+), 36 deletions(-) diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index e671a572a8..12c22184a3 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -375,19 +375,22 @@ static int start_ebml_master_crc32(AVIOContext **dyn_cp, MatroskaMuxContext *mkv return 0; } -static void end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, +static int end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, MatroskaMuxContext *mkv, uint32_t id, int length_size, int keep_buffer, int add_seekentry) { uint8_t *buf, crc[4]; - int size, skip = 0; + int ret, size, skip = 0; + + size = avio_get_dyn_buf(*dyn_cp, &buf); + if ((ret = (*dyn_cp)->error) < 0) + goto fail; if (add_seekentry) mkv_add_seekhead_entry(mkv, id, avio_tell(pb)); put_ebml_id(pb, id); - size = avio_get_dyn_buf(*dyn_cp, &buf); put_ebml_length(pb, size, length_size); if (mkv->write_crc) { skip = 6; /* Skip reserved 6-byte long void element from the dynamic buffer. */ @@ -396,18 +399,20 @@ static void end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, } avio_write(pb, buf + skip, size - skip); +fail: if (keep_buffer) { ffio_reset_dyn_buf(*dyn_cp); } else { ffio_free_dyn_buf(dyn_cp); } + return ret; } /** * Output EBML master. Keep the buffer if seekable, allowing for later updates. * Furthermore always add a SeekHead Entry for this element. */ -static void end_ebml_master_crc32_tentatively(AVIOContext *pb, +static int end_ebml_master_crc32_tentatively(AVIOContext *pb, ebml_stored_master *elem, MatroskaMuxContext *mkv, uint32_t id) { @@ -415,14 +420,19 @@ static void end_ebml_master_crc32_tentatively(AVIOContext *pb, uint8_t *buf; int size = avio_get_dyn_buf(elem->bc, &buf); + if (elem->bc->error < 0) + return elem->bc->error; + elem->pos = avio_tell(pb); mkv_add_seekhead_entry(mkv, id, elem->pos); put_ebml_id(pb, id); put_ebml_length(pb, size, 0); avio_write(pb, buf, size); + + return 0; } else - end_ebml_master_crc32(pb, &elem->bc, mkv, id, 0, 0, 1); + return end_ebml_master_crc32(pb, &elem->bc, mkv, id, 0, 0, 1); } static void put_xiph_size(AVIOContext *pb, int size) @@ -501,7 +511,10 @@ static int mkv_write_seekhead(AVIOContext *pb, MatroskaMuxContext *mkv, put_ebml_uint(dyn_cp, MATROSKA_ID_SEEKPOSITION, entry->segmentpos); end_ebml_master(dyn_cp, seekentry); } - end_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_SEEKHEAD, 0, 0, 0); + ret = end_ebml_master_crc32(pb, &dyn_cp, mkv, + MATROSKA_ID_SEEKHEAD, 0, 0, 0); + if (ret < 0) + return ret; remaining = seekhead->filepos + seekhead->reserved_size - avio_tell(pb); put_ebml_void(pb, remaining); @@ -574,12 +587,14 @@ static int mkv_assemble_cues(AVStream **streams, AVIOContext *dyn_cp, end_ebml_master(cuepoint, track_positions); } while (++entry < end && entry->pts == pts); size = avio_get_dyn_buf(cuepoint, &buf); + if ((ret = cuepoint->error) < 0) + break; put_ebml_binary(dyn_cp, MATROSKA_ID_POINTENTRY, buf, size); ffio_reset_dyn_buf(cuepoint); } ffio_free_dyn_buf(&cuepoint); - return 0; + return ret; } static int put_xiph_codecpriv(AVFormatContext *s, AVIOContext *pb, @@ -800,10 +815,12 @@ static int mkv_write_codecprivate(AVFormatContext *s, AVIOContext *pb, ff_put_wav_header(s, dyn_cp, par, FF_PUT_WAV_HEADER_FORCE_WAVEFORMATEX); } + if (ret >= 0) { codecpriv_size = avio_get_dyn_buf(dyn_cp, &codecpriv); - if (codecpriv_size) + if ((ret = dyn_cp->error) >= 0 && codecpriv_size) put_ebml_binary(pb, MATROSKA_ID_CODECPRIVATE, codecpriv, codecpriv_size); + } ffio_free_dyn_buf(&dyn_cp); return ret; } @@ -1423,10 +1440,8 @@ static int mkv_write_tracks(AVFormatContext *s) return ret; } - end_ebml_master_crc32_tentatively(pb, &mkv->track, mkv, + return end_ebml_master_crc32_tentatively(pb, &mkv->track, mkv, MATROSKA_ID_TRACKS); - - return 0; } static int mkv_write_chapters(AVFormatContext *s) @@ -1482,10 +1497,10 @@ static int mkv_write_chapters(AVFormatContext *s) end_ebml_master(dyn_cp, chapteratom); } end_ebml_master(dyn_cp, editionentry); - end_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_CHAPTERS, 0, 0, 1); - mkv->wrote_chapters = 1; - return 0; + + return end_ebml_master_crc32(pb, &dyn_cp, mkv, + MATROSKA_ID_CHAPTERS, 0, 0, 1); } static int mkv_write_simpletag(AVIOContext *pb, const AVDictionaryEntry *t) @@ -1677,7 +1692,7 @@ static int mkv_write_tags(AVFormatContext *s) } if (mkv->tags.bc) { - end_ebml_master_crc32_tentatively(s->pb, &mkv->tags, mkv, + return end_ebml_master_crc32_tentatively(s->pb, &mkv->tags, mkv, MATROSKA_ID_TAGS); } return 0; @@ -1740,9 +1755,8 @@ static int mkv_write_attachments(AVFormatContext *s) put_ebml_uid(dyn_cp, MATROSKA_ID_FILEUID, track->uid); end_ebml_master(dyn_cp, attached_file); } - end_ebml_master_crc32(pb, &dyn_cp, mkv, MATROSKA_ID_ATTACHMENTS, 0, 0, 1); - - return 0; + return end_ebml_master_crc32(pb, &dyn_cp, mkv, + MATROSKA_ID_ATTACHMENTS, 0, 0, 1); } static int64_t get_metadata_duration(AVFormatContext *s) @@ -1856,7 +1870,10 @@ static int mkv_write_header(AVFormatContext *s) put_ebml_void(pb, 11); // assumes double-precision float to be written } } - end_ebml_master_crc32_tentatively(s->pb, &mkv->info, mkv, MATROSKA_ID_INFO); + ret = end_ebml_master_crc32_tentatively(s->pb, &mkv->info, + mkv, MATROSKA_ID_INFO); + if (ret < 0) + return ret; pb = s->pb; ret = mkv_write_tracks(s); @@ -2137,18 +2154,23 @@ static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, const AVPac return pkt->duration; } -static void mkv_end_cluster(AVFormatContext *s) +static int mkv_end_cluster(AVFormatContext *s) { MatroskaMuxContext *mkv = s->priv_data; + int ret; - end_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv, - MATROSKA_ID_CLUSTER, 0, 1, 0); if (!mkv->have_video) { for (unsigned i = 0; i < s->nb_streams; i++) mkv->tracks[i].has_cue = 0; } mkv->cluster_pos = -1; + ret = end_ebml_master_crc32(s->pb, &mkv->cluster_bc, mkv, + MATROSKA_ID_CLUSTER, 0, 1, 0); + if (ret < 0) + return ret; + avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_FLUSH_POINT); + return 0; } static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt) @@ -2216,15 +2238,16 @@ static int mkv_check_new_extra_data(AVFormatContext *s, const AVPacket *pkt) if (ret < 0) return ret; ff_isom_write_av1c(dyn_cp, side_data, side_data_size); - codecpriv_size = avio_close_dyn_buf(dyn_cp, &codecpriv); - if (!codecpriv_size) { - av_free(codecpriv); - return AVERROR_INVALIDDATA; + codecpriv_size = avio_get_dyn_buf(dyn_cp, &codecpriv); + if ((ret = dyn_cp->error) < 0 || + !codecpriv_size && (ret = AVERROR_INVALIDDATA)) { + ffio_free_dyn_buf(&dyn_cp); + return ret; } avio_seek(mkv->track.bc, track->codecpriv_offset, SEEK_SET); // Do not write the OBUs as we don't have space saved for them put_ebml_binary(mkv->track.bc, MATROSKA_ID_CODECPRIVATE, codecpriv, 4); - av_free(codecpriv); + ffio_free_dyn_buf(&dyn_cp); ret = ff_alloc_extradata(par, side_data_size); if (ret < 0) return ret; @@ -2262,7 +2285,9 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt) if (mkv->cluster_pos != -1) { int64_t cluster_time = ts - mkv->cluster_pts; if ((int16_t)cluster_time != cluster_time) { - mkv_end_cluster(s); + ret = mkv_end_cluster(s); + if (ret < 0) + return ret; av_log(s, AV_LOG_WARNING, "Starting new cluster due to timestamp\n"); } } @@ -2372,8 +2397,11 @@ static int mkv_write_packet(AVFormatContext *s, const AVPacket *pkt) } else start_new_cluster = 0; - if (start_new_cluster) - mkv_end_cluster(s); + if (start_new_cluster) { + ret = mkv_end_cluster(s); + if (ret < 0) + return ret; + } } if (!mkv->cluster_pos) @@ -2408,7 +2436,9 @@ static int mkv_write_flush_packet(AVFormatContext *s, AVPacket *pkt) if (!pkt) { if (mkv->cluster_pos != -1) { - mkv_end_cluster(s); + int ret = mkv_end_cluster(s); + if (ret < 0) + return ret; av_log(s, AV_LOG_DEBUG, "Flushing cluster at offset %" PRIu64 " bytes\n", avio_tell(s->pb)); @@ -2435,8 +2465,10 @@ static int mkv_write_trailer(AVFormatContext *s) } if (mkv->cluster_bc) { - end_ebml_master_crc32(pb, &mkv->cluster_bc, mkv, + ret = end_ebml_master_crc32(pb, &mkv->cluster_bc, mkv, MATROSKA_ID_CLUSTER, 0, 0, 0); + if (ret < 0) + return ret; } ret = mkv_write_chapters(s); @@ -2493,8 +2525,10 @@ static int mkv_write_trailer(AVFormatContext *s) } } } - end_ebml_master_crc32(pb, &cues, mkv, MATROSKA_ID_CUES, + ret = end_ebml_master_crc32(pb, &cues, mkv, MATROSKA_ID_CUES, length_size, 0, 1); + if (ret < 0) + return ret; if (mkv->reserve_cues_space) { if (size < mkv->reserve_cues_space) put_ebml_void(pb, mkv->reserve_cues_space - size); @@ -2511,13 +2545,18 @@ static int mkv_write_trailer(AVFormatContext *s) av_log(s, AV_LOG_DEBUG, "end duration = %" PRIu64 "\n", mkv->duration); avio_seek(mkv->info.bc, mkv->duration_offset, SEEK_SET); put_ebml_float(mkv->info.bc, MATROSKA_ID_DURATION, mkv->duration); - end_ebml_master_crc32(pb, &mkv->info.bc, mkv, MATROSKA_ID_INFO, 0, 0, 0); + ret = end_ebml_master_crc32(pb, &mkv->info.bc, mkv, + MATROSKA_ID_INFO, 0, 0, 0); + if (ret < 0) + return ret; if (mkv->track.bc) { // write Tracks master avio_seek(pb, mkv->track.pos, SEEK_SET); - end_ebml_master_crc32(pb, &mkv->track.bc, mkv, + ret = end_ebml_master_crc32(pb, &mkv->track.bc, mkv, MATROSKA_ID_TRACKS, 0, 0, 0); + if (ret < 0) + return ret; } // update stream durations @@ -2545,8 +2584,10 @@ static int mkv_write_trailer(AVFormatContext *s) } avio_seek(pb, mkv->tags.pos, SEEK_SET); - end_ebml_master_crc32(pb, &mkv->tags.bc, mkv, + ret = end_ebml_master_crc32(pb, &mkv->tags.bc, mkv, MATROSKA_ID_TAGS, 0, 0, 0); + if (ret < 0) + return ret; } avio_seek(pb, endpos, SEEK_SET);