| Message ID | 20200901150335.103855-4-derek.buitenhuis@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avformat/mov: Fix return type used for av_seek in mfra code | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 9/1/2020 12:03 PM, Derek Buitenhuis wrote: > This also changes a check for mfra_size from < 0 to == 0, since > it was always wrong, as avio_rb32 returns an unsigned integer. mfra_size in this function was an int32_t, so storing the output of avio_rb32() could end up with a negative value. It'll no longer be the case now that you made mfra_size in MOVContext an uint32_t. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/mov.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index e33031f158..e901eb527f 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -7499,22 +7499,22 @@ static int mov_read_mfra(MOVContext *c, AVIOContext *f) > int64_t stream_size = avio_size(f); > int64_t original_pos = avio_tell(f); > int64_t seek_ret; > - int32_t mfra_size; > int ret = -1; > if ((seek_ret = avio_seek(f, stream_size - 4, SEEK_SET)) < 0) { > ret = seek_ret; > goto fail; > } > - mfra_size = avio_rb32(f); > - if (mfra_size < 0 || mfra_size > stream_size) { > + c->mfra_size = avio_rb32(f); > + c->have_read_mfra_size = 1; > + if (c->mfra_size == 0 || c->mfra_size > stream_size) { nit: !c->mfra_size. > av_log(c->fc, AV_LOG_DEBUG, "doesn't look like mfra (unreasonable size)\n"); > goto fail; > } > - if ((seek_ret = avio_seek(f, -mfra_size, SEEK_CUR)) < 0) { > + if ((seek_ret = avio_seek(f, -c->mfra_size, SEEK_CUR)) < 0) { You may have to cast c->mfra_size to int64_t before negating it, or use a local int64_t variable. > ret = seek_ret; > goto fail; > } > - if (avio_rb32(f) != mfra_size) { > + if (avio_rb32(f) != c->mfra_size) { > av_log(c->fc, AV_LOG_DEBUG, "doesn't look like mfra (size mismatch)\n"); > goto fail; > } >
On 01/09/2020 16:36, James Almer wrote: > mfra_size in this function was an int32_t, so storing the output of > avio_rb32() could end up with a negative value. > It'll no longer be the case now that you made mfra_size in MOVContext an > uint32_t. How was that ever valid in the first place, considering avio_rb32 returns an unsigned 32-bit integer? - Derek
On 9/1/2020 12:49 PM, Derek Buitenhuis wrote: > On 01/09/2020 16:36, James Almer wrote: >> mfra_size in this function was an int32_t, so storing the output of >> avio_rb32() could end up with a negative value. >> It'll no longer be the case now that you made mfra_size in MOVContext an >> uint32_t. > > How was that ever valid in the first place, considering avio_rb32 returns an > unsigned 32-bit integer? How else would you read 32bit signed integers using AVIOContext function helpers if not with avio_rb32()? > > - Derek > _______________________________________________ > 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 01/09/2020 17:02, James Almer wrote: > How else would you read 32bit signed integers using AVIOContext function > helpers if not with avio_rb32()? The mfra size is not signed. It is an unsigned 32-bit integer. Spec (mfro is the last part of the mfra): aligned(8) class MovieFragmentRandomAccessOffsetBox extends FullBox(‘mfro’, version, 0) { unsigned int(32) size; } - Derek
On 9/1/2020 1:06 PM, Derek Buitenhuis wrote: > On 01/09/2020 17:02, James Almer wrote: >> How else would you read 32bit signed integers using AVIOContext function >> helpers if not with avio_rb32()? > > The mfra size is not signed. It is an unsigned 32-bit integer. > > Spec (mfro is the last part of the mfra): > > aligned(8) class MovieFragmentRandomAccessOffsetBox > extends FullBox(‘mfro’, version, 0) { > unsigned int(32) size; > } > > - Derek Oh, you mean how it worked with mfra_size being declared as an int32_t. I was just mentioning why there was a <= 0 check for it. And I guess because no mfra box parsed by lavf was ever bigger than ~2gb, so it never failed. But yes, it was a bug that you're fixing in this set. (For that matter, size could in theory also be a 64bit integer according to the spec).
On 01/09/2020 17:11, James Almer wrote: > Oh, you mean how it worked with mfra_size being declared as an int32_t. > I was just mentioning why there was a <= 0 check for it. And I guess > because no mfra box parsed by lavf was ever bigger than ~2gb, so it > never failed. > But yes, it was a bug that you're fixing in this set. Yep, apologies if I was not clear. > (For that matter, size could in theory also be a 64bit integer according > to the spec). It cannot, as far as I can tell. The 'size' entry in the mfra box is a separate member from the normal box size, and is always 32-bits, so that you know to seek to END-4. - Derek
On 9/1/2020 1:14 PM, Derek Buitenhuis wrote: > On 01/09/2020 17:11, James Almer wrote: >> Oh, you mean how it worked with mfra_size being declared as an int32_t. >> I was just mentioning why there was a <= 0 check for it. And I guess >> because no mfra box parsed by lavf was ever bigger than ~2gb, so it >> never failed. >> But yes, it was a bug that you're fixing in this set. > > Yep, apologies if I was not clear. > >> (For that matter, size could in theory also be a 64bit integer according >> to the spec). > > It cannot, as far as I can tell. The 'size' entry in the mfra box is a separate > member from the normal box size, and is always 32-bits, so that you know to > seek to END-4. Right, this is a field in mfro and not the mfra Box size. Nevermind then. > > - Derek > > _______________________________________________ > 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/mov.c b/libavformat/mov.c index e33031f158..e901eb527f 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -7499,22 +7499,22 @@ static int mov_read_mfra(MOVContext *c, AVIOContext *f) int64_t stream_size = avio_size(f); int64_t original_pos = avio_tell(f); int64_t seek_ret; - int32_t mfra_size; int ret = -1; if ((seek_ret = avio_seek(f, stream_size - 4, SEEK_SET)) < 0) { ret = seek_ret; goto fail; } - mfra_size = avio_rb32(f); - if (mfra_size < 0 || mfra_size > stream_size) { + c->mfra_size = avio_rb32(f); + c->have_read_mfra_size = 1; + if (c->mfra_size == 0 || c->mfra_size > stream_size) { av_log(c->fc, AV_LOG_DEBUG, "doesn't look like mfra (unreasonable size)\n"); goto fail; } - if ((seek_ret = avio_seek(f, -mfra_size, SEEK_CUR)) < 0) { + if ((seek_ret = avio_seek(f, -c->mfra_size, SEEK_CUR)) < 0) { ret = seek_ret; goto fail; } - if (avio_rb32(f) != mfra_size) { + if (avio_rb32(f) != c->mfra_size) { av_log(c->fc, AV_LOG_DEBUG, "doesn't look like mfra (size mismatch)\n"); goto fail; }
This also changes a check for mfra_size from < 0 to == 0, since it was always wrong, as avio_rb32 returns an unsigned integer. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- libavformat/mov.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)