Message ID | 20200920161657.1979613-1-andreas.rheinhardt@gmail.com |
---|---|
State | Accepted |
Commit | 5b33f523d736a11c9db88f998708c6f67691f445 |
Headers | show |
Series | [FFmpeg-devel] avformat/aaxdec: Fix potential integer overflow | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On Sun, Sep 20, 2020 at 06:16:57PM +0200, Andreas Rheinhardt wrote: > The AAX demuxer reads a 32bit number containing the amount of entries > of an array and stores it in an uint32_t. Yet when iterating over this > array, a loop counter of type int is used. This leads to undefined > behaviour if the amount of entries is not in the range of int; to avoid > this, it is generally good to use the same type for the loop counter as > for the variable it is compared to. This is done in one of the two loops > affected by this. > > In the other loop, the undefined behaviour can begin even earlier: Here > the loop counter is multiplied by an uint16_t which can overflow as soon > as the loop counter is > 2^15. Using an unsigned type would avoid the > undefined behaviour, but truncation would still be possible, so use an > uint64_t. > > Also use an uint32_t for a variable containing an index in said array. > > This fixes Coverity issue #1466767. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > This is untested as I could only find out that this is a gaming format. > lgtm > libavformat/aaxdec.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/libavformat/aaxdec.c b/libavformat/aaxdec.c > index cfd2e10a15..3db6e9bc6d 100644 > --- a/libavformat/aaxdec.c > +++ b/libavformat/aaxdec.c > @@ -51,7 +51,7 @@ typedef struct AAXContext { > int64_t strings_size; > char *string_table; > > - int current_segment; > + uint32_t current_segment; > > AAXColumn *xcolumns; > AAXSegment *segments; > @@ -239,7 +239,7 @@ static int aax_read_header(AVFormatContext *s) > flag = a->xcolumns[c].flag; > col_offset = a->xcolumns[c].offset; > > - for (int r = 0; r < a->nb_segments; r++) { > + for (uint64_t r = 0; r < a->nb_segments; r++) { > if (flag & COLUMN_FLAG_DEFAULT) { > data_offset = a->schema_offset + col_offset; > } else if (flag & COLUMN_FLAG_ROW) { > @@ -330,7 +330,7 @@ static int aax_read_packet(AVFormatContext *s, AVPacket *pkt) > > pkt->pos = avio_tell(pb); > > - for (int seg = 0; seg < a->nb_segments; seg++) { > + for (uint32_t seg = 0; seg < a->nb_segments; seg++) { > int64_t start = a->segments[seg].start; > int64_t end = a->segments[seg].end; > > -- > 2.25.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".
diff --git a/libavformat/aaxdec.c b/libavformat/aaxdec.c index cfd2e10a15..3db6e9bc6d 100644 --- a/libavformat/aaxdec.c +++ b/libavformat/aaxdec.c @@ -51,7 +51,7 @@ typedef struct AAXContext { int64_t strings_size; char *string_table; - int current_segment; + uint32_t current_segment; AAXColumn *xcolumns; AAXSegment *segments; @@ -239,7 +239,7 @@ static int aax_read_header(AVFormatContext *s) flag = a->xcolumns[c].flag; col_offset = a->xcolumns[c].offset; - for (int r = 0; r < a->nb_segments; r++) { + for (uint64_t r = 0; r < a->nb_segments; r++) { if (flag & COLUMN_FLAG_DEFAULT) { data_offset = a->schema_offset + col_offset; } else if (flag & COLUMN_FLAG_ROW) { @@ -330,7 +330,7 @@ static int aax_read_packet(AVFormatContext *s, AVPacket *pkt) pkt->pos = avio_tell(pb); - for (int seg = 0; seg < a->nb_segments; seg++) { + for (uint32_t seg = 0; seg < a->nb_segments; seg++) { int64_t start = a->segments[seg].start; int64_t end = a->segments[seg].end;
The AAX demuxer reads a 32bit number containing the amount of entries of an array and stores it in an uint32_t. Yet when iterating over this array, a loop counter of type int is used. This leads to undefined behaviour if the amount of entries is not in the range of int; to avoid this, it is generally good to use the same type for the loop counter as for the variable it is compared to. This is done in one of the two loops affected by this. In the other loop, the undefined behaviour can begin even earlier: Here the loop counter is multiplied by an uint16_t which can overflow as soon as the loop counter is > 2^15. Using an unsigned type would avoid the undefined behaviour, but truncation would still be possible, so use an uint64_t. Also use an uint32_t for a variable containing an index in said array. This fixes Coverity issue #1466767. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- This is untested as I could only find out that this is a gaming format. libavformat/aaxdec.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)