| Message ID | 20210312130733.1980124-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/mxfdec: Don't use wrong type of pointer | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
fre 2021-03-12 klockan 14:07 +0100 skrev Andreas Rheinhardt: > @@ -2922,20 +2929,19 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF > } > } > } > - if (ctx_size && tag == 0x3C0A) { > - avio_read(pb, ctx->uid, 16); > + if (meta && tag == 0x3C0A) { Why not keep this conditional on ctx_size? That way you could get rid of ctx altogether > + avio_read(pb, meta->uid, 16); > } else if ((ret = read_child(ctx, pb, tag, size, uid, -1)) < 0) { This could be ctx_size ? meta : mxf > - if (ctx_size) > - mxf_free_metadataset(&ctx, 1); > + if (meta) > + mxf_free_metadataset(&meta, 1); Please add {} while you're at it /Tomas
Tomas Härdin: > fre 2021-03-12 klockan 14:07 +0100 skrev Andreas Rheinhardt: >> @@ -2922,20 +2929,19 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF >> } >> } >> } >> - if (ctx_size && tag == 0x3C0A) { >> - avio_read(pb, ctx->uid, 16); >> + if (meta && tag == 0x3C0A) { > > Why not keep this conditional on ctx_size? That way you could get rid > of ctx altogether > Because I consider it more natural to check for the existence of a MXFMetadataSet by checking whether a pointer to MXFMetadataSet is NULL or not. After all, ctx_size could also contain the size of the MXFContext. >> + avio_read(pb, meta->uid, 16); >> } else if ((ret = read_child(ctx, pb, tag, size, uid, -1)) < 0) { > > This could be ctx_size ? meta : mxf > Seems like another check to me. >> - if (ctx_size) >> - mxf_free_metadataset(&ctx, 1); >> + if (meta) >> + mxf_free_metadataset(&meta, 1); > > Please add {} while you're at it > Will do. - Andreas
mån 2021-03-15 klockan 14:38 +0100 skrev Andreas Rheinhardt: > Tomas Härdin: > > fre 2021-03-12 klockan 14:07 +0100 skrev Andreas Rheinhardt: > > > @@ -2922,20 +2929,19 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF > > > } > > > } > > > } > > > - if (ctx_size && tag == 0x3C0A) { > > > - avio_read(pb, ctx->uid, 16); > > > + if (meta && tag == 0x3C0A) { > > > > Why not keep this conditional on ctx_size? That way you could get rid > > of ctx altogether > > > > Because I consider it more natural to check for the existence of a > MXFMetadataSet by checking whether a pointer to MXFMetadataSet is NULL > or not. After all, ctx_size could also contain the size of the MXFContext. No, it cannot. Look at mxf_metadata_read_table. ctx_size serves a dual purpose: whether to pass MXFContext to read(), or to allocate a new metadata element that is passed to read() instead. /Tomas
Tomas Härdin: > mån 2021-03-15 klockan 14:38 +0100 skrev Andreas Rheinhardt: >> Tomas Härdin: >>> fre 2021-03-12 klockan 14:07 +0100 skrev Andreas Rheinhardt: >>>> @@ -2922,20 +2929,19 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF >>>> } >>>> } >>>> } >>>> - if (ctx_size && tag == 0x3C0A) { >>>> - avio_read(pb, ctx->uid, 16); >>>> + if (meta && tag == 0x3C0A) { >>> >>> Why not keep this conditional on ctx_size? That way you could get rid >>> of ctx altogether >>> >> >> Because I consider it more natural to check for the existence of a >> MXFMetadataSet by checking whether a pointer to MXFMetadataSet is NULL >> or not. After all, ctx_size could also contain the size of the MXFContext. > > No, it cannot. Look at mxf_metadata_read_table. ctx_size serves a dual > purpose: whether to pass MXFContext to read(), or to allocate a new > metadata element that is passed to read() instead. > I know that it can't (yes, I looked at that table), but just judging from its name, ctx_size could also be the size of the MXFContext. And that's why I think that using a MXFMetadataSet* is more natural. - Andreas
mån 2021-03-15 klockan 19:48 +0100 skrev Andreas Rheinhardt: > Tomas Härdin: > > mån 2021-03-15 klockan 14:38 +0100 skrev Andreas Rheinhardt: > > > Tomas Härdin: > > > > fre 2021-03-12 klockan 14:07 +0100 skrev Andreas Rheinhardt: > > > > > @@ -2922,20 +2929,19 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF > > > > > } > > > > > } > > > > > } > > > > > - if (ctx_size && tag == 0x3C0A) { > > > > > - avio_read(pb, ctx->uid, 16); > > > > > + if (meta && tag == 0x3C0A) { > > > > > > > > Why not keep this conditional on ctx_size? That way you could get rid > > > > of ctx altogether > > > > > > > > > > Because I consider it more natural to check for the existence of a > > > MXFMetadataSet by checking whether a pointer to MXFMetadataSet is NULL > > > or not. After all, ctx_size could also contain the size of the MXFContext. > > > > No, it cannot. Look at mxf_metadata_read_table. ctx_size serves a dual > > purpose: whether to pass MXFContext to read(), or to allocate a new > > metadata element that is passed to read() instead. > > > I know that it can't (yes, I looked at that table), but just judging > from its name, ctx_size could also be the size of the MXFContext. And > that's why I think that using a MXFMetadataSet* is more natural. Except it isn't sizeof(MXFContext). It's zero. But anyway, this is bikeshedding on my part since it'll work the same. I have a little patch that adds some documentation to the function to clarify what ctx_size does. /Tomas
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index bb00838a3f..d7213bda30 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -2889,13 +2889,20 @@ static int mxf_metadataset_init(MXFMetadataSet *ctx, enum MXFMetadataSetType typ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadFunc *read_child, int ctx_size, enum MXFMetadataSetType type) { AVIOContext *pb = mxf->fc->pb; - MXFMetadataSet *ctx = ctx_size ? av_mallocz(ctx_size) : mxf; uint64_t klv_end = avio_tell(pb) + klv->length; + MXFMetadataSet *meta; + void *ctx; - if (!ctx) - return AVERROR(ENOMEM); - if (ctx_size) - mxf_metadataset_init(ctx, type); + if (ctx_size) { + meta = av_mallocz(ctx_size); + if (!meta) + return AVERROR(ENOMEM); + ctx = meta; + mxf_metadataset_init(meta, type); + } else { + meta = NULL; + ctx = mxf; + } while (avio_tell(pb) + 4 < klv_end && !avio_feof(pb)) { int ret; int tag = avio_rb16(pb); @@ -2922,20 +2929,19 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF } } } - if (ctx_size && tag == 0x3C0A) { - avio_read(pb, ctx->uid, 16); + if (meta && tag == 0x3C0A) { + avio_read(pb, meta->uid, 16); } else if ((ret = read_child(ctx, pb, tag, size, uid, -1)) < 0) { - if (ctx_size) - mxf_free_metadataset(&ctx, 1); + if (meta) + mxf_free_metadataset(&meta, 1); return ret; } /* Accept the 64k local set limit being exceeded (Avid). Don't accept * it extending past the end of the KLV though (zzuf5.mxf). */ if (avio_tell(pb) > klv_end) { - if (ctx_size) { - mxf_free_metadataset(&ctx, 1); - } + if (meta) + mxf_free_metadataset(&meta, 1); av_log(mxf->fc, AV_LOG_ERROR, "local tag %#04x extends past end of local set @ %#"PRIx64"\n", @@ -2944,7 +2950,7 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF } else if (avio_tell(pb) <= next) /* only seek forward, else this can loop for a long time */ avio_seek(pb, next, SEEK_SET); } - return ctx_size ? mxf_add_metadata_set(mxf, &ctx) : 0; + return meta ? mxf_add_metadata_set(mxf, &meta) : 0; } /**
If one of the two results of a ternary conditional is a pointer to void, the type of the whole conditional operator is a pointer to void, even when the other possible result is not a pointer to void. This loophole in the type system has allowed mxf_read_local_tags to have a pointer of type pointer to MXFMetadataSet that actually points to an MXFContext. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/mxfdec.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)