#377 vorbiscomment_set_entry breaks API contract

1.3.0
closed-fixed
Erik
libFLAC (57)
8
2014-04-06
2011-03-08
Casey
No

Passing a null terminated zero length string to FLAC__metadata_object_vorbiscomment_set_comment, with copy true is incorrectly handled.

The copy parameter is ignored, and instead the source pointer is assigned directly to the vorbis comment block, and the function returns TRUE. This violates the API, since if TRUE is returned, the copy is guaranteed, which is not the case.

The problem is traced to vorbiscomment_set_entry() method in metadat_object.c:

if\(0 \!= src->entry && src->length > 0\) \{

.....
}
else {
/* the src is null */
*dest = *src;
}

If if statement is negative, because the length is 0, however the pointer is valid, and assigned directly to the dest pointer, and the method returns TRUE.

Discussion

  • Casey
    Casey
    2011-03-08

    Priority is elevated because this bug will likely lead to a memory corruption in the client code.

     
  • Casey
    Casey
    2011-03-08

    • priority: 5 --> 8
     
  • Erik
    Erik
    2014-03-23

    • status: open --> closed-fixed
    • assigned_to: Erik
    • Group: --> 1.3.0
     
  • Erik
    Erik
    2014-03-23

    Fixed in commit:

    commit 49d9d742e2f84295a0dca8ad9a88c94c9a57b95d
    Author: Erik de Castro Lopo <erikd@mega-nerd.com>
    Date:   Sun Mar 23 21:40:54 2014 +1100
    
    metadata_object.c : Fix handling of zero length vorbis comment string.
    
    Previously if a zero length string was passed in, the pointer would be
    stored regardless of the copy parameter. If the original source pointer
    was reassigned to something else bad things could happen.