Bug#679826: zsnes: segfaults on start in testing i386

[Ron]

On Tue, Jul 03, 2012 at 09:27:21AM +0200, Fabian Greffrath wrote:
> Am 03.07.2012 01:13, schrieb Ron:
> >Well, no ...  _sanitize_matrix() only gets called if format->matrix is
> >not NULL.  So I don't really see what "more robust" check it could do.
> 
> My first idea was to check if strlen(format->matrix) is within
> reasonable boundaries, before using it to allocate memory.

That won't have helped in this case, since any dereference of the invalid
format->matrix is allowed to explode before our code even touches what is
there, and strlen on something that isn't known to be NUL terminated isn't
safe either.

Since linux overcommits on malloc anyway, the allocation part is really the
least of our worries if something corrupted managed to get that far.  Even
making a string long enough for that to be a problem would be quite tricky
enough on its own without having something else explode on you first.


> >If the caller sets format->matrix to point to an invalid memory location
> >there isn't really anything more that libao can do to validate that.
> >They could set it to &main with more or less equivalent results to leaving
> >it uninitialised, so only the caller is in a position to validate that is
> >sanely set before they pass it to libao.
> 
> Generally, I agree that it's the applications fault to pass a
> pointer-to-garbage to libao. But that's the critical point: If you
> are not in control of the data, you shouldn't use it unseen to
> allocate memory.

Well, that was sort of my point, if you aren't passing a NULL matrix here,
then you really have no excuse not to be in control of that data.  There's
no good reason or excuse to be passing untrusted data there, and you could
probably do "terrible enough" things passing something that was perfectly
legitimate in every technical sense if you didn't sanity check or control
it yourself to meet your app's own definition of what constitutes sane
(as opposed to libao's necessarily broader definition of what is legal).

This didn't blow up because libao was trying to malloc((size_t)-1), it
blew up because dereferencing address 0x5 (which format->matrix pointed to)
wasn't a valid address.  And the only way libao could 'know' that was to
dereference it and have it explode.  It hadn't even got to calling calloc
yet when that happened, it was the strlen dereference where it died, and
even strnlen(format->matrix, 1) would have died the very same way here.

So I still don't see what else we could have checked that would save zsnes
from exploding here, or what else we can check that will save someone else
who passes 'garbage' here.  The only real mystery remaining is why zsnes
hasn't been reported to die like this before now ...

 Ron