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

potential undefined behavior in OpenMP4Source() #102

Closed
invd opened this issue Jul 9, 2020 · 3 comments
Closed

potential undefined behavior in OpenMP4Source() #102

invd opened this issue Jul 9, 2020 · 3 comments

Comments

@invd
Copy link

invd commented Jul 9, 2020

In OpenMP4Source(), the following three variables are defined as signed 32 bit integers:

int32_t segment_duration; //integer that specifies the duration of this edit segment in units of the movie�s time scale.
int32_t segment_mediaTime; //integer containing the starting time within the media of this edit segment(in media timescale units).If this field is set to �1, it is an empty edit.The last edit in a track should never be an empty edit.Any difference between the movie�s duration and the track�s duration is expressed as an implicit empty edit.
int32_t segment_mediaRate; //point number that specifies the relative rate at which to play the media corresponding to this edit segment.This rate value cannot be 0 or negative.

However, it appears that

  • 32 bit of data is written into them via fread() and values are shifted with <<24 via the BYTESWAP32 macro
  • the represented values are (likely) only supposed to be positive

This can lead to undefined behaviour. Switching the int32_t to uint32_t might be a solution, but this should be checked by someone that is more familiar with the code.

@dnewman-gpsw
Copy link
Collaborator

dnewman-gpsw commented Jul 10, 2020

From the Quicktime spec, one of the fields can be -1. From the Quicktime spec "If this field is set to –1, it is an empty edit." Safer to test of 0xffffffff.

@invd
Copy link
Author

invd commented Jul 10, 2020

Some debugging information: the runtime error: left shift of 255 by 24 places cannot be represented in type 'int' is present in the following three lines:

segment_duration = BYTESWAP32(segment_duration); // in MP4 clock base
segment_mediaTime = BYTESWAP32(segment_mediaTime); // in trak clock base
segment_mediaRate = BYTESWAP32(segment_mediaRate); // Fixed-point 65536 = 1.0X

In my opinion, value checks would have to be applied before the swapping to prevent the undefined behaviour from being reachable.

Also, note that the offset calculation can potentially overflow:
runtime error: signed integer overflow: -2097149565 + -402652929 cannot be represented in type 'int'

mp4->trak_edit_list_offsets[mp4->trak_num] += segment_duration; //samples are delay, data starts after presentation time zero.

@dnewman-gpsw
Copy link
Collaborator

dnewman-gpsw commented Aug 20, 2020

Good feedback, fixed in the current develop branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants