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@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. 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 _______________________________________________ vdr mailing list vdr@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
