[FFmpeg-devel] avformat/isom.h: use usnigned types in MOVStsc

Submitted by chcunningham@chromium.org on Feb. 2, 2019, 2:16 a.m.

Details

Message ID 20190202021659.21933-1-chcunningham@chromium.org
State New
Headers show

Commit Message

chcunningham@chromium.org Feb. 2, 2019, 2:16 a.m.
Unsigned types match the isobmff spec.
---
 libavformat/isom.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Michael Niedermayer Feb. 2, 2019, 11:37 a.m.
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

[...]
chcunningham@chromium.org Feb. 4, 2019, 9:48 p.m.
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

Patch hide | download patch | download mbox

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 {