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/29718712eb2e53c09d28f08e39...

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:

--- PLUGINS/src/pictures/player.c 2008/02/09 12:13:10 2.0 +++ PLUGINS/src/pictures/player.c 2011/02/20 17:15:25 @@ -66,8 +66,15 @@ length = read(f, buffer, size); if (length > 0) { if (length >= size) { - size = size * 3 / 2; - buffer = (uchar *)realloc(buffer, size); + int NewSize = size * 3 / 2; + if (uchar *NewBuffer = (uchar *)realloc(buffer, NewSize)) { + buffer = NewBuffer; + size = NewSize; + } + else { + LOG_ERROR_STR("out of memory"); + break; + } lseek(f, 0, SEEK_SET); continue; }

Klaus