Thread: I'm amazed we never caught this before ...

I'm amazed we never caught this before ...

From
Tom Lane
Date:
The stringToNode routines (backend/nodes/read.c and readfuncs.c) depend
on a static variable that is the next-input pointer for lsptok().

Guess what happens if stringToNode is invoked recursively.  (Hint:
it's not good.)

It's apparently not easy for this to happen, but I have a reproducible
test case involving relcache flush while reading the rule action
parsetree for a view.  readDatum() needs to be able to look up the
pg_type entry for the value it's reading, and that lookup could result
in relcache flush, which would lead to an attempt to read the rule
actions for any locked relcache entries --- like, say, the one we
are currently trying to load.

I propose fixing this by making stringToNode save and restore the
entry-time value of the static pointer.  (Alternatively, we could
explicitly pass a pointer-to-pointer-to-char to lsptok() and every
single one of the readfuncs, but that seems like way too much
notational clutter.)

I am not sure that is enough to fix the problem completely.  It might
be best to avoid calling get_typbyval() from readDatum, which could
be done if we re-ordered the fields of a CONST dump so that the byval
field appears before we need to read the const value itself.  That
means an initdb, but we've already forced an initdb for beta2 ...

Marc, can you hold off wrapping beta2 for a few hours?  I think this
is a 'must fix'.
        regards, tom lane


Re: I'm amazed we never caught this before ...

From
The Hermit Hacker
Date:
On Sat, 6 Jan 2001, Tom Lane wrote:

> The stringToNode routines (backend/nodes/read.c and readfuncs.c) depend
> on a static variable that is the next-input pointer for lsptok().
>
> Guess what happens if stringToNode is invoked recursively.  (Hint:
> it's not good.)
>
> It's apparently not easy for this to happen, but I have a reproducible
> test case involving relcache flush while reading the rule action
> parsetree for a view.  readDatum() needs to be able to look up the
> pg_type entry for the value it's reading, and that lookup could result
> in relcache flush, which would lead to an attempt to read the rule
> actions for any locked relcache entries --- like, say, the one we
> are currently trying to load.
>
> I propose fixing this by making stringToNode save and restore the
> entry-time value of the static pointer.  (Alternatively, we could
> explicitly pass a pointer-to-pointer-to-char to lsptok() and every
> single one of the readfuncs, but that seems like way too much
> notational clutter.)
>
> I am not sure that is enough to fix the problem completely.  It might
> be best to avoid calling get_typbyval() from readDatum, which could
> be done if we re-ordered the fields of a CONST dump so that the byval
> field appears before we need to read the const value itself.  That
> means an initdb, but we've already forced an initdb for beta2 ...
>
> Marc, can you hold off wrapping beta2 for a few hours?  I think this
> is a 'must fix'.

No probs ... I got the update to mk-release from Peter, so let me know
when you are ready and we'll go with beta2 ...




Re: Re: I'm amazed we never caught this before ...

From
Tom Lane
Date:
OK, stringToNode changes committed.  I think we're ready to go for
beta2...
        regards, tom lane