Commit 529bd012 authored by Timothy B. Terriberry's avatar Timothy B. Terriberry
Browse files

Fix uninitialized free in tag parsing.

The tags have two possible representations of the binary suffix
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

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

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
parent 0adf7303
......@@ -107,26 +107,33 @@ static int op_tags_ensure_capacity(OpusTags *_tags,size_t _ncomments){
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;
if(size/sizeof(*_tags->comment_lengths)!=_ncomments+1)return OP_EFAULT;
/*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.*/
comment_lengths=(int *)_ogg_realloc(_tags->comment_lengths,size);
if(OP_UNLIKELY(comment_lengths==NULL))return OP_EFAULT;
if(size/sizeof(*_tags->user_comments)!=_ncomments+1)return OP_EFAULT;
user_comments=(char **)_ogg_realloc(_tags->user_comments,size);
if(OP_UNLIKELY(user_comments==NULL))return OP_EFAULT;
return 0;
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment