Out of bound write access when parsing .srt

[evangelos@…]

(Shamelessly stealing the title and description from the mpv-player project's commit that fixes this issue. :P)

==============
The breakage was relatively subtle: it set all hour components to 0, while everything else was parsed successfully.

But the problem is really that sscanf wrote 1 byte past the sep
variable (or more, for invalid/specially prepared input). The %..
format specifier is unbounded. Fix that by letting sscanf drop the
parsed contents with "*", and also make it skip only one input
character by adding "1" (=> "%*1[...").
==============

This issue affects .srt subtitles when libass is used and mplayer is built with GCC 4.8. (I have tested with mplayer r35920.)

Please see the following pages for more information:

​https://github.com/mpv-player/mpv/issues/68
​https://github.com/mpv-player/mpv/commit/d98e61ea438db

Thanks.

[evangelos@…]

Hi Reimar,

Sorry for manually adding you to the CC list but I don't know whether
Evgeniy is active or not; his last commit seems to have been in 2009.

I'll attach the patch we use in Arch Linux and seems to work fine with GCC 4.8; it's the same patch from the mpv-player project but I changed the variable names in the first chunk so that it applies cleanly to mplayer.

I'd like to get this committed if possible.

Thanks!

[evangelos@…]

Fix from the mpv-player project with tweaked variable names in the first chunk

[reimar]

I believe I fixed in in r36285.
I admit I made the questionable decision of still allowing more than one :., etc.
But note: I found one additional chunk with that issue, I presume mpv will get this fix automatically when updating? Otherwise it might be good to inform him about it.

[evangelos@…]

(In reply to comment #3)

I believe I fixed in in r36285.

Thanks for the quick resolution; I'll try to verify the fix soon.

I admit I made the questionable decision of still allowing more than one :.,
etc.

I think %*1[,.:] might be a better choice here; I wouldn't expect .srt files to use more than a single decimal mark before the milliseconds value.

Unless you've encountered such weird .srt files in the wild, YAGNI⁰ probably applies here and I'd be in favor of using %*1[,.:]. :P

But note: I found one additional chunk with that issue, I presume mpv will
get this fix automatically when updating? Otherwise it might be good to
inform him about it.

Nice catch. :)

It appears that mpv is a fork of mplayer2.¹ They found and later fixed the additional scanf call through a commit in mplayer2.²³

⁰ ​http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it
¹ ​http://en.wikipedia.org/wiki/MPlayer#Forks
² ​https://github.com/mpv-player/mpv/commit/2cd1a8286b
³ ​http://git.mplayer2.org/mplayer2/commit/?id=5cb9aac

[evangelos@…]

(In reply to comment #4)

(In reply to comment #3)

I believe I fixed in in r36285.

Thanks for the quick resolution; I'll try to verify the fix soon.

Subtitles work fine with r36285, thanks again.