RE: [Firebird-devel] gpre coredump with CVS build

[Claudio V. C. <cv...@us...>]

Erik Kunze wrote:
> Hello,
>
> gpre from FB2 CVS HEAD dumps a core:
>
> $ gdb5 gen/firebird/bin/gpre_current gen/core
> GNU gdb 5.2.1
> Core was generated by `../gen/firebird/bin/gpre_current -n -z
> -gds_cxx -raw -ids ../src/jrd/dyn.epp ..'.
> Program terminated with signal 6, Aborted.
> (gdb) bt
> #0  0x30018ac4 in kill () from /usr/lib/libc.so.1
> #1  0x30011703 in abort () from /usr/lib/libc.so.1
> #2  0x0806f887 in NOD_LEN (cnt=0) at gpre.h:285
> #3  0x0806f497 in MSC_node(nod_t, short) (type=nod_user_name, count=0)
>     at ../src/gpre/msc.cpp:366
> #4  0x08067899 in par_primitive_value (request=0x812d428,
>     field=0x8124930) at ../src/gpre/exp.cpp:1456

Ok, I thought it was worse. In gpre.h, you'll find:
#define MAKE_NODE(type, count)		MSC_node (type, count)

Then let's go to exp.cpp:par_primitive_value(request, field)
Line 1449:
		node = MAKE_NODE(nod_upcase, 1);
Ok, but see line 1456:
		return MAKE_NODE(nod_user_name, 0);

Now, since MAKE_NODE is a call to MSC_node() and we got zero as the second
param,
msc.cpp:MSC_node(type, count) does
	node = (GPRE_NOD) ALLOC(NOD_LEN(count));

Now, when you look at the inline function and compare it with the macro,

> inline size_t NOD_LEN(const size_t cnt)
> {
>     fb_assert(cnt != 0); <----------
>     return sizeof(gpre_nod) + (cnt - 1) * sizeof(gpre_nod*); }
> //#define NOD_LEN(cnt) (sizeof(gpre_nod) + (cnt - 1) *
> sizeof(gpre_nod*))

you'll realize it's impossible to have zero nodes. Due to the structure of
gpre_nod
typedef struct gpre_nod {
	enum nod_t nod_type;		/* node type */
	USHORT nod_count;			/* number of sub-items */
	gpre_nod* nod_arg[1];		/* argument */
} *GPRE_NOD;

you have always at least one element in the array. Therefore, even with the
best wishes, the code can't ask for zero nodes. I found that problem is
several places and this is the reason I put that check. The original macro
went ahead and tried to allocate
sizeof(gpre_nod) + (ZERO - 1) * sizeof(gpre_nod*)
bytes, then
sizeof(gpre_nod) + (-1) * sizeof(gpre_nod*)
but since sizeof is size_t, defined as unsigned in all platforms I know, you
got a gruesome result. The outcome is that when a node gpre_nod with zero
elements is requested, an excess allocation happens and this can be big.
Actually, due to the macro expansion, after Blas did the initial cleanup in
dudley some weeks ago, I got a bizarre "constant out of range" or something
similar from the compiler. I must clarify that Blas' cleanup only made the
problem evident: it was for lots of years in dudley and it exists in gpre,
too. After I found the problem and recognized gpre's same issue, I decided
to put a spy (the assert). Before posting to CVS, I go for a full rebuild,
so I don't know why I didn't catch this. To bootstrap, I build gpre_meta,
used against gpre_static (these are the Win32 names at least), then
gpre_static runs against the rest, etc.

As I wrote, I fixed the same problem in our obsolete tool dudley (gdef), in
its parse.cpp:
DUDLEY_IDX index = (DUDLEY_IDX) DDL_alloc(IDX_LEN(count == 0 ? 1 : count));
and
DUDLEY_IDX index = (DUDLEY_IDX) DDL_alloc(IDX_LEN(1)); // 0 is invalid


> Can someone please investigate the problem or help me to debug it. I
> can provide the binary and core file.

I think that one quick solution is to comment the assertion and inside the
calculation, do
(cnt ? cnt - 1 : 0)
instead of
(cnt - 1)
at line 286 in gpre.h.

Maybe the long term solution is to make all callers aware that they can't
request zero elements in the array, but they are logically correct: they
request some node types that don't have arguments, hence they send zero.
It's that #$%^&* design throughout the code that defines the first element
inside the parent node that hurts us.

C.