Opened 7 years ago

Closed 7 years ago

#559 closed defect (fixed)

Crash due to memory corruption when importing subtitles [include fix]

Reported by: Timac2 Owned by: astrange
Priority: normal Milestone: 1.2.2
Component: Subtitles Version: 1.2
Severity: normal Keywords:
Cc:

Description

The code to import subtitles (SubImport?.mm) has a memory corruption which can led to a crash.

Steps to reproduce:
1- Import a movie with subtitles (for example .ass file)

Result:
There is a memory corruption in the function ParseSubTime? which can led to a random crash. The sscanf call in ParseSubTime? writes a NULL character on the stack. Depending on the stack, this might led to a crash.

The ParseSubTime? function contains the following code:

char separator;
if (sscanf(time,"%u:%u:%u%[,.:]%u",&hour,&minute,&second,&separator,&subsecond) < 5)

return 0;

You use "%[,.:]" to parse the separator but the separator variable is a char. According to the sscanf documentation (see man sscanf), "the next pointer must be a pointer to char, and there must be enough room for all the characters in the string, plus a terminating NUL character.". In Perian source code, this is not the case and led to a memory corruption.

My suggested fix is to use "%c" instead of "%[,.:]" (and check the separator):

if (sscanf(time,"%u:%u:%u%[,.:]%u",&hour,&minute,&second,&separator,&subsecond) < 5)

return 0;

by

if (sscanf(time,"%u:%u:%u%c%u",&hour,&minute,&second,&separator,&subsecond) < 5
(separator != ',' && separator != '.' && separator != ':'))

return 0;

Attachments (1)

ParseSubTimeFix.txt (513 bytes) - added by Timac2 7 years ago.
Suggested fix

Download all attachments as: .zip

Change History (5)

Changed 7 years ago by Timac2

Suggested fix

comment:1 Changed 7 years ago by astrange

  • Milestone set to 1.2.2
  • Owner set to astrange
  • Status changed from new to assigned

comment:2 Changed 7 years ago by astrange

  • Milestone 1.2.2 deleted

I would certainly have expected a crash by now, so this is probably writing to an unused area of the stack. The fix is OK, I'll apply before releasing.

comment:3 Changed 7 years ago by astrange

  • Milestone set to 1.2.2

comment:4 Changed 7 years ago by astrange

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

(In [1405]) Fix scanf format string

%[...] wants a two-byte string, not a char. Possible crash, but I've never
seen it in practice.

Fixes #559.

Patch by Timac2

Note: See TracTickets for help on using tickets.