OlivierGrenoble / mp4v2

Automatically exported from code.google.com/p/mp4v2
Other
0 stars 0 forks source link

Crash when reading a file with an empty metadata atom #131

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?

1. Construct an m4a file with a moov/udta/meta/ilst/©alb with nothing in it 
(that is, an 8-byte atom with just the length "8" and the name "©alb", no data 
child).
2. Run mp4info on the file.

What is the expected output? What do you see instead?

It should treat this as having an empty album. Instead, it crashes in 
fetchString.

What version of the product are you using? On what operating system?

A default clean build of r479 on Mac 10.7.3 x86_64 with Xcode 4.3.1 reproduces 
the problem, and so does a MacPorts universal build (running with either arch) 
on a 10.6.6 machine with Xcode 3.2.1); it was originally discovered with a 
custom build of r453 on some Windows platform that I don't know the details of.

Please provide any additional information below.

I've gotten a few m4a files like this from users. I don't know what tool 
created them. But it's not hard to hack one up in hexl-mode.

iTunes can import the files, display them (showing "Unknown Album"), and edit 
them (including setting the album to something else) with no problems. (Of 
course if I set the album and then clear it out, the atom disappears.)

AtomicParsley -t handles the tracks fine, just not listing an album.

AtomicParsley -T dumps the structure fine, like this:

     Atom udta @ 41301 of size: 2425, ends @ 43726
         Atom meta @ 41309 of size: 2417, ends @ 43726
             Atom hdlr @ 41321 of size: 34, ends @ 41355
             Atom ilst @ 41355 of size: 459, ends @ 41814
                 Atom ©nam @ 41363 of size: 48, ends @ 41411
                     Atom data @ 41371 of size: 40, ends @ 41411
                 Atom ©ART @ 41411 of size: 27, ends @ 41438
                     Atom data @ 41419 of size: 19, ends @ 41438
                 Atom ©alb @ 41438 of size: 8, ends @ 41446
                 Atom trkn @ 41446 of size: 32, ends @ 41478
                     Atom data @ 41454 of size: 24, ends @ 41478

Hex-dumping around the area gives:

0000a180: 6c00 0000 0000 0000 0000 0000 0001 cb69  l..............i
0000a190: 6c73 7400 0000 30a9 6e61 6d00 0000 2864  lst...0.nam...(d
0000a1a0: 6174 6100 0000 0100 0000 0041 7265 2059  ata........Are Y
0000a1b0: 6f75 2047 6f6e 6e61 2042 6520 4d79 2047  ou Gonna Be My G
0000a1c0: 6972 6c00 0000 1ba9 4152 5400 0000 1364  irl.....ART....d
0000a1d0: 6174 6100 0000 0100 0000 004a 6574 0000  ata........Jet..
0000a1e0: 0008 a961 6c62 0000 0020 7472 6b6e 0000  ...alb... trkn..
0000a1f0: 0018 6461 7461 0000 0000 0000 0000 0000  ..data..........
0000a200: 0020 0000 0000 0000 0026 a963 6d74 0000  . .......&.cmt..

From what I can tell, the libmp4v2 parsing code builds up a map where "©alb" 
is mapped to an MP4ItmfItem with a dataList whose size is 0, so, the line 
(Tags.cpp:616, in r479) where fetchString tries to copy 
f->second->dataList.elements[0] crashes.

The comments for MP4ItmfItem say "size is always >= 1". If that's a legitimate 
invariant, then the error is on the parser side (which I haven't tracked down 
yet). Alternatively, if it's an incorrect assumption, then the error is in that 
comment, and in all of the fetch* methods in Tags.cpp (which just need, e.g., 
s/f == cim.end()/f == cim.end() || !f->second->dataList.size/g).

Original issue reported on code.google.com by abarn...@gmail.com on 19 Apr 2012 at 11:56

GoogleCodeExporter commented 8 years ago
Oops, looks like this is a dup of #125, and the patch from that bug (which 
appears to be committed in r490) fixes the problem.

However, itmf_generic.h:126 still says "size is always >= 1", which isn't true, 
so it should be changed.

Original comment by abarn...@gmail.com on 20 Apr 2012 at 12:17

GoogleCodeExporter commented 8 years ago
Yeah, should hopefully be fixed, although it's still not clear to me if there's 
an error on the parser side, or if it's just screwy itmf data in the MP4 file.  
So if you have any ideas, let me know.  

I removed the size comment in itmf_generic.h, since that isn't the case for 
whatever reason.

Original comment by kid...@gmail.com on 20 May 2012 at 8:05

GoogleCodeExporter commented 8 years ago
Well, it's hard to know what counts as "screwy" and what counts as "legitimate" 
if Apple doesn't document the format. (It's certainly legitimate MPEG 4 
structure, but that doesn't prove anything.) I do know that iTunes, the iPod 
software, and AVFoundation can all read it. I haven't tested to see whether 
AVFoundation can generate it (maybe by setting an AVMutableMetadataItem's value 
to nil or NSNull and saving it into an export session?), which I guess would be 
the real test. I'll try various ways to do it when I get a chance.

Original comment by abarn...@gmail.com on 20 May 2012 at 9:45