[RFC] player.c: fix `(error) Common realloc mistake: 'buffer' nulled but not freed upon failure`
Commit Message
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(-)
Comments
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.
Klaus
@@ -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;
}