| Message ID | 20190202021659.21933-1-chcunningham@chromium.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 01, 2019 at 06:16:59PM -0800, chcunningham wrote: > Unsigned types match the isobmff spec. > --- > libavformat/isom.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/libavformat/isom.h b/libavformat/isom.h > index e629663949..8e0d8355b3 100644 > --- a/libavformat/isom.h > +++ b/libavformat/isom.h > @@ -59,9 +59,9 @@ typedef struct MOVStts { > } MOVStts; > > typedef struct MOVStsc { > - int first; > - int count; > - int id; > + unsigned int first; > + unsigned int count; > + unsigned int id; > } MOVStsc; Is this change needed by some valid file ? The change taken on its own is probably correct but its increasing the range of values without actually adding support for that thus quite possibly introducing bugs. In case of the id, we use signed int for the entries this indexes into, so the extended range is not going to work. also wonder if billions of STSD entries in a stream will work when more than 1 cause problems already. Another obvious issue is that currently we scan this table for negative values and eliminate them but when this is made unsigned suddenly larger values pass through. This has the potential to introduce integer overflow bugs in later stages. More so unsigned overflows dont show up in asan and these first/count values might affect array indexes iam not saying theres a bug here just that its the set of circunstances that is fertile for such bugs. variables related to indexes or counts always require extra scrutiny when their type is changed Thanks [...]
On Sat, Feb 2, 2019 at 3:37 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > Is this change needed by some valid file ? > No, just a drive by fix since I happened to be looking at these types and the spec. > The change taken on its own is probably correct but its increasing the > range > of values without actually adding support for that thus quite possibly > introducing bugs. > > In case of the id, we use signed int for the entries this indexes into, > so the extended range is not going to work. also wonder if billions > of STSD entries in a stream will work when more than 1 cause problems > already. > > Another obvious issue is that currently we scan this table for negative > values and eliminate them but when this is made unsigned suddenly > larger values pass through. This has the potential to introduce > integer overflow bugs in later stages. More so unsigned overflows dont > show up in asan and these first/count values might affect array indexes > iam not saying theres a bug here just that its the set of circunstances > that is fertile for such bugs. > > variables related to indexes or counts always require extra scrutiny > when their type is changed > I really appreciate the scrutiny. Given I don't have a file that needs this, happy to abandon this patch. Chris
diff --git a/libavformat/isom.h b/libavformat/isom.h index e629663949..8e0d8355b3 100644 --- a/libavformat/isom.h +++ b/libavformat/isom.h @@ -59,9 +59,9 @@ typedef struct MOVStts { } MOVStts; typedef struct MOVStsc { - int first; - int count; - int id; + unsigned int first; + unsigned int count; + unsigned int id; } MOVStsc; typedef struct MOVElst {