gnuplot-beta

A memory leak with current df_3dmatrix routine

[Daniel J S. <dan...@ie...>]

Browsing through the df_3dmatrix() to make sure I've covered everything, 
I think I spotted a memory leak.  Near the top of the routine is

    if (df_binary) {
    if (!fread_matrix(data_fp, &dmatrix, &nr, &nc, &rt, &ct))
        int_error(NO_CARET, "Binary file read error: format unknown!");
    /* fread_matrix() drains the file */
    df_eof = 1;
    } else {

which records the memory pointers to vectors rt and ct and matrix 
dmatrix.  Then near the bottom of the routine is

    free_matrix(dmatrix, 0, nr - 1, 0);
    if (rt)
    free_vector(rt, 0);
    if (ct)
    free_vector(ct, 0);

But of course in between is all sorts of ways that the routine can error 
out.  Hence the memory leak.  Try

splot "binary1" binary using 1

a bunch of times and watch memory increase.

The way to fix this naturally is to make dmatrix, rt annd ct static and 
test for non-zero before allocating.  If non-zero, free before calling 
fread_matrix().  But again, whether it is worth fixing, I'm not sure. 
 (From my perspective, its obsolete.)

Dan

Re: A memory leak with current df_3dmatrix routine

[Hans-Bernhard B. <br...@ph...>]

Daniel J Sebald wrote:

> Browsing through the df_3dmatrix() to make sure I've covered everything, 
> I think I spotted a memory leak.  Near the top of the routine is

> But of course in between is all sorts of ways that the routine can error 
> out.  Hence the memory leak.

Dropping allocated memory on erroring out doesn't necessarily count as a
memory leak, IMHO, if it errors out all the way to the command line.

I.e. I view calling int_error() is a license to leak as much memory as
you like, because trying to avoid memory leaks across a longjmp() is a
futile exercise.

-- 
Hans-Bernhard Broeker (br...@ph...)
Even if all the snow were burnt, ashes would remain.

Re: A memory leak with current df_3dmatrix routine

[Daniel J S. <dan...@ie...>]

Hans-Bernhard Broeker wrote:

> Daniel J Sebald wrote:
>
>> Browsing through the df_3dmatrix() to make sure I've covered 
>> everything, I think I spotted a memory leak.  Near the top of the 
>> routine is
>
>
>> But of course in between is all sorts of ways that the routine can 
>> error out.  Hence the memory leak.
>
>
> Dropping allocated memory on erroring out doesn't necessarily count as a
> memory leak, IMHO, if it errors out all the way to the command line. 


This is an internal error in the sense that the user typed an incorrect 
using string.  That's really an external error.  (The user.)

> I.e. I view calling int_error() is a license to leak as much memory as
> you like, because trying to avoid memory leaks across a longjmp() is a
> futile exercise. 


No it isn't.  It is very straightforward and robust.  Inside of a 
function, construct things as follows:

void foo (void)
{
static float *allocated_floats = 0;

<any code one wants>

    if (allocated_floats)
        free(allocated_floats);
    allocated_floats = alloc(...);

<any code one wants>

    free(allocated_floats);
}

It is what I've been using for a long time.  In fact, you can replace 
those inner memory assignment routines with 
gp_realloc(allocated_floats), so long is the memory pointer is kept 
_static_.

The program errors out to the command line and the memory sits unused, 
sure.  But if the user attempts the command again, the memory is cleaned 
up first.  I think that is perfectly acceptable.

In the gnuplot_x11 dynamic plot code, it is a similar idea.  It's set up 
so that if the X11 window errors out for reasons outside of gnuplot's 
control, the memory allocated for the plot will sit unused... until one 
creates another plot with the same window number, at which point 
gnuplot_x11 knows there is some memory allocated for a plot with that 
number and cleans it up first.  Not doing memory clean up could be a 
_big_ problem in some situations.  As I said before, because of X11's 
construction, it took quite a while to work this out just right.  (The 
error message from X11 can come along at any time, like an interrupt.)

Anyway, please don't attempt a fix on that portion of the code.  I'll 
write back with the final details of the image/binary patch, which I 
think are looking pretty good now.  I'll address this issue.

Dan