Message ID | d58ba5dc48bab7c3f743af5670b47ed5118579af.1568205721.git.barsnick@gmx.net |
---|---|
State | Superseded |
Headers | show |
On 9/11/2019 10:34 AM, Moritz Barsnick wrote: > Only the first element of the array is used currently, the other > elements are in preparation for a new muxer calculating multiple > hashes. > > Also move alloc/init code from the write_header() functions to > dedicated init() functions, and the cleanup code from the > write_trailer() functions to dedicated deinit() functions. > --- > libavformat/hashenc.c | 88 ++++++++++++++++++++++++++++--------------- > 1 file changed, 58 insertions(+), 30 deletions(-) > > diff --git a/libavformat/hashenc.c b/libavformat/hashenc.c > index 210bfdea0e..4fd41e41b6 100644 > --- a/libavformat/hashenc.c > +++ b/libavformat/hashenc.c > @@ -29,7 +29,7 @@ > > struct HashContext { > const AVClass *avclass; > - struct AVHashContext *hash; > + struct AVHashContext **hashes; > char *hash_name; > int format_version; > }; > @@ -72,20 +72,26 @@ static const AVOption framemd5_options[] = { > #endif > > #if CONFIG_HASH_MUXER || CONFIG_MD5_MUXER > -static int hash_write_header(struct AVFormatContext *s) > +static int hash_init(struct AVFormatContext *s) > { > + int res; > struct HashContext *c = s->priv_data; > - int res = av_hash_alloc(&c->hash, c->hash_name); > - if (res < 0) > + c->hashes = av_mallocz_array(1, sizeof(c->hashes)); > + if (!c->hashes) > + return AVERROR(ENOMEM); > + res = av_hash_alloc(&c->hashes[0], c->hash_name); > + if (res < 0) { > + av_freep(&c->hashes); > return res; > - av_hash_init(c->hash); > + } > + av_hash_init(c->hashes[0]); > return 0; > } > > static int hash_write_packet(struct AVFormatContext *s, AVPacket *pkt) > { > struct HashContext *c = s->priv_data; > - av_hash_update(c->hash, pkt->data, pkt->size); > + av_hash_update(c->hashes[0], pkt->data, pkt->size); > return 0; > } > > @@ -93,16 +99,22 @@ static int hash_write_trailer(struct AVFormatContext *s) > { > struct HashContext *c = s->priv_data; > char buf[AV_HASH_MAX_SIZE*2+128]; > - snprintf(buf, sizeof(buf) - 200, "%s=", av_hash_get_name(c->hash)); > + snprintf(buf, sizeof(buf) - 200, "%s=", av_hash_get_name(c->hashes[0])); > > - av_hash_final_hex(c->hash, buf + strlen(buf), sizeof(buf) - strlen(buf)); > + av_hash_final_hex(c->hashes[0], buf + strlen(buf), sizeof(buf) - strlen(buf)); > av_strlcatf(buf, sizeof(buf), "\n"); > avio_write(s->pb, buf, strlen(buf)); > avio_flush(s->pb); > > - av_hash_freep(&c->hash); > return 0; > } > + > +static void hash_free(struct AVFormatContext *s) > +{ > + struct HashContext *c = s->priv_data; > + av_hash_freep(&c->hashes[0]); AVOutputFormat.deinit() is called when AVOutputFormat.init() fails, so c->hashes can be NULL. same with the framehash muxer. > + av_freep(&c->hashes); > +} > #endif > > #if CONFIG_HASH_MUXER > @@ -119,9 +131,10 @@ AVOutputFormat ff_hash_muxer = { > .priv_data_size = sizeof(struct HashContext), > .audio_codec = AV_CODEC_ID_PCM_S16LE, > .video_codec = AV_CODEC_ID_RAWVIDEO, > - .write_header = hash_write_header, > + .init = hash_init, > .write_packet = hash_write_packet, > .write_trailer = hash_write_trailer, > + .deinit = hash_free, > .flags = AVFMT_VARIABLE_FPS | AVFMT_TS_NONSTRICT | > AVFMT_TS_NEGATIVE, > .priv_class = &hashenc_class, > @@ -142,9 +155,10 @@ AVOutputFormat ff_md5_muxer = { > .priv_data_size = sizeof(struct HashContext), > .audio_codec = AV_CODEC_ID_PCM_S16LE, > .video_codec = AV_CODEC_ID_RAWVIDEO, > - .write_header = hash_write_header, > + .init = hash_init, > .write_packet = hash_write_packet, > .write_trailer = hash_write_trailer, > + .deinit = hash_free, > .flags = AVFMT_VARIABLE_FPS | AVFMT_TS_NONSTRICT | > AVFMT_TS_NEGATIVE, > .priv_class = &md5enc_class, > @@ -164,24 +178,36 @@ static void framehash_print_extradata(struct AVFormatContext *s) > char buf[AV_HASH_MAX_SIZE*2+1]; > > avio_printf(s->pb, "#extradata %d, %31d, ", i, par->extradata_size); > - av_hash_init(c->hash); > - av_hash_update(c->hash, par->extradata, par->extradata_size); > - av_hash_final_hex(c->hash, buf, sizeof(buf)); > + av_hash_init(c->hashes[0]); > + av_hash_update(c->hashes[0], par->extradata, par->extradata_size); > + av_hash_final_hex(c->hashes[0], buf, sizeof(buf)); > avio_write(s->pb, buf, strlen(buf)); > avio_printf(s->pb, "\n"); > } > } > } > > -static int framehash_write_header(struct AVFormatContext *s) > +static int framehash_init(struct AVFormatContext *s) > { > + int res; > struct HashContext *c = s->priv_data; > - int res = av_hash_alloc(&c->hash, c->hash_name); > - if (res < 0) > + c->hashes = av_mallocz_array(1, sizeof(c->hashes)); > + if (!c->hashes) > + return AVERROR(ENOMEM); > + res = av_hash_alloc(&c->hashes[0], c->hash_name); > + if (res < 0) { > + av_freep(&c->hashes); > return res; > + } > + return 0; > +} > + > +static int framehash_write_header(struct AVFormatContext *s) > +{ > + struct HashContext *c = s->priv_data; > avio_printf(s->pb, "#format: frame checksums\n"); > avio_printf(s->pb, "#version: %d\n", c->format_version); > - avio_printf(s->pb, "#hash: %s\n", av_hash_get_name(c->hash)); > + avio_printf(s->pb, "#hash: %s\n", av_hash_get_name(c->hashes[0])); > framehash_print_extradata(s); > ff_framehash_write_header(s); > avio_printf(s->pb, "#stream#, dts, pts, duration, size, hash\n"); > @@ -193,30 +219,30 @@ static int framehash_write_packet(struct AVFormatContext *s, AVPacket *pkt) > struct HashContext *c = s->priv_data; > char buf[AV_HASH_MAX_SIZE*2+128]; > int len; > - av_hash_init(c->hash); > - av_hash_update(c->hash, pkt->data, pkt->size); > + av_hash_init(c->hashes[0]); > + av_hash_update(c->hashes[0], pkt->data, pkt->size); > > snprintf(buf, sizeof(buf) - (AV_HASH_MAX_SIZE * 2 + 1), "%d, %10"PRId64", %10"PRId64", %8"PRId64", %8d, ", > pkt->stream_index, pkt->dts, pkt->pts, pkt->duration, pkt->size); > len = strlen(buf); > - av_hash_final_hex(c->hash, buf + len, sizeof(buf) - len); > + av_hash_final_hex(c->hashes[0], buf + len, sizeof(buf) - len); > avio_write(s->pb, buf, strlen(buf)); > > if (c->format_version > 1 && pkt->side_data_elems) { > int i, j; > avio_printf(s->pb, ", S=%d", pkt->side_data_elems); > for (i = 0; i < pkt->side_data_elems; i++) { > - av_hash_init(c->hash); > + av_hash_init(c->hashes[0]); > if (HAVE_BIGENDIAN && pkt->side_data[i].type == AV_PKT_DATA_PALETTE) { > for (j = 0; j < pkt->side_data[i].size; j += sizeof(uint32_t)) { > uint32_t data = AV_RL32(pkt->side_data[i].data + j); > - av_hash_update(c->hash, (uint8_t *)&data, sizeof(uint32_t)); > + av_hash_update(c->hashes[0], (uint8_t *)&data, sizeof(uint32_t)); > } > } else > - av_hash_update(c->hash, pkt->side_data[i].data, pkt->side_data[i].size); > + av_hash_update(c->hashes[0], pkt->side_data[i].data, pkt->side_data[i].size); > snprintf(buf, sizeof(buf) - (AV_HASH_MAX_SIZE * 2 + 1), ", %8d, ", pkt->side_data[i].size); > len = strlen(buf); > - av_hash_final_hex(c->hash, buf + len, sizeof(buf) - len); > + av_hash_final_hex(c->hashes[0], buf + len, sizeof(buf) - len); > avio_write(s->pb, buf, strlen(buf)); > } > } > @@ -226,11 +252,11 @@ static int framehash_write_packet(struct AVFormatContext *s, AVPacket *pkt) > return 0; > } > > -static int framehash_write_trailer(struct AVFormatContext *s) > +static void framehash_free(struct AVFormatContext *s) > { > struct HashContext *c = s->priv_data; > - av_hash_freep(&c->hash); > - return 0; > + av_hash_freep(&c->hashes[0]); > + av_freep(&c->hashes); > } > #endif > > @@ -248,9 +274,10 @@ AVOutputFormat ff_framehash_muxer = { > .priv_data_size = sizeof(struct HashContext), > .audio_codec = AV_CODEC_ID_PCM_S16LE, > .video_codec = AV_CODEC_ID_RAWVIDEO, > + .init = framehash_init, > .write_header = framehash_write_header, > .write_packet = framehash_write_packet, > - .write_trailer = framehash_write_trailer, > + .deinit = framehash_free, > .flags = AVFMT_VARIABLE_FPS | AVFMT_TS_NONSTRICT | > AVFMT_TS_NEGATIVE, > .priv_class = &framehash_class, > @@ -271,9 +298,10 @@ AVOutputFormat ff_framemd5_muxer = { > .priv_data_size = sizeof(struct HashContext), > .audio_codec = AV_CODEC_ID_PCM_S16LE, > .video_codec = AV_CODEC_ID_RAWVIDEO, > + .init = framehash_init, > .write_header = framehash_write_header, > .write_packet = framehash_write_packet, > - .write_trailer = framehash_write_trailer, > + .deinit = framehash_free, > .flags = AVFMT_VARIABLE_FPS | AVFMT_TS_NONSTRICT | > AVFMT_TS_NEGATIVE, > .priv_class = &framemd5_class, > -- > 2.20.1 > > _______________________________________________ > 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". >
On Wed, Sep 11, 2019 at 10:39:40 -0300, James Almer wrote: > On 9/11/2019 10:34 AM, Moritz Barsnick wrote: > > +static void hash_free(struct AVFormatContext *s) > > +{ > > + struct HashContext *c = s->priv_data; > > + av_hash_freep(&c->hashes[0]); > > AVOutputFormat.deinit() is called when AVOutputFormat.init() fails, so > c->hashes can be NULL. same with the framehash muxer. > > > + av_freep(&c->hashes); > > +} Does this mean there should just be a check for "if (c->hashes)" here, or can the suggestion in http://ffmpeg.org/pipermail/ffmpeg-devel/2019-August/248014.html actually not be implemented? No common freeing function? Thanks, Moritz
On 9/11/2019 10:53 AM, Moritz Barsnick wrote: > On Wed, Sep 11, 2019 at 10:39:40 -0300, James Almer wrote: >> On 9/11/2019 10:34 AM, Moritz Barsnick wrote: >>> +static void hash_free(struct AVFormatContext *s) >>> +{ >>> + struct HashContext *c = s->priv_data; >>> + av_hash_freep(&c->hashes[0]); >> >> AVOutputFormat.deinit() is called when AVOutputFormat.init() fails, so >> c->hashes can be NULL. same with the framehash muxer. >> >>> + av_freep(&c->hashes); >>> +} > > Does this mean there should just be a check for "if (c->hashes)" here, > or can the suggestion in > http://ffmpeg.org/pipermail/ffmpeg-devel/2019-August/248014.html > actually not be implemented? No common freeing function? av_freep() can be used with NULL variables without issues. The problem is that you're dereferencing c->hashes for the av_hash_freep() call. If it's NULL, you'll get a segfault. So just do if (c->hashes) av_hash_freep(&c->hashes[0]); av_freep(&c->hashes); In both muxers. Then adapt it in PATCH 3/3 for the streamhash muxer addition. > > Thanks, > Moritz > _______________________________________________ > 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". >
On Wed, Sep 11, 2019 at 10:39:40 -0300, James Almer wrote: > > +static void hash_free(struct AVFormatContext *s) > > +{ > > + struct HashContext *c = s->priv_data; > > + av_hash_freep(&c->hashes[0]); > > AVOutputFormat.deinit() is called when AVOutputFormat.init() fails, so > c->hashes can be NULL. same with the framehash muxer. BTW, do we have any tools or methods for triggering these failure paths (without modifying the tested code)? Like an LD_PRELOAD lib to randomly or directedly fail av_malloc*()? Or is all this covered by review and fuzzing(?) only? I only do random short use cases and corner/error cases with valgrind, but that doesn't hit ENOMEM. Thanks, Moritz
On 9/11/2019 11:53 AM, Moritz Barsnick wrote: > On Wed, Sep 11, 2019 at 10:39:40 -0300, James Almer wrote: >>> +static void hash_free(struct AVFormatContext *s) >>> +{ >>> + struct HashContext *c = s->priv_data; >>> + av_hash_freep(&c->hashes[0]); >> >> AVOutputFormat.deinit() is called when AVOutputFormat.init() fails, so >> c->hashes can be NULL. same with the framehash muxer. > > BTW, do we have any tools or methods for triggering these failure paths > (without modifying the tested code)? Like an LD_PRELOAD lib to randomly > or directedly fail av_malloc*()? Or is all this covered by review and > fuzzing(?) only? > > I only do random short use cases and corner/error cases with valgrind, > but that doesn't hit ENOMEM. There's ulimit on Linux, but never used it. I know it's the standard way to emulate low RAM environments to trigger ENOMEM errors and detect unchecked allocs. > > Thanks, > Moritz > _______________________________________________ > 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". >
diff --git a/libavformat/hashenc.c b/libavformat/hashenc.c index 210bfdea0e..4fd41e41b6 100644 --- a/libavformat/hashenc.c +++ b/libavformat/hashenc.c @@ -29,7 +29,7 @@ struct HashContext { const AVClass *avclass; - struct AVHashContext *hash; + struct AVHashContext **hashes; char *hash_name; int format_version; }; @@ -72,20 +72,26 @@ static const AVOption framemd5_options[] = { #endif #if CONFIG_HASH_MUXER || CONFIG_MD5_MUXER -static int hash_write_header(struct AVFormatContext *s) +static int hash_init(struct AVFormatContext *s) { + int res; struct HashContext *c = s->priv_data; - int res = av_hash_alloc(&c->hash, c->hash_name); - if (res < 0) + c->hashes = av_mallocz_array(1, sizeof(c->hashes)); + if (!c->hashes) + return AVERROR(ENOMEM); + res = av_hash_alloc(&c->hashes[0], c->hash_name); + if (res < 0) { + av_freep(&c->hashes); return res; - av_hash_init(c->hash); + } + av_hash_init(c->hashes[0]); return 0; } static int hash_write_packet(struct AVFormatContext *s, AVPacket *pkt) { struct HashContext *c = s->priv_data; - av_hash_update(c->hash, pkt->data, pkt->size); + av_hash_update(c->hashes[0], pkt->data, pkt->size); return 0; } @@ -93,16 +99,22 @@ static int hash_write_trailer(struct AVFormatContext *s) { struct HashContext *c = s->priv_data; char buf[AV_HASH_MAX_SIZE*2+128]; - snprintf(buf, sizeof(buf) - 200, "%s=", av_hash_get_name(c->hash)); + snprintf(buf, sizeof(buf) - 200, "%s=", av_hash_get_name(c->hashes[0])); - av_hash_final_hex(c->hash, buf + strlen(buf), sizeof(buf) - strlen(buf)); + av_hash_final_hex(c->hashes[0], buf + strlen(buf), sizeof(buf) - strlen(buf)); av_strlcatf(buf, sizeof(buf), "\n"); avio_write(s->pb, buf, strlen(buf)); avio_flush(s->pb); - av_hash_freep(&c->hash); return 0; } + +static void hash_free(struct AVFormatContext *s) +{ + struct HashContext *c = s->priv_data; + av_hash_freep(&c->hashes[0]); + av_freep(&c->hashes); +} #endif #if CONFIG_HASH_MUXER @@ -119,9 +131,10 @@ AVOutputFormat ff_hash_muxer = { .priv_data_size = sizeof(struct HashContext), .audio_codec = AV_CODEC_ID_PCM_S16LE, .video_codec = AV_CODEC_ID_RAWVIDEO, - .write_header = hash_write_header, + .init = hash_init, .write_packet = hash_write_packet, .write_trailer = hash_write_trailer, + .deinit = hash_free, .flags = AVFMT_VARIABLE_FPS | AVFMT_TS_NONSTRICT | AVFMT_TS_NEGATIVE, .priv_class = &hashenc_class, @@ -142,9 +155,10 @@ AVOutputFormat ff_md5_muxer = { .priv_data_size = sizeof(struct HashContext), .audio_codec = AV_CODEC_ID_PCM_S16LE, .video_codec = AV_CODEC_ID_RAWVIDEO, - .write_header = hash_write_header, + .init = hash_init, .write_packet = hash_write_packet, .write_trailer = hash_write_trailer, + .deinit = hash_free, .flags = AVFMT_VARIABLE_FPS | AVFMT_TS_NONSTRICT | AVFMT_TS_NEGATIVE, .priv_class = &md5enc_class, @@ -164,24 +178,36 @@ static void framehash_print_extradata(struct AVFormatContext *s) char buf[AV_HASH_MAX_SIZE*2+1]; avio_printf(s->pb, "#extradata %d, %31d, ", i, par->extradata_size); - av_hash_init(c->hash); - av_hash_update(c->hash, par->extradata, par->extradata_size); - av_hash_final_hex(c->hash, buf, sizeof(buf)); + av_hash_init(c->hashes[0]); + av_hash_update(c->hashes[0], par->extradata, par->extradata_size); + av_hash_final_hex(c->hashes[0], buf, sizeof(buf)); avio_write(s->pb, buf, strlen(buf)); avio_printf(s->pb, "\n"); } } } -static int framehash_write_header(struct AVFormatContext *s) +static int framehash_init(struct AVFormatContext *s) { + int res; struct HashContext *c = s->priv_data; - int res = av_hash_alloc(&c->hash, c->hash_name); - if (res < 0) + c->hashes = av_mallocz_array(1, sizeof(c->hashes)); + if (!c->hashes) + return AVERROR(ENOMEM); + res = av_hash_alloc(&c->hashes[0], c->hash_name); + if (res < 0) { + av_freep(&c->hashes); return res; + } + return 0; +} + +static int framehash_write_header(struct AVFormatContext *s) +{ + struct HashContext *c = s->priv_data; avio_printf(s->pb, "#format: frame checksums\n"); avio_printf(s->pb, "#version: %d\n", c->format_version); - avio_printf(s->pb, "#hash: %s\n", av_hash_get_name(c->hash)); + avio_printf(s->pb, "#hash: %s\n", av_hash_get_name(c->hashes[0])); framehash_print_extradata(s); ff_framehash_write_header(s); avio_printf(s->pb, "#stream#, dts, pts, duration, size, hash\n"); @@ -193,30 +219,30 @@ static int framehash_write_packet(struct AVFormatContext *s, AVPacket *pkt) struct HashContext *c = s->priv_data; char buf[AV_HASH_MAX_SIZE*2+128]; int len; - av_hash_init(c->hash); - av_hash_update(c->hash, pkt->data, pkt->size); + av_hash_init(c->hashes[0]); + av_hash_update(c->hashes[0], pkt->data, pkt->size); snprintf(buf, sizeof(buf) - (AV_HASH_MAX_SIZE * 2 + 1), "%d, %10"PRId64", %10"PRId64", %8"PRId64", %8d, ", pkt->stream_index, pkt->dts, pkt->pts, pkt->duration, pkt->size); len = strlen(buf); - av_hash_final_hex(c->hash, buf + len, sizeof(buf) - len); + av_hash_final_hex(c->hashes[0], buf + len, sizeof(buf) - len); avio_write(s->pb, buf, strlen(buf)); if (c->format_version > 1 && pkt->side_data_elems) { int i, j; avio_printf(s->pb, ", S=%d", pkt->side_data_elems); for (i = 0; i < pkt->side_data_elems; i++) { - av_hash_init(c->hash); + av_hash_init(c->hashes[0]); if (HAVE_BIGENDIAN && pkt->side_data[i].type == AV_PKT_DATA_PALETTE) { for (j = 0; j < pkt->side_data[i].size; j += sizeof(uint32_t)) { uint32_t data = AV_RL32(pkt->side_data[i].data + j); - av_hash_update(c->hash, (uint8_t *)&data, sizeof(uint32_t)); + av_hash_update(c->hashes[0], (uint8_t *)&data, sizeof(uint32_t)); } } else - av_hash_update(c->hash, pkt->side_data[i].data, pkt->side_data[i].size); + av_hash_update(c->hashes[0], pkt->side_data[i].data, pkt->side_data[i].size); snprintf(buf, sizeof(buf) - (AV_HASH_MAX_SIZE * 2 + 1), ", %8d, ", pkt->side_data[i].size); len = strlen(buf); - av_hash_final_hex(c->hash, buf + len, sizeof(buf) - len); + av_hash_final_hex(c->hashes[0], buf + len, sizeof(buf) - len); avio_write(s->pb, buf, strlen(buf)); } } @@ -226,11 +252,11 @@ static int framehash_write_packet(struct AVFormatContext *s, AVPacket *pkt) return 0; } -static int framehash_write_trailer(struct AVFormatContext *s) +static void framehash_free(struct AVFormatContext *s) { struct HashContext *c = s->priv_data; - av_hash_freep(&c->hash); - return 0; + av_hash_freep(&c->hashes[0]); + av_freep(&c->hashes); } #endif @@ -248,9 +274,10 @@ AVOutputFormat ff_framehash_muxer = { .priv_data_size = sizeof(struct HashContext), .audio_codec = AV_CODEC_ID_PCM_S16LE, .video_codec = AV_CODEC_ID_RAWVIDEO, + .init = framehash_init, .write_header = framehash_write_header, .write_packet = framehash_write_packet, - .write_trailer = framehash_write_trailer, + .deinit = framehash_free, .flags = AVFMT_VARIABLE_FPS | AVFMT_TS_NONSTRICT | AVFMT_TS_NEGATIVE, .priv_class = &framehash_class, @@ -271,9 +298,10 @@ AVOutputFormat ff_framemd5_muxer = { .priv_data_size = sizeof(struct HashContext), .audio_codec = AV_CODEC_ID_PCM_S16LE, .video_codec = AV_CODEC_ID_RAWVIDEO, + .init = framehash_init, .write_header = framehash_write_header, .write_packet = framehash_write_packet, - .write_trailer = framehash_write_trailer, + .deinit = framehash_free, .flags = AVFMT_VARIABLE_FPS | AVFMT_TS_NONSTRICT | AVFMT_TS_NEGATIVE, .priv_class = &framemd5_class,