Skip to content

Fix NULL strcmp and misaligned load on malformed tracks - #101

Open
1820893135-pixel wants to merge 1 commit into
TechSmith:mainfrom
1820893135-pixel:fix-null-strcmp-and-misaligned-load
Open

Fix NULL strcmp and misaligned load on malformed tracks#101
1820893135-pixel wants to merge 1 commit into
TechSmith:mainfrom
1820893135-pixel:fix-null-strcmp-and-misaligned-load

Conversation

@1820893135-pixel

Copy link
Copy Markdown

Fixes the two undefined behaviors reported in #99.

1. NULL pointer passed to strcmp (MP4File::GenerateTracks)

if (!strcmp(pTypeProperty->GetValue(), MP4_HINT_TRACK_TYPE)) {

A malformed trak/mdia/hdlr can leave the handlerType string property with a NULL value, so strcmp(NULL, ...) is UB. UBSan reports null pointer passed as argument 1, which is declared to never be null. Guard with a NULL check first:

const char* handlerType = pTypeProperty->GetValue();
if (handlerType && !strcmp(handlerType, MP4_HINT_TRACK_TYPE)) {

2. Misaligned load in STRTOINT32/INT32TOSTR

return MP4V2_NTOHL(*(uint32_t *)s);

When MP4V2_INTSTRING_ALIGNMENT is not defined, STRTOINT32 dereferences *(uint32_t*)s. Atom-type strings reached through ATOMID() (e.g. in MP4Atom::ReadChildAtoms) are not guaranteed 4-byte aligned, so this is a misaligned load. The fix makes the memcpy path unconditional (it was already the ARM-safe path), avoiding the UB on all platforms:

uint32_t tmp;
memcpy( &tmp, s, sizeof(tmp) );
return MP4V2_NTOHL( tmp );

INT32TOSTR is made consistent (always memcpy).

Verified: the reproducer from #99 triggers null pointer passed as argument 1 on the unfixed build; with this change it parses cleanly (exit 0, no sanitizer report) under -fsanitize=address,undefined -fno-sanitize-recover=all.

Fixes #99.

Two related undefined behaviors when parsing malformed MP4 tracks:

- MP4File::GenerateTracks() calls strcmp(pTypeProperty->GetValue(),
  MP4_HINT_TRACK_TYPE) without checking the handlerType string for NULL.
  A malformed trak/mdia/hdlr can leave the string property with a NULL
  value, so strcmp(NULL, ...) is UB (UBSan: null pointer passed as
  argument 1). Guard with a NULL check first.
- STRTOINT32()/INT32TOSTR() dereference *(uint32_t*)s when
  MP4V2_INTSTRING_ALIGNMENT is not defined. Atom-type strings reached
  through ATOMID() are not guaranteed 4-byte aligned, so the cast is a
  misaligned load/store. Always copy through a local with memcpy(),
  matching the existing ARM-safe path.

Fixes TechSmith#99.
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

Successfully merging this pull request may close these issues.

[Bug]Undefined behavior on malformed MP4: null pointer passed to strcmp in MP4File::GenerateTracks (and misaligned load in STRTOINT32) (CWE-476)

1 participant