| Message ID | GV1P250MB0737F127E8B0D4869D3CFCE78F2CA@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | e5ab2dab2c73710c753bf8a44171fc747625e9f3 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/evcdec: Avoid nonsense casts | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 7/6/2023 6:08 PM, Andreas Rheinhardt wrote: > Fixes potential use of uninitialized values > in evc_read_nal_unit_length(). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavformat/evcdec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/evcdec.c b/libavformat/evcdec.c > index 9886542311..0f464930f7 100644 > --- a/libavformat/evcdec.c > +++ b/libavformat/evcdec.c > @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s, AVPacket *pkt) > ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE); > if (ret < 0) > return ret; > + if (ret != EVC_NALU_LENGTH_PREFIX_SIZE) > + return AVERROR_INVALIDDATA; There's a ffio_ensure_seekback() for EVC_NALU_LENGTH_PREFIX_SIZE bytes immediately before the avio_read() call. Shouldn't that be enough to guarantee that much can be read? Also, you can just pass ret to evc_read_nal_unit_length() below instead of adding this check here. It will return an error if it's < EVC_NALU_LENGTH_PREFIX_SIZE. > > nalu_size = evc_read_nal_unit_length(buf, EVC_NALU_LENGTH_PREFIX_SIZE); > if (!nalu_size || nalu_size > INT_MAX)
On 7/6/2023 10:14 PM, James Almer wrote: > On 7/6/2023 6:08 PM, Andreas Rheinhardt wrote: >> Fixes potential use of uninitialized values >> in evc_read_nal_unit_length(). >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavformat/evcdec.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/libavformat/evcdec.c b/libavformat/evcdec.c >> index 9886542311..0f464930f7 100644 >> --- a/libavformat/evcdec.c >> +++ b/libavformat/evcdec.c >> @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s, >> AVPacket *pkt) >> ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE); >> if (ret < 0) >> return ret; >> + if (ret != EVC_NALU_LENGTH_PREFIX_SIZE) >> + return AVERROR_INVALIDDATA; > > There's a ffio_ensure_seekback() for EVC_NALU_LENGTH_PREFIX_SIZE bytes > immediately before the avio_read() call. Shouldn't that be enough to > guarantee that much can be read? > > Also, you can just pass ret to evc_read_nal_unit_length() below instead > of adding this check here. It will return an error if it's < > EVC_NALU_LENGTH_PREFIX_SIZE. Oh, my bad, i was looking at the function of the same name in libavcodec/evc_parse.h The function in evc.h could be changed to also use the same check as the one the evc_parse.h version alongside the other change you're doing in patch 3/3. > >> nalu_size = evc_read_nal_unit_length(buf, >> EVC_NALU_LENGTH_PREFIX_SIZE); >> if (!nalu_size || nalu_size > INT_MAX)
James Almer: > On 7/6/2023 6:08 PM, Andreas Rheinhardt wrote: >> Fixes potential use of uninitialized values >> in evc_read_nal_unit_length(). >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavformat/evcdec.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/libavformat/evcdec.c b/libavformat/evcdec.c >> index 9886542311..0f464930f7 100644 >> --- a/libavformat/evcdec.c >> +++ b/libavformat/evcdec.c >> @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s, >> AVPacket *pkt) >> ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE); >> if (ret < 0) >> return ret; >> + if (ret != EVC_NALU_LENGTH_PREFIX_SIZE) >> + return AVERROR_INVALIDDATA; > > There's a ffio_ensure_seekback() for EVC_NALU_LENGTH_PREFIX_SIZE bytes > immediately before the avio_read() call. Shouldn't that be enough to > guarantee that much can be read? > ffio_ensure_seekback() ensures that the read buffer is big enough so that reading EVC_NALU_LENGTH_PREFIX_SIZE bytes does not lead to a reset of the buffer; it does not imply that the buffer already contains EVC_NALU_LENGTH_PREFIX_SIZE bytes. In fact, there is not a single codepath in ffio_ensure_seekback() that actually reads further input. (If EVC_NALU_LENGTH_PREFIX_SIZE bytes are not available in the buffer, then the buf_size <= s->buffer_size codepath will likely be taken in the non-seekable case (in the seekable case, ffio_ensure_seekback() does even less, namely nothing).) > Also, you can just pass ret to evc_read_nal_unit_length() below instead > of adding this check here. It will return an error if it's < > EVC_NALU_LENGTH_PREFIX_SIZE. > It will actually return 0 which the caller will transform into an error. I do not want to rely on this behaviour. (Why did you add two inline functions of the same name in different evc headers?) >> nalu_size = evc_read_nal_unit_length(buf, >> EVC_NALU_LENGTH_PREFIX_SIZE); >> if (!nalu_size || nalu_size > INT_MAX)
James Almer: > On 7/6/2023 10:14 PM, James Almer wrote: >> On 7/6/2023 6:08 PM, Andreas Rheinhardt wrote: >>> Fixes potential use of uninitialized values >>> in evc_read_nal_unit_length(). >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> libavformat/evcdec.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/libavformat/evcdec.c b/libavformat/evcdec.c >>> index 9886542311..0f464930f7 100644 >>> --- a/libavformat/evcdec.c >>> +++ b/libavformat/evcdec.c >>> @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s, >>> AVPacket *pkt) >>> ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE); >>> if (ret < 0) >>> return ret; >>> + if (ret != EVC_NALU_LENGTH_PREFIX_SIZE) >>> + return AVERROR_INVALIDDATA; >> >> There's a ffio_ensure_seekback() for EVC_NALU_LENGTH_PREFIX_SIZE bytes >> immediately before the avio_read() call. Shouldn't that be enough to >> guarantee that much can be read? >> >> Also, you can just pass ret to evc_read_nal_unit_length() below >> instead of adding this check here. It will return an error if it's < >> EVC_NALU_LENGTH_PREFIX_SIZE. > > Oh, my bad, i was looking at the function of the same name in > libavcodec/evc_parse.h > > The function in evc.h could be changed to also use the same check as the > one the evc_parse.h version alongside the other change you're doing in > patch 3/3. > These functions already do the same (except for the log message); they both return 0 if not enough data is available. The return value would need to be int64_t if one wanted to return both error codes and lengths via it. I don't really see the advantage of this. - Andreas
On 7/6/2023 10:30 PM, Andreas Rheinhardt wrote: > James Almer: >> On 7/6/2023 10:14 PM, James Almer wrote: >>> On 7/6/2023 6:08 PM, Andreas Rheinhardt wrote: >>>> Fixes potential use of uninitialized values >>>> in evc_read_nal_unit_length(). >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>> --- >>>> libavformat/evcdec.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/libavformat/evcdec.c b/libavformat/evcdec.c >>>> index 9886542311..0f464930f7 100644 >>>> --- a/libavformat/evcdec.c >>>> +++ b/libavformat/evcdec.c >>>> @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s, >>>> AVPacket *pkt) >>>> ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE); >>>> if (ret < 0) >>>> return ret; >>>> + if (ret != EVC_NALU_LENGTH_PREFIX_SIZE) >>>> + return AVERROR_INVALIDDATA; >>> >>> There's a ffio_ensure_seekback() for EVC_NALU_LENGTH_PREFIX_SIZE bytes >>> immediately before the avio_read() call. Shouldn't that be enough to >>> guarantee that much can be read? >>> >>> Also, you can just pass ret to evc_read_nal_unit_length() below >>> instead of adding this check here. It will return an error if it's < >>> EVC_NALU_LENGTH_PREFIX_SIZE. >> >> Oh, my bad, i was looking at the function of the same name in >> libavcodec/evc_parse.h >> >> The function in evc.h could be changed to also use the same check as the >> one the evc_parse.h version alongside the other change you're doing in >> patch 3/3. >> > > These functions already do the same (except for the log message); they > both return 0 if not enough data is available. The return value would > need to be int64_t if one wanted to return both error codes and lengths > via it. I don't really see the advantage of this. Fair enough. Then what about ffio_ensure_seekback()? Should that not guarantee at least EVC_NALU_LENGTH_PREFIX_SIZE bytes are buffered, thus readable and seekable backwards?
James Almer: > On 7/6/2023 10:30 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 7/6/2023 10:14 PM, James Almer wrote: >>>> On 7/6/2023 6:08 PM, Andreas Rheinhardt wrote: >>>>> Fixes potential use of uninitialized values >>>>> in evc_read_nal_unit_length(). >>>>> >>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>>> --- >>>>> libavformat/evcdec.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/libavformat/evcdec.c b/libavformat/evcdec.c >>>>> index 9886542311..0f464930f7 100644 >>>>> --- a/libavformat/evcdec.c >>>>> +++ b/libavformat/evcdec.c >>>>> @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s, >>>>> AVPacket *pkt) >>>>> ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE); >>>>> if (ret < 0) >>>>> return ret; >>>>> + if (ret != EVC_NALU_LENGTH_PREFIX_SIZE) >>>>> + return AVERROR_INVALIDDATA; >>>> >>>> There's a ffio_ensure_seekback() for EVC_NALU_LENGTH_PREFIX_SIZE bytes >>>> immediately before the avio_read() call. Shouldn't that be enough to >>>> guarantee that much can be read? >>>> >>>> Also, you can just pass ret to evc_read_nal_unit_length() below >>>> instead of adding this check here. It will return an error if it's < >>>> EVC_NALU_LENGTH_PREFIX_SIZE. >>> >>> Oh, my bad, i was looking at the function of the same name in >>> libavcodec/evc_parse.h >>> >>> The function in evc.h could be changed to also use the same check as the >>> one the evc_parse.h version alongside the other change you're doing in >>> patch 3/3. >>> >> >> These functions already do the same (except for the log message); they >> both return 0 if not enough data is available. The return value would >> need to be int64_t if one wanted to return both error codes and lengths >> via it. I don't really see the advantage of this. > > Fair enough. Then what about ffio_ensure_seekback()? Should that not > guarantee at least EVC_NALU_LENGTH_PREFIX_SIZE bytes are buffered, thus > readable and seekable backwards? No. It is just supposed to ensure that if one reads EVC_NALU_LENGTH_PREFIX_SIZE bytes, one can seek back and "unread" them. See also my earlier mail. - Andreas
On 7/6/2023 10:24 PM, Andreas Rheinhardt wrote: > James Almer: >> On 7/6/2023 6:08 PM, Andreas Rheinhardt wrote: >>> Fixes potential use of uninitialized values >>> in evc_read_nal_unit_length(). >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> libavformat/evcdec.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/libavformat/evcdec.c b/libavformat/evcdec.c >>> index 9886542311..0f464930f7 100644 >>> --- a/libavformat/evcdec.c >>> +++ b/libavformat/evcdec.c >>> @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s, >>> AVPacket *pkt) >>> ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE); >>> if (ret < 0) >>> return ret; >>> + if (ret != EVC_NALU_LENGTH_PREFIX_SIZE) >>> + return AVERROR_INVALIDDATA; >> >> There's a ffio_ensure_seekback() for EVC_NALU_LENGTH_PREFIX_SIZE bytes >> immediately before the avio_read() call. Shouldn't that be enough to >> guarantee that much can be read? >> > > ffio_ensure_seekback() ensures that the read buffer is big enough so > that reading EVC_NALU_LENGTH_PREFIX_SIZE bytes does not lead to a reset > of the buffer; it does not imply that the buffer already contains > EVC_NALU_LENGTH_PREFIX_SIZE bytes. In fact, there is not a single > codepath in ffio_ensure_seekback() that actually reads further input. > (If EVC_NALU_LENGTH_PREFIX_SIZE bytes are not available in the buffer, > then the buf_size <= s->buffer_size codepath will likely be taken in the > non-seekable case (in the seekable case, ffio_ensure_seekback() does > even less, namely nothing).) > >> Also, you can just pass ret to evc_read_nal_unit_length() below instead >> of adding this check here. It will return an error if it's < >> EVC_NALU_LENGTH_PREFIX_SIZE. >> > > It will actually return 0 which the caller will transform into an error. > I do not want to rely on this behaviour. Ok, patch LGTM then. > (Why did you add two inline functions of the same name in different evc > headers?) If i recall correctly, because the probe function must not call av_log. > >>> nalu_size = evc_read_nal_unit_length(buf, >>> EVC_NALU_LENGTH_PREFIX_SIZE); >>> if (!nalu_size || nalu_size > INT_MAX) > > _______________________________________________ > 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/evcdec.c b/libavformat/evcdec.c index 9886542311..0f464930f7 100644 --- a/libavformat/evcdec.c +++ b/libavformat/evcdec.c @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s, AVPacket *pkt) ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE); if (ret < 0) return ret; + if (ret != EVC_NALU_LENGTH_PREFIX_SIZE) + return AVERROR_INVALIDDATA; nalu_size = evc_read_nal_unit_length(buf, EVC_NALU_LENGTH_PREFIX_SIZE); if (!nalu_size || nalu_size > INT_MAX)
Fixes potential use of uninitialized values in evc_read_nal_unit_length(). Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavformat/evcdec.c | 2 ++ 1 file changed, 2 insertions(+)