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

[Klaus Schmidinger <Klaus.Schmidinger@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxx>
> CC: Klaus Schmidinger <Klaus.Schmidinger@xxxxxxx>
> ---
> 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.

Klaus

_______________________________________________
vdr mailing list
vdr@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr