Re: [PATCH] Make gram.y use palloc/pfree for memory management

Currently gram.y uses malloc/free for memory management,
but all pointers are local to function. Unlike flex-based
scan.l which has static references to buffers and reuses them
across calls.

This means gram.y can leak memory if error is throws in
the middle of parsing.

The patch redefines malloc/free to palloc/pfree to fix the leak.

I did not use YYSTACK_ALLOC / YYSTACK_FREE that exists in newer bison,
because bison 1.875 does not support those.

Marko Kreen <markokr(at)gmail(dot)com> writes:
> This means gram.y can leak memory if error is throws in
> the middle of parsing.

Please offer some evidence for that claim.

regards, tom lane

First a correction, overriding malloc/free seems dangerous they
seems to leak out, so correct would be to use YYMALLOC/YYFREE.
This leaves 1.875 potentially leaking, but danger seems small.

On 9/1/08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Kreen <markokr(at)gmail(dot)com> writes:
> > This means gram.y can leak memory if error is throws in
> > the middle of parsing.
>
> Please offer some evidence for that claim.

The leak occurs when
1. bison does allocation.
2. error is thrown.

Now, normally bison does not do allocation as it has initially 200-item
stack allocated in stack. When this is full it does allocate.

But I'm not familial enough with bison internals and Postgres parser
structure, on how cause the stack fill up. It may be that Postgres
parser avoids recursive stack allocations, thus in practice the
leak cannot occur.

--
marko

On 9/1/08, Marko Kreen <markokr(at)gmail(dot)com> wrote:
> First a correction, overriding malloc/free seems dangerous they
> seems to leak out, so correct would be to use YYMALLOC/YYFREE.
> This leaves 1.875 potentially leaking, but danger seems small.

Here is the safer patch. As the chance for the leak is rare,
leaving 1.875 vulnerable should not be problem.

--
marko

"Marko Kreen" <markokr(at)gmail(dot)com> writes:
> On 9/1/08, Marko Kreen <markokr(at)gmail(dot)com> wrote:
>> First a correction, overriding malloc/free seems dangerous they
>> seems to leak out, so correct would be to use YYMALLOC/YYFREE.
>> This leaves 1.875 potentially leaking, but danger seems small.

> Here is the safer patch. As the chance for the leak is rare,
> leaving 1.875 vulnerable should not be problem.

Actually, 1.875 appears to default to using alloca() when available,
so the issue doesn't even arise if you're building with gcc. (This
is probably why I'd never noticed a problem in this area.)

I thought that we might have an issue with flex as well, but so far as
I can tell there's no real problem given our current usage style for
the lexers.

Applied to all the backend .l files ...

regards, tom lane