Skip to content
  • Timothy B. Terriberry's avatar
    Fix uninitialized free in tag parsing. · 529bd012
    Timothy B. Terriberry authored
    The tags have two possible representations of the binary suffix
     data.
    An implicit one: if comment_lengths and user_comments are NULL,
     then the length of the binary suffix data is 0 and the pointer to
     that data is also implicitly taken to be NULL.
    And an explicit one: if comment_lengths and user_comments are
     non-NULL, then comment_lengths[comments] and
     user_comments[comments] contains the length and pointer to the
     binary suffix data (where "comments" is the number of user
     comments).
    
    The implicit one allows us to initialize a tags struct without
     doing any allocations, which ensures it always succeeds.
    op_tags_ensure_capacity() may upgrade the implicit representation
     to the explicit representation.
    However, when doing so, it stores the implicit (0, NULL) values
     in their new explicit locations at the end of the array assuming
     the requested capacity will become the new size.
    If the caller of op_tags_ensure_capacity() then fails before
     enlarging the array to that size, then comment_lengths and
     user_comments will be non-NULL, but the explicit locations for the
     binary suffix data for the _old_ size may not have been
     initialized.
    
    In particular, in opus_tags_parse_impl(), if there was exactly one
     comment, and any of the three checks at the start of the main loop
     failed, then the explicit locations for the binary suffix data
     would remain uninitialized, and the call to opus_tags_clear() in
     the caller would attempt to free them.
    For counts larger than 1, the extra line marked "Needed by
     opus_tags_clear()" will update the explicit location as the array
     grows, but with a count of 1 this line never gets a chance to run.
    
    This patch fixes the problem by making op_tags_ensure_capacity()
     promote the implicit representation to explicit at _both_ the old
     and new array sizes.
    We could have fixed up opus_tags_parse_impl() to do this instead,
     as suggested in the original bug reports, but doing it this way
     ensures that the tags are always in a valid state when
     op_tags_ensure_capacity() returns.
    
    Thanks to the Google AutoFuzz Team for the report.
    
    Fixes #2324
    Fixes #2325
    529bd012