flac dev - Jun 2015 - about libFLAC/metadata_object.c

There are two functions:

     static void
vorbiscomment_entry_array_delete_(FLAC__StreamMetadata_VorbisComment_Entry
*object_array, unsigned num_comments);

and

     static void
cuesheet_track_array_delete_(FLAC__StreamMetadata_CueSheet_Track *object_array,
unsigned num_tracks);


which first dereference object_array and only then check it for NULL:

             free(object_array[i].indices);
         ...
         ...
         if(0 != object_array)
             free(object_array);

Currently the condition "if(0 != object_array)" is not necessary
because libFLAC
always checks the first argument of these static functions before calling them.

Also the check was added in the commit "fix null pointer handling in
metadata object routines":
http://git.xiph.org/?p=flac.git;a=commitdiff;h=7294309ba6bc642a5907f4246b7e085c93f23144
The function wasn't static back then.

So it make sense either to remove this unnecessary check or to make it much
earlier,
before dereferencing.

lvqcl <lvqcl.mail at gmail.com> wrote:
> There are two functions:
>
>      static void
vorbiscomment_entry_array_delete_(FLAC__StreamMetadata_VorbisComment_Entry
*object_array, unsigned num_comments);
>
> and
>
>      static void
cuesheet_track_array_delete_(FLAC__StreamMetadata_CueSheet_Track *object_array,
unsigned num_tracks);
>
>
> which first dereference object_array and only then check it for NULL:
>
>              free(object_array[i].indices);
>          ...
>          ...
>          if(0 != object_array)
>              free(object_array);
>
> Currently the condition "if(0 != object_array)" is not necessary
because libFLAC
> always checks the first argument of these static functions before calling
them.

Another example is in static_metadata_clear() in src\flac\encode.c:

     for(i = 0; i < m->num_metadata; i++)
         if(m->needs_delete[i])
             FLAC__metadata_object_delete(m->metadata[i]);

     if(m->metadata)
         free(m->metadata);
     if(m->needs_delete)
         free(m->needs_delete);

Here m->metadata and m->needs_delete were first dereferenced
and then (unnecessarily) checked against NULL.

lvqcl wrote:
> Another example is in static_metadata_clear() in src\flac\encode.c:
> 
>      for(i = 0; i < m->num_metadata; i++)
>          if(m->needs_delete[i])
>              FLAC__metadata_object_delete(m->metadata[i]);
> 
>      if(m->metadata)
>          free(m->metadata);
>      if(m->needs_delete)
>          free(m->needs_delete);
> 
> Here m->metadata and m->needs_delete were first dereferenced
> and then (unnecessarily) checked against NULL.
Its actually deeper than that. Passing a NULL pointer to the free() function
is perfectly safe as that is specified (by the C standard) to be a NO-OP.

Currently working on a patch to remove all these from metadata_object.c.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/