shithub: opusfile

Download patch

ref: 529bd01294c13a62f13fc4fe882eec175fc0bebc
parent: 0adf7303ecf08776205e85b5b65ee8bd3419945c
author: Timothy B. Terriberry <tterribe@xiph.org>
date: Tue May 23 11:48:40 EDT 2017

Fix uninitialized free in tag parsing.

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

--- a/src/info.c

+++ b/src/info.c

@@ -107,26 +107,33 @@

   char   **user_comments;

   int     *comment_lengths;

   int      cur_ncomments;

-  char    *binary_suffix_data;

-  int      binary_suffix_len;

   size_t   size;

   if(OP_UNLIKELY(_ncomments>=(size_t)INT_MAX))return OP_EFAULT;

   size=sizeof(*_tags->comment_lengths)*(_ncomments+1);

   if(size/sizeof(*_tags->comment_lengths)!=_ncomments+1)return OP_EFAULT;

   cur_ncomments=_tags->comments;

+  /*We only support growing.

+    Trimming requires cleaning up the allocated strings in the old space, and

+     is best handled separately if it's ever needed.*/

+  OP_ASSERT(_ncomments>=cur_ncomments);

   comment_lengths=_tags->comment_lengths;

-  binary_suffix_len=comment_lengths==NULL?0:comment_lengths[cur_ncomments];

   comment_lengths=(int *)_ogg_realloc(_tags->comment_lengths,size);

   if(OP_UNLIKELY(comment_lengths==NULL))return OP_EFAULT;

-  comment_lengths[_ncomments]=binary_suffix_len;

+  if(_tags->comment_lengths==NULL){

+    OP_ASSERT(cur_ncomments==0);

+    comment_lengths[cur_ncomments]=0;

+  }

+  comment_lengths[_ncomments]=comment_lengths[cur_ncomments];

   _tags->comment_lengths=comment_lengths;

   size=sizeof(*_tags->user_comments)*(_ncomments+1);

   if(size/sizeof(*_tags->user_comments)!=_ncomments+1)return OP_EFAULT;

-  user_comments=_tags->user_comments;

-  binary_suffix_data=user_comments==NULL?NULL:user_comments[cur_ncomments];

   user_comments=(char **)_ogg_realloc(_tags->user_comments,size);

   if(OP_UNLIKELY(user_comments==NULL))return OP_EFAULT;

-  user_comments[_ncomments]=binary_suffix_data;

+  if(_tags->user_comments==NULL){

+    OP_ASSERT(cur_ncomments==0);

+    user_comments[cur_ncomments]=NULL;

+  }

+  user_comments[_ncomments]=user_comments[cur_ncomments];

   _tags->user_comments=user_comments;

   return 0;

 }