Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible integer overflow in StreamInfo::set_duration() #214

Closed
sylt opened this issue Mar 22, 2017 · 2 comments
Closed

Possible integer overflow in StreamInfo::set_duration() #214

sylt opened this issue Mar 22, 2017 · 2 comments
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly

Comments

@sylt
Copy link
Contributor

sylt commented Mar 22, 2017

System info

Operating System: Ubuntu 14.04 trusty
Shaka Packager Version: c223bc9-release

Issue and steps to reproduce the problem

When I tried to segment a WebM file with a long duration, I got a mediaPresentationDuration attribute in my MPD manifest that was about int64_t(-1), but written as floating point (it was something like 1.844e+13).

According to libwebm (using the mkvparser_sample program), my WebM stream header looks like this:

	 libwebm version: 1.0.0.30
		    EBML Header
	EBML Version		: 1
	EBML MaxIDLength	: 4
	EBML MaxSizeLength	: 8
	Doc Type		: webm
	Pos			: 43

		   Segment Info
	TimeCodeScale		: 1000000 
	Duration		: 4096000000000
	Duration(secs)		: 4096.000
	Track Name		: NULL
	Muxing App		: Lavf57.41.100
	Writing App		: Lavf57.41.100
	Position(Segment)	: 55
	Size(Segment)		: 51598901

This duration will be (correctly) parsed by the WebM parser in the shaka-packager, but in WebMMediaParser::ParseInfoAndTracks(), where we should set the parsed duration to our AudioStreamInfo or VideoStreamInfo using StreamInfo::set_duration(), there will in this case be an integer overflow.

The reason is that the duration accepted by StreamInfo::set_duration() is int, as opposed to int64_t or uint64_t (which by the way is the internal representation for the duration in the StreamInfo class). Once I exchanged that to uint64_t, my mediaPresentationDuration was alright again.

Thank you for having a look! :)

@kqyang
Copy link
Contributor

kqyang commented Mar 22, 2017

That is definitely a bug. Thanks for catching it! Since you have a fix already, would you like to submit a pull request to fix the issue? See https://github.com/google/shaka-packager/blob/master/CONTRIBUTING.md on how to contribute.

@kqyang kqyang added the type: bug Something isn't working correctly label Mar 22, 2017
sylt added a commit to sylt/shaka-packager that referenced this issue Mar 23, 2017
The issue could lead to the MPD attribute mediaPresentationDuration being
wrongly generated for WebM streams with a duration longer than INT32_MAX.

The root-cause was that StreamInfo::set_duration() accepted an int instead of a
uint64_t. This seems like a pure typo, since StreamInfo already uses a uint64_t
internally for representing duration.
@kqyang
Copy link
Contributor

kqyang commented Mar 23, 2017

@sylt Thanks for fixing the bug!

@kqyang kqyang closed this as completed Mar 23, 2017
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 19, 2018
@shaka-project shaka-project locked and limited conversation to collaborators Apr 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

3 participants