On 9/14/2021 6:09 PM, Michael Niedermayer wrote:
On Sat, Jul 10, 2021 at 03:31:14PM +0200, Michael Niedermayer wrote:
On Sat, Apr 17, 2021 at 03:12:29AM +0200, Andreas Rheinhardt wrote:
James Almer:
On 4/16/2021 9:13 PM, Andreas Rheinhardt wrote:
James Almer:
On 4/16/2021 8:45 PM, Andreas Rheinhardt wrote:
James Almer:
On 4/16/2021 7:45 PM, James Almer wrote:
On 4/16/2021 7:24 PM, Andreas Rheinhardt wrote:
James Almer:
On 4/16/2021 4:04 PM, Michael Niedermayer wrote:
On Thu, Apr 15, 2021 at 06:22:10PM -0300, James Almer wrote:
On 4/15/2021 5:44 PM, Michael Niedermayer wrote:
Fixes: runtime error: signed integer overflow: 65312 * 65535
cannot
be represented in type 'int'
Fixes:
32832/clusterfuzz-testcase-minimized-ffmpeg_dem_RM_fuzzer-4817710040088576
Found-by: continuous fuzzing process
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc>
---
libavformat/rmdec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index fc3bff4859..af032ed90a 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -269,9 +269,9 @@ static int
rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
case DEINT_ID_INT4:
if (ast->coded_framesize >
ast->audio_framesize ||
sub_packet_h <= 1 ||
- ast->coded_framesize * sub_packet_h > (2 +
(sub_packet_h & 1)) * ast->audio_framesize)
+ ast->coded_framesize * (uint64_t)sub_packet_h
(2
+ (sub_packet_h & 1)) * ast->audio_framesize)
This check seems superfluous with the one below right after it.
ast->coded_framesize * sub_packet_h must be equal to 2 *
ast->audio_framesize. It can be removed.
return AVERROR_INVALIDDATA;
- if (ast->coded_framesize * sub_packet_h !=
2*ast->audio_framesize) {
+ if (ast->coded_framesize *
(uint64_t)sub_packet_h !=
2*ast->audio_framesize) {
avpriv_request_sample(s, "mismatching
interleaver
parameters");
return AVERROR_INVALIDDATA;
}
How about something like
diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index fc3bff4859..09880ee3fe 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -269,7 +269,7 @@ static int
rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
case DEINT_ID_INT4:
if (ast->coded_framesize >
ast->audio_framesize ||
sub_packet_h <= 1 ||
- ast->coded_framesize * sub_packet_h > (2 +
(sub_packet_h & 1)) * ast->audio_framesize)
+ ast->audio_framesize > INT_MAX / sub_packet_h)
return AVERROR_INVALIDDATA;
if (ast->coded_framesize * sub_packet_h !=
2*ast->audio_framesize) {
avpriv_request_sample(s, "mismatching
interleaver
parameters");
Instead?
The 2 if() execute different things, the 2nd requests a sample,
the
first
not. I think this suggestion would change when we request a sample
Why are we returning INVALIDDATA after requesting a sample, for
that
matter? If it's considered an invalid scenario, do we really need a
sample?
In any case, if you don't want more files where
"ast->coded_framesize *
sub_packet_h != 2*ast->audio_framesize" would print a sample
request,
then maybe something like the following could be used instead?
diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index fc3bff4859..10c1699a81 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -269,6 +269,7 @@ static int
rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
case DEINT_ID_INT4:
if (ast->coded_framesize > ast->audio_framesize ||
sub_packet_h <= 1 ||
+ ast->audio_framesize > INT_MAX / sub_packet_h ||
ast->coded_framesize * sub_packet_h > (2 +
(sub_packet_h & 1)) * ast->audio_framesize)
return AVERROR_INVALIDDATA;
if (ast->coded_framesize * sub_packet_h !=
2*ast->audio_framesize) {
@@ -278,12 +279,16 @@ static int
rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
break;
case DEINT_ID_GENR:
if (ast->sub_packet_size <= 0 ||
+ ast->audio_framesize > INT_MAX / sub_packet_h ||
ast->sub_packet_size > ast->audio_framesize)
return AVERROR_INVALIDDATA;
if (ast->audio_framesize % ast->sub_packet_size)
return AVERROR_INVALIDDATA;
break;
case DEINT_ID_SIPR:
+ if (ast->audio_framesize > INT_MAX / sub_packet_h)
sub_packet_h has not been checked for being != 0 here and in the
DEINT_ID_GENR codepath.
Ah, good catch. This also means av_new_packet() is potentially being
called with 0 as size for these two codepaths.
+ return AVERROR_INVALIDDATA;
+ break;
case DEINT_ID_INT0:
case DEINT_ID_VBRS:
case DEINT_ID_VBRF:
@@ -296,7 +301,6 @@ static int
rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
ast->deint_id == DEINT_ID_GENR ||
ast->deint_id == DEINT_ID_SIPR) {
if (st->codecpar->block_align <= 0 ||
- ast->audio_framesize * (uint64_t)sub_packet_h >
(unsigned)INT_MAX ||
ast->audio_framesize * sub_packet_h <
st->codecpar->block_align)
return AVERROR_INVALIDDATA;
if (av_new_packet(&ast->pkt,
ast->audio_framesize *
sub_packet_h) < 0)
Same amount of checks for all three deint ids, and no integer
casting to
prevent overflows.
Since when is a division better than casting to 64bits to perform a
multiplication?
This is done in plenty of places across the codebase to catch the
same
kind of overflows. Does it make any measurable difference even worth
mentioning, especially considering this is read in the header?
All these casts make the code really ugly and harder to read.
Especially things like (unsigned)INT_MAX. So if there are cleaner
solutions, they should be used if possible.
Code needs to not only work, but also be maintainable.
Another option is to just change the type of the RMStream fields,
like so:
diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index fc3bff4859..304984d2b0 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -50,8 +50,8 @@ struct RMStream {
/// Audio descrambling matrix parameters
int64_t audiotimestamp; ///< Audio packet timestamp
int sub_packet_cnt; // Subpacket counter, used while reading
- int sub_packet_size, sub_packet_h, coded_framesize; ///<
Descrambling parameters from container
- int audio_framesize; /// Audio frame size from container
+ unsigned sub_packet_size, sub_packet_h, coded_framesize; ///<
Descrambling parameters from container
+ unsigned audio_framesize; /// Audio frame size from container
int sub_packet_lengths[16]; /// Length of each subpacket
int32_t deint_id; ///< deinterleaver used in audio stream
};
@@ -277,7 +277,7 @@ static int
rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
}
break;
case DEINT_ID_GENR:
- if (ast->sub_packet_size <= 0 ||
+ if (!ast->sub_packet_size ||
ast->sub_packet_size > ast->audio_framesize)
return AVERROR_INVALIDDATA;
if (ast->audio_framesize % ast->sub_packet_size)
@@ -296,7 +296,7 @@ static int
rm_read_audio_stream_info(AVFormatContext *s, AVIOContext *pb,
ast->deint_id == DEINT_ID_GENR ||
ast->deint_id == DEINT_ID_SIPR) {
if (st->codecpar->block_align <= 0 ||
- ast->audio_framesize * (uint64_t)sub_packet_h >
(unsigned)INT_MAX ||
+ ast->audio_framesize * sub_packet_h > INT_MAX ||
ast->audio_framesize * sub_packet_h <
st->codecpar->block_align)
return AVERROR_INVALIDDATA;
if (av_new_packet(&ast->pkt, ast->audio_framesize *
sub_packet_h) < 0)
ast->audio_framesize and sub_packet_h are never bigger than INT16_MAX,
so unless I'm missing something, this should be enough.
In the multiplication ast->coded_framesize * sub_packet_h the first is
read via av_rb32(). Your patch will indeed eliminate the undefined
behaviour (because unsigned), but it might be that the check will now
not trigger when it should trigger because only the lower 32bits are
compared.
ast->coded_framesize is guaranteed to be less than or equal to
ast->audio_framesize, which is guaranteed to be at most INT16_MAX.
True (apart from the bound being UINT16_MAX).
Yes, my bad.
Doesn't fix the
uninitialized data that I mentioned though.
Yet there is a check for coded_framesize being < 0 immediately after it
is read. Said check would be moot with your changes. The problem is that
if its value is not representable as an int, one could set a negative
block_align value based upon it.
With coded_framesize being an int (local variable where the value is
read with avio_rb32()) and ast->coded_framesize being unsigned (context
variable where the value is ultimately stored), the end result after the
< 0 check will be that ast->coded_framesize is at most INT_MAX right
from the beginning, so block_align can't be negative either.
True, the check uses a local int variable.
The issue that started this thread is still open. And even after re-reading
this thread iam not sure what changes to it exactly are requested.
Do you or James remember what exactly you wanted me to do instead of my
initial patch ?
ping
Just push your version. I think i suggested to just change the type of
some variables to unsigned plus some extra checks, but it may not be
worth the extra complexity.
_______________________________________________
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".
