On 2/8/2021 12:57 PM, Andreas Rheinhardt wrote:
James Almer:
On 2/8/2021 12:32 PM, Andreas Rheinhardt wrote:
James Almer:
On 2/8/2021 12:22 PM, Andreas Rheinhardt wrote:
James Almer:
Signed-off-by: James Almer <jamr...@gmail.com>
---
libavformat/matroskadec.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 374831baa3..9fad78c78b 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -365,6 +365,8 @@ typedef struct MatroskaDemuxContext {
/* byte position of the segment inside the stream */
int64_t segment_start;
+ AVPacket *pkt;
+
/* the packet queue */
AVPacketList *queue;
AVPacketList *queue_end;
@@ -2885,6 +2887,10 @@ static int
matroska_read_header(AVFormatContext *s)
}
ebml_free(ebml_syntax, &ebml);
+ matroska->pkt = av_packet_alloc();
+ if (!matroska->pkt)
Missing AVERROR(ENOMEM).
This seems to be a common mistake in this set. Sorry.
Yeah, the failure paths seem untested.
make fate doesn't test failure paths. I recall someone did some manual
runs limiting available memory long ago, which detected and removed a
lot of unchecked av_mallocs, but nothing automated.
Fuzzing has handled failure path issue detection since then.
No, fuzzing does not simulate low-memory environments (at least ossfuzz
as-is does not).
I know, which is why i said failure path issues, not OOM specifically.
Most failure paths it triggers are due to invalid data. But outside of
fuzzing, nothing tests any kind of failure path (Maybe static
analyzers?). All FATE tests do is ensure working scenarios keep working.
I think it errors out on OOM when one tries to allocate
a lot, but small stuff like AVPackets don't trigger this. I have a long
list of patches in libavcodec where an earlier allocation in an init
function would leak if a subsequent allocation fails; or even something
like 96061c5a4f690c3ab49e4458701bb013fd3dd57f where the close function
assumed that several allocations were successful. These bugs existed for
years and the fuzzer never found them.
This means that all these errors paths need to be manually tested, so
that something that adds error paths (like requiring things to be
allocated) adds a burden on devs (in addition to the runtime overhead).
(I actually have an idea to completely remove the packet list from
matroskadec.)
Should i commit this while you work on that, or just withdraw this
patch? It's not urgent at all, since any deprecation period is 2+ years.
Either option is fine by me.
I don't want to have deprecation warnings.
Ok.
+ goto fail;
+
/* The next thing is a segment. */
pos = avio_tell(matroska->ctx->pb);
res = ebml_parse(matroska, matroska_segments, matroska);
@@ -2947,7 +2953,7 @@ static int matroska_read_header(AVFormatContext
*s)
st->disposition |=
AV_DISPOSITION_ATTACHED_PIC;
st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
- av_init_packet(pkt);
+ av_packet_unref(pkt);
pkt->buf = attachments[j].bin.buf;
attachments[j].bin.buf = NULL;
pkt->data = attachments[j].bin.data;
@@ -3180,7 +3186,7 @@ static int
matroska_parse_rm_audio(MatroskaDemuxContext *matroska,
while (track->audio.pkt_cnt) {
int ret;
- AVPacket pktl, *pkt = &pktl;
+ AVPacket *pkt = matroska->pkt;
ret = av_new_packet(pkt, a);
if (ret < 0) {
@@ -3317,7 +3323,7 @@ static int
matroska_parse_webvtt(MatroskaDemuxContext *matroska,
uint64_t duration,
int64_t pos)
{
- AVPacket pktl, *pkt = &pktl;
+ AVPacket *pkt = matroska->pkt;
uint8_t *id, *settings, *text, *buf;
int id_len, settings_len, text_len;
uint8_t *p, *q;
@@ -3434,7 +3440,7 @@ static int
matroska_parse_frame(MatroskaDemuxContext *matroska,
{
uint8_t *pkt_data = data;
int res = 0;
- AVPacket pktl, *pkt = &pktl;
+ AVPacket *pkt = matroska->pkt;
if (st->codecpar->codec_id == AV_CODEC_ID_WAVPACK) {
res = matroska_parse_wavpack(track, &pkt_data, &pkt_size);
@@ -3464,7 +3470,7 @@ static int
matroska_parse_frame(MatroskaDemuxContext *matroska,
if (!pkt_size && !additional_size)
goto no_output;
- av_init_packet(pkt);
+ av_packet_unref(pkt);
if (!buf)
pkt->buf = av_buffer_create(pkt_data, pkt_size +
AV_INPUT_BUFFER_PADDING_SIZE,
NULL, NULL, 0);
@@ -3838,6 +3844,7 @@ static int
matroska_read_close(AVFormatContext *s)
int n;
matroska_clear_queue(matroska);
+ av_packet_free(&matroska->pkt);
for (n = 0; n < matroska->tracks.nb_elem; n++)
if (tracks[n].type == MATROSKA_TRACK_TYPE_AUDIO)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
