Opened 6 years ago

Closed 6 years ago

#2139 closed defect (fixed)

Out of bound write access when parsing .srt

Reported by: evangelos@… Owned by: eugeni.stepanov@…
Priority: normal Component: libass
Version: HEAD Severity: normal
Keywords: Cc: reimar
Blocked By: Blocking:
Reproduced by developer: Analyzed by developer:

Description

(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.

Attachments (1)

subreader-fix-srt-parsing.patch (2.6 KB) - added by evangelos@… 6 years ago.
Fix from the mpv-player project with tweaked variable names in the first chunk

Download all attachments as: .zip

Change History (6)

comment:1 Changed 6 years ago by evangelos@…

  • Cc Reimar.Doeffinger@… added

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!

Changed 6 years ago by evangelos@…

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

comment:2 Changed 6 years ago by evangelos@…

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by 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

comment:5 Changed 6 years ago by evangelos@…

  • Resolution set to fixed
  • Status changed from new to closed

(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.

Note: See TracTickets for help on using tickets.