[RFC] player.c: fix `(error) Common realloc mistake: 'buffer' nulled but not freed upon failure`

  On 20.02.2011 17:53, Klaus Schmidinger wrote:
> On 14.02.2011 15:55, Paul Menzel wrote:
>> Date: Mon, 14 Feb 2011 14:29:48 +0100
>>
>> Output of Cppcheck 1.47 [1], VDR 1.7.16:
>>
>>         Checking ./PLUGINS/src/pictures/player.c...
>>         [./PLUGINS/src/pictures/player.c:9]: (debug) Include file: "vdr/remote.h" not found.
>>         [./PLUGINS/src/pictures/player.c:10]: (debug) Include file: "vdr/tools.h" not found.
>>         [./PLUGINS/src/pictures/player.c:70]: (error) Common realloc mistake: 'buffer' nulled but not freed upon failure
>>         Checking ./PLUGINS/src/pictures/player.c: BIDI...
>>         Checking ./PLUGINS/src/pictures/player.c: DEBUGRINGBUFFERS...
>>         Checking ./PLUGINS/src/pictures/player.c: PLUGIN_NAME_I18N...
>>         Checking ./PLUGINS/src/pictures/player.c: __STL_CONFIG_H...
>>         9/72 files checked 12% done
>>
>> This patch uses the fix for QEMU [2] as a template and is build tested.
>>
>> [1] http://cppcheck.sourceforge.net/
>> [2] http://meego.gitorious.org/qemu-maemo/qemu/commit/29718712eb2e53c09d28f08e39f6514d690f6fd3
>>
>> Signed-off-by: Paul Menzel <paulepanter@users.sourceforge.net>
>> CC: Klaus Schmidinger <Klaus.Schmidinger@tvdr.de>
>> ---
>> Dear VDR folks,
>>
>>
>> please advise if the `break` is enough and what kind of error message
>> would be useful. I would then apply this fix to the whole tree and
>> submit a final patch.
>>
>>
>> Thanks,
>>
>> Paul
>> ---
>>  PLUGINS/src/pictures/player.c |    9 ++++++++-
>>  1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/PLUGINS/src/pictures/player.c b/PLUGINS/src/pictures/player.c
>> index a0123e4..3f87696 100644
>> --- a/PLUGINS/src/pictures/player.c
>> +++ b/PLUGINS/src/pictures/player.c
>> @@ -60,6 +60,7 @@ void cPicturePlayer::Activate(bool On)
>>  
>>  void cPicturePlayer::SetPicture(const char *FileName)
>>  {
>> +  uchar *new_buffer;
>>    int f = open(FileName, O_RDONLY);
>>    if (f >= 0) {
>>       for (;;) {
>> @@ -67,7 +68,13 @@ void cPicturePlayer::SetPicture(const char *FileName)
>>           if (length > 0) {
>>              if (length >= size) {
>>                 size = size * 3 / 2;
>> -               buffer = (uchar *)realloc(buffer, size);
>> +               new_buffer = (uchar *)realloc(buffer, size);
>> +               if (new_buffer == NULL) {
>> +                  free(buffer);
>> +                  LOG_ERROR_STR("realloc");
>> +                  break;
>> +               }
>> +               buffer = new_buffer;
>>                 lseek(f, 0, SEEK_SET);
>>                 continue;
>>                 }
> 
> Just doing a 'break' here will leave 'buffer' pointing to its initial
> location, and therefore it will be free'd again in cPicturePlayer::~cPicturePlayer().
> 
> Since there are many places in VDR where a realloc() is done this way,
> and they probably should all be revisited with this in mind, I'm going
> to put a REALLOC() function into tools.h, as in
> 
> template <class T> T REALLOC(T Var, size_t Size)
> {
>   T p = (T)realloc(Var, Size);
>   if (!p)
>      free(Var);
>   return p;
> }
> 
> and use it in cPicturePlayer::SetPicture() as
> 
>             if (length >= size) {
>                size = size * 3 / 2;
>                if (!(buffer = REALLOC(buffer, size))) {
>                   LOG_ERROR_STR("out of memory");
>                   break;
>                   }
>                lseek(f, 0, SEEK_SET);
>                continue;
>                }
> 
> I'll apply this accordingly to other realloc() calls.

Forget about that. Its' probably better to handle each realloc() individually,
according to the suggestion from

  http://www.vdr-portal.de/board/thread.php?postid=979939#post979939

So I guess the fix should look like this:


Klaus