LCDproc development and user support list

Text archives Help


[Lcdproc] PATCH: Memory leak in PicoLCD driver


Chronological Thread 
  • From: peter at adpm.de (Peter Marschall)
  • Subject: [Lcdproc] PATCH: Memory leak in PicoLCD driver
  • Date: Tue, 16 Sep 2008 17:58:48 +0200

Hi Jack,

On Monday, 1. September 2008, Jack Cleaver wrote:
> I'm offering the attached patch for the picoLCD driver.
>
> I'm no great shakes as a C/Linux coder, not having worked in C for over
> 15 years. I'd therefore appreciate if someone could verify by inspection
> that what appears to be an obvious leak really is a leak, and that this
> patch fixes it (without breaking something else).
>
> JackC.
>
> jackc at virya:~/Projects/lcdproc-0.5.2/server/drivers$ diff picolcd.c-orig
> picolcd.c
> 303a304
>
> > char * retval = NULL;
>
> 329a331,333
>
> > if (strlen(keystr))
> > retval = keystr;
>
> 333d336
> < return NULL;
> 342,345c345
> < if (! strlen(keystr))
> < return NULL;
> <
> < return keystr;
> ---
>
> > return retval;
>

Maybe I am a bit dim today, but I fail to see the issue in the original
version, while I see problems in your version.

Original says:
if length of variable keystr is 0,
then return the NULL pointer to the caller
to indicate no key was pressed.
otherwise return the contents of the variable keystr
to the caller to indicate that a key was pressed and which one.

The patch replaces this with:
Create a new variable retval and preset it with the NULL pointer.
If length of variable keystr differs from 0,
then assign the conntents of the variable keystr to
the variable retval;
return the NULL pointer to the caller

retval is only used on initialization, and on assignment from keystr.
It is not returned either.
What is it good for ?

With your solution the function returns NULL all the time indicating that no
key was pressed. I guess this is not what you intended.


But you are right, there is a memory leak, and even worse
writing to unallocated memory in this function.

The solution of returning two keys combined with '+'
is at least strange, because this behaviour is not
defined in LCDd.
A solution that queues keys and sends them back one by one
is IMHO more appropriate.

Regards
Peter

--
Peter Marschall
peter at adpm.de




Archive powered by MHonArc 2.6.18.

Top of page