BROKEN patch - generates memory leaks [was: Re: r498 - packages/cube/trunk/debian/patches]

[Eddy Petrişor]

On 4/15/06, Gonéri Le Bouder <goneri-guest at costa.debian.org> wrote:
> Author: goneri-guest
> Date: 2006-04-15 01:18:22 +0000 (Sat, 15 Apr 2006)
> New Revision: 498
>
> Modified:
>    packages/cube/trunk/debian/patches/02_data_in_usr_share.patch

I am sorry, but this patch is unacceptable. It generates memeory leaks.

> -+  char *prefixe = "/usr/share/cube/";
> ++  char prefixe[] = "/usr/share/cube/";
>  +
>  +  char *ret;
>  +
> -+  ret = malloc (sizeof(char)*(strlen(prefixe)+strlen(s)));
> ++  ret = (char*)malloc (sizeof(char)*(strlen(prefixe)+strlen(s)+1));

you are allocating memory here, and returning a pointer to the
allocated memory, but on the upper level you are not freeing the
memory. Also, looking over the usual use cases, the older value of the
paths are just forgot, thus generating more memory leaks.

Examples below:

bool installtex(int tnum, char *texname, int &xs, int &ys, bool clamp)
{
    texname = fullpath (texname);
    SDL_Surface *s = IMG_Load(texname);
    if(!s) { conoutf("couldn't load texture %s", texname); return false; };
[snip]
}

The function ends without deallocating the old texname, thus letting
the old memory allocated. On the upper level nobody will know you did
this dirty thing.

Another example, broken in another way, but because of the same
reason, the new function:


void save_world(char *mname)
{
    resettagareas();    // wouldn't be able to reproduce tagged areas otherwise
    voptimize();
    toptimize();
    if(!*mname) mname = getclientmap();
    setnames(mname);
    backup(cgzname, bakname);
    gzFile f = gzopen(fullpath(cgzname), "wb9");
    if(!f) { conoutf("could not write map to %s", cgzname); return; };
    hdr.version = MAPVERSION;
[snip]

Because you are allocating memory in the fullname function to return
it to the upper level, when you call it in "gzFile f =
gzopen(fullpath(cgzname), "wb9");" you will loose the pointer to that
memory piece you just allocated, thus generating more memory leaks.

The proper way of calling the function would be to store the value of
the newly given pointer  and free the memory once you finished using
it. If you intent to use only the new string, then the older pointer
should be memorised and freed once you get the new one.

BUT all of these are broken, because you will have to remember the
classical use case of the function.  Besides, I still don't understand
WHY are you still working on making the installation dependant on
being installed in /usr/. I will repeat, debian packages should be
installable in other places than in the root directory!!

--
Regards,
EddyP
=============================================
"Imagination is more important than knowledge" A.Einstein