[PATCH] Mp4: fixed setting wrong mdat atom size in very rarecases.

[Maxim Dounin]

Hello!

On Mon, Nov 21, 2016 at 10:04:25PM +0800, 胡聪 (hucc) wrote:

> Hi,
> 
> On Mon, Nov 21, 2016 at 9:07 PM +0300, Maxim Dounin wrote:
> 
> >Hello!
> >
> >On Mon, Nov 21, 2016 at 05:40:52PM +0800, 胡聪 (hucc) wrote:
> >
> >> Hi,
> >>
> >> # HG changeset patch
> >> # User hucongcong <hucong.c at foxmail.com>
> >> # Date 1479719580 -28800
> >> #      Mon Nov 21 17:13:00 2016 +0800
> >> # Node ID f43feabe2e45ec102ffbe36428ccb504ff1526d2
> >> # Parent  6a26016e9a138102798a7ec3e74747fbd6018f82
> >> Mp4: fixed setting wrong mdat atom size in very rare cases.
> >>
> >> diff -r 6a26016e9a13 -r f43feabe2e45 src/http/modules/ngx_http_mp4_module.c
> >> --- a/src/http/modules/ngx_http_mp4_module.c  Tue Nov 15 18:11:46 2016 +0300
> >> +++ b/src/http/modules/ngx_http_mp4_module.c  Mon Nov 21 17:13:00 2016 +0800
> >> @@ -1229,7 +1229,7 @@ ngx_http_mp4_update_mdat_atom(ngx_http_m
> >> 
> >>      atom_header = mp4->mdat_atom_header;
> >> 
> >> -    if ((uint64_t) atom_data_size > (uint64_t) 0xffffffff) {
> >> +    if ((uint64_t) atom_data_size > (uint64_t) 0xfffffff7) {
> >>          atom_size = 1;
> >>          atom_header_size = sizeof(ngx_mp4_atom_header64_t);
> >>          ngx_mp4_set_64value(atom_header + sizeof(ngx_mp4_atom_header_t),
> >
> >You may want to elaborate more about what are you trying to fix 
> >here.
> >
> >Current code uses 32-bit atom headers for sizes up to maximum
> >unsigned 32-bit value, 0xffffffff.  And this seems to perfectly
> >agree with the specification.
> 
> Well, atom size is the sum of atom header size and atom data size.
> And the specification says that the first 4 bytes are set to one
> when the atom size is greater than 0xffffffff, which means atom
> header size should be considered when the comparison takes place
> between atom data size and the maximum unsigned 32-bit value.
> We can image that atom_data_size is 0xfffffff9, and the first 4 bytes
> will be set to the sum of sizeof(ngx_mp4_atom_header_t) and 
> atom_data_size, then the error is obvious.

Ah, ok, I see the problem now.  Please clarify things in the 
commit log as well.

It also make sense to use sizeof() explicitly, instead of 
introducing a magic number, e.g.:

@@ -1229,7 +1229,9 @@ ngx_http_mp4_update_mdat_atom(ngx_http_m
 
     atom_header = mp4->mdat_atom_header;
 
-    if ((uint64_t) atom_data_size > (uint64_t) 0xffffffff) {
+    if ((uint64_t) atom_data_size > (uint64_t) 0xffffffff
+                                    - sizeof(ngx_mp4_atom_header_t))
+    {
         atom_size = 1;
         atom_header_size = sizeof(ngx_mp4_atom_header64_t);
         ngx_mp4_set_64value(atom_header + sizeof(ngx_mp4_atom_header_t),


-- 
Maxim Dounin
http://nginx.org/