This is pretty awful code. I saw "buffer[size] = 0" and assumed immediately that was a past-the-end write, but they actually allocate size + 1 bytes. Urgh.

Next they introduced a check for strings shorter than 6 bytes because that's the shortest possible valid string. Why not just check for a valid encoding in the first place? There are too many implicit assumptions about the data going on here and not enough actual validation.

This entire module needs scrapping and rewriting with a proper FSM/parser generator.

And why is an MPEG4 metadata decoder directly handling UTF anyway?

Wow. Integer overflows and underflows all over the place. It's almost like no-one's ever even taken a fuzzer to this before, which would surprise me given that Google is usually relatively security-concious.

This code is probably written by embedded software engineers like myself. No one knows anything about security, and certainly hasn't heard of a fuzzer. If the code works, it ships.

The other parts of Android written in C++ are also a horror show, though I think they have gotten better recently.

And don't even talk about the proprietary HAL blobs or kernel modules. Unspeakable things happen there. And of course, libstagefright talks directly to these.

Or it's almost like it was done on purpose as a backdoor.

Is it normal to have 2000+ lines class?

Looks like an instant code smell to me.

I've seen 10000+ line classes in the corporate world

I've seen longer classes in the corporate world too. Such code was always buggy and hard to maintain.

Why? I know one should avoid "god objects" but if the class does a lot, many lines are needed, surely?

The whole idea of avoiding "god objects" is exactly in preventing classes from doing a lot.

https://en.wikipedia.org/wiki/Single_responsibility_principl...

Hahahahah! Good one, dennisgorelik!