Re: Postgresql 8.3 beta crash

Following test case is crashing the postgresql-8.3-beta

create schema st;

CREATE TABLE ST.STUDENT(
STUDENT_ID VARCHAR2(10),
NAME VARCHAR(50) NOT NULL,
NIC CHAR(11),
DOB DATE NOT NULL,
GENDER CHAR(1) NOT NULL,
MAR_STAT CHAR(1) NOT NULL,
NATION VARCHAR2(15),
FNAME VARCHAR2(50) NOT NULL,
GNAME VARCHAR2(50),
ADDRESS VARCHAR2(100) NOT NULL,
POS_CODE VARCHAR2(8),
PER_TEL VARCHAR2(15),
EMAIL_ADD VARCHAR(20),
LAST_DEGREE VARCHAR(25),
ENT_DATE DATE NOT NULL);

ALTER TABLE ST.STUDENT
ADD CONSTRAINT PK_ST PRIMARY KEY (STUDENT_ID);

Insert into st.student values(0,'Hassan
Mustafa','1887749999','10-Jan-1981','M','S','Pakistani','Mustafa
Kamal',NULL,'H#2,ST#20,
Islamabad',44000,'0300-4329984','hmustafa(at)hotmail(dot)com','BS','10-JAN-2005');
Insert into st.student values(1,'Faiza
Shah','1887749999','05-Jan-1980','F','S','Pakistani','Muneer
Shah','Sadia Shah','H#6,ST#1 G-10,
Islamabad',44000,'0333-233329984','faiza(at)hotmail(dot)com','FSC','12-FEB-2005');
Insert into st.student values(2,'Mustafa
Kamal','23388777','23-Aug-1978','M','S','Pakistani','Ali
baig',NULL,'H#12,ST#2 F-6,
Islamabad',44000,'0321-66788800','mkamal(at)hotmail(dot)com','BS','10-JAN-2005');
Insert into st.student values(3,'Kashif
Parveez','334459999','27-Dec-1975','M','S','Pakistani','Parveez
Haq',NULL,'H#2,ST#20,
Islamabad',44000,'0345-54329984','kp(at)hotmail(dot)com','BCom','10-JAN-2005');
Insert into st.student values(4,'Ali
Khan','567749999','11-Jan-1981','M','S','Pakistani','S
Kamal',NULL,'H#2,ST#20,
Islamabad',44000,'0399-4329984','skmsss(at)hotmail(dot)com','BS','10-JAN-2005');
Insert into st.student values(5,'Saadia
Naz','1887749999','10-Jan-1981','S','S','Pakistani','Malik
Awan',NULL,'H#2,ST#20,
Islamabad',44000,'0300-4329984','hmustafa(at)hotmail(dot)com','BS','10-JAN-2005');
Insert into st.student values(6,'Shah
Rafeeq','1887749999','10-Jan-1982','M','S','Pakistani','Rafeed
Ahmed',NULL,'H#2,ST#20,
Islamabad',44000,'0300-4329984','hmustafa(at)hotmail(dot)com','BS','10-JAN-2005');
Insert into st.student values(7,'Mohammad
Bilal','1887749999','10-Jun-1981','M','S','Pakistani','Abdul
Hameed',NULL,'H#2,ST#20,
Islamabad',44000,'0300-4329984','hmustafa(at)hotmail(dot)com','BS','10-JAN-2005');
Insert into st.student values(8,'Bilal
Mustafa','1887749999','10-Feb-1980','M','S','Pakistani','Mustafa
Kamal',NULL,'H#2,ST#20,
Islamabad',44000,'0300-4329984','hmustafa(at)hotmail(dot)com','BS','10-JAN-2005');
Insert into st.student values(8,'Shahkir Hussain
Naqvi','1887749999','10-Mar-1978','M','S','Pakistani','Mustafa
Kamal',NULL,'H#2,ST#20,
Islamabad',44000,'0300-4329984','hmustafa(at)hotmail(dot)com','BS','10-JAN-2005');
Insert into st.student values(9,'Amir
Husain','5677499993','20-Jan-1981','M','S','Pakistani','Mustafa
Kamal',NULL,'H#2,ST#20,
Islamabad',44000,'0300-4329984','hmustafa(at)hotmail(dot)com','BS','10-JAN-2005');

SELECT XMLELEMENT
( NAME "Program",
XMLAGG
( XMLELEMENT
( NAME "Student", s.name::xml )
)
) AS "Registered Student"
from st.student s ;

Sheikh Amjad wrote:
> Following test case is crashing the postgresql-8.3-beta
> create schema st;
>
> CREATE TABLE ST.STUDENT(
> STUDENT_ID VARCHAR2(10),
> NAME VARCHAR(50) NOT NULL,
> NIC CHAR(11),
> DOB DATE NOT NULL,
> GENDER CHAR(1) NOT NULL,
> MAR_STAT CHAR(1) NOT NULL,
> NATION VARCHAR2(15),
> FNAME VARCHAR2(50) NOT NULL,
> GNAME VARCHAR2(50),
> ADDRESS VARCHAR2(100) NOT NULL,
> POS_CODE VARCHAR2(8),
> PER_TEL VARCHAR2(15),

VARCHAR2? That smells like Oracle...

I was able to reproduce this after replacing those VARCHAR2's with
VARCHAR. I added some debugging elog's (attached), and it looks like
libxml2 is trying xml_pfree a pointer we never gave it in any of the
alloc functions. Log attached, last xml_pfree crashes and it's the first
time 853c180 is mentioned.

I guess the next step is to narrow it down to a self-contained test case
and send a bug report to libxml people.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

I wrote:
> I was able to reproduce this after replacing those VARCHAR2's with
> VARCHAR. I added some debugging elog's (attached), and it looks like
> libxml2 is trying xml_pfree a pointer we never gave it in any of the
> alloc functions. Log attached, last xml_pfree crashes and it's the first
> time 853c180 is mentioned.

Looking closer, I think it's a memory management bug on our end. I
hadn't looked at the way we use palloc with xml before.

So my current theory is:

In xmlelement(), we use ExecEvalExpr(), which in turn calls xml_parse.
xml_parse calls xmlCleanupParser(). But when we call ExecEvalExpr(),
we're in the middle of constructing an xml buffer, so calling
xmlCleanupBuffer() probably frees something we still need.

Does that sound plausible to you libxml experts out there? If so, how
about we move the calls to ExecEvalExpr() before we start building the
xml buffer?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
> So my current theory is:

> In xmlelement(), we use ExecEvalExpr(), which in turn calls xml_parse.
> xml_parse calls xmlCleanupParser(). But when we call ExecEvalExpr(),
> we're in the middle of constructing an xml buffer, so calling
> xmlCleanupBuffer() probably frees something we still need.

No, your first theory is closer to the mark. What is happening is that
xmlelement neglects to call xml_init, therefore the various stuff
allocated by libxml is allocated using malloc(). Then xml_parse is
called, and it *does* do xml_init(), which calls xmlMemSetup. Then
when we return to xmlelement and start freeing stuff, libxml tries
to use xml_pfree to free something it got from malloc().

I think that (1) we need a call to xml_init here, and hence also a
PG_TRY block; (2) there is a lot of stuff in xml_init that should be
one-time-only, why does it not have an "already done" flag?

regards, tom lane

Tom Lane wrote:
> Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
>> So my current theory is:
>
>> In xmlelement(), we use ExecEvalExpr(), which in turn calls xml_parse.
>> xml_parse calls xmlCleanupParser(). But when we call ExecEvalExpr(),
>> we're in the middle of constructing an xml buffer, so calling
>> xmlCleanupBuffer() probably frees something we still need.
>
> No, your first theory is closer to the mark. What is happening is that
> xmlelement neglects to call xml_init, therefore the various stuff
> allocated by libxml is allocated using malloc(). Then xml_parse is
> called, and it *does* do xml_init(), which calls xmlMemSetup. Then
> when we return to xmlelement and start freeing stuff, libxml tries
> to use xml_pfree to free something it got from malloc().

Oh, yes, you're right.

It still feels unsafe to call ExecEvalExpr while holding on to xml
structs. It means that it's not safe for external modules to use libxml2
and call xmlMemSetup or xmlSetGenericErrorFunc themselves.

> I think that (1) we need a call to xml_init here, and hence also a
> PG_TRY block;

xml_init doesn't actually do anything that would need to be free'd in
case of error. But yeah, it does seem like a good idea to free the "text
writer" and "xml buffer" allocated at the beginning of xmlelement().
They should be allocated by xml_palloc in the current memory context,
though, and freed by the memory context reset as usual, but apparently
we don't trust that for xml document or dtd objects either.

> (2) there is a lot of stuff in xml_init that should be
> one-time-only, why does it not have an "already done" flag?

Hmm. There's the check "sizeof(char) == sizeof(xmlChar)", which in fact
should be evaluated at compile time (should that actually be an
#error?). Then there's the call to xmlSetGenericErrorFunc and
xmlMemSetup which only need to be called once. But it doesn't hurt to
call them again, and it does protect us in case a dynamically loaded
module sets them differently. And then there's the call to xmlInitParser
which does need to be called every time.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:

> Hmm. There's the check "sizeof(char) == sizeof(xmlChar)", which in fact should
> be evaluated at compile time (should that actually be an #error?).

sizeof() isn't expanded by cpp (and cannot be due to cross-compilation) so it
can't be a #error. It could be an autoconf test though.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's PostGIS support!

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
> Tom Lane wrote:
>> I think that (1) we need a call to xml_init here, and hence also a
>> PG_TRY block;

> xml_init doesn't actually do anything that would need to be free'd in
> case of error. But yeah, it does seem like a good idea to free the "text
> writer" and "xml buffer" allocated at the beginning of xmlelement().
> They should be allocated by xml_palloc in the current memory context,
> though, and freed by the memory context reset as usual, but apparently
> we don't trust that for xml document or dtd objects either.

Well, xml_init calls xmlInitParser() which needs to be cleaned up.
But since xmlelement doesn't need that, maybe we should factor it
out of xml_init.

As for the try/catch blocks instead of relying on memory context
cleanup, I'm not entirely sure if that's still needed or if it's a
hangover from before we understood how to use xmlMemSetup. The note
at line 27ff of xml.c implies that libxml keeps static pointers to
allocated things that it thinks will survive indefinitely, so we
may have to have these. I'm suspicious whether xmlelement doesn't
have a problem if the called expressions error out ...

Peter, any comment on this stuff?

regards, tom lane

Tom Lane wrote:
> No, your first theory is closer to the mark. What is happening is
> that xmlelement neglects to call xml_init, therefore the various
> stuff allocated by libxml is allocated using malloc(). Then
> xml_parse is called, and it *does* do xml_init(), which calls
> xmlMemSetup. Then when we return to xmlelement and start freeing
> stuff, libxml tries to use xml_pfree to free something it got from
> malloc().

That sounds plausible.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Heikki Linnakangas wrote:
> It still feels unsafe to call ExecEvalExpr while holding on to xml
> structs. It means that it's not safe for external modules to use
> libxml2 and call xmlMemSetup or xmlSetGenericErrorFunc themselves.

Well yeah, they shouldn't do that. I don't think we want to support
that.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Tom Lane wrote:
> Well, xml_init calls xmlInitParser() which needs to be cleaned up.
> But since xmlelement doesn't need that, maybe we should factor it
> out of xml_init.

That could help.

> As for the try/catch blocks instead of relying on memory context
> cleanup, I'm not entirely sure if that's still needed or if it's a
> hangover from before we understood how to use xmlMemSetup.

It's for the xmlCleanupParser().

> The note
> at line 27ff of xml.c implies that libxml keeps static pointers to
> allocated things that it thinks will survive indefinitely, so we
> may have to have these. I'm suspicious whether xmlelement doesn't
> have a problem if the called expressions error out ...

Hmm. That could also be fixed if we separated xml_init() and
xmlInitParser().
--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Tom Lane wrote:
>> The note
>> at line 27ff of xml.c implies that libxml keeps static pointers to
>> allocated things that it thinks will survive indefinitely, so we
>> may have to have these. I'm suspicious whether xmlelement doesn't
>> have a problem if the called expressions error out ...

> Hmm. That could also be fixed if we separated xml_init() and
> xmlInitParser().

Do we know that only xmlInitParser() sets up such static pointers?

If we do, I wonder whether we could call xmlInitParser once in
TopMemoryContext (or before changing the memory function pointers
at all) and then never do xmlCleanupParser?

regards, tom lane

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Heikki Linnakangas wrote:
>> It still feels unsafe to call ExecEvalExpr while holding on to xml
>> structs. It means that it's not safe for external modules to use
>> libxml2 and call xmlMemSetup or xmlSetGenericErrorFunc themselves.

> Well yeah, they shouldn't do that. I don't think we want to support
> that.

I'm with Heikki that it would be cleaner/safer if we could allow that.
The particular case that's bothering me is the idea that something like
Perl could well try to use libxml internally, and if so it'd very likely
call these functions. Now if Perl thinks it's got sole control, and
tries to (say) re-use the results of xmlInitParser across calls, we're
screwed anyway. But that's not an argument for designing our own code
in a way that guarantees it can't share libxml with other code.

So I'm thinking that we should continue to call xmlMemSetup,
xmlSetGenericErrorFunc, and xmlInitParser (if needed) at the start of
every XML function, and reorganize the code so that we don't call out
to random other code until we've shut down libxml again.

The main disadvantage I can see of reorganizing like that is that
it will increase xmlelement's transient memory consumption, since it
will need to accumulate all the OutputFunctionCall result strings
before it starts to pass them to libxml. This probably isn't a huge
problem though.

Has anyone started actively working on this yet? If not, I can tackle
it.

regards, tom lane

Am Mittwoch, 31. Oktober 2007 schrieb Sheikh Amjad:
> Following test case is crashing the postgresql-8.3-beta

> SELECT XMLELEMENT
> ( NAME "Program",
> XMLAGG
> ( XMLELEMENT
> ( NAME "Student", s.name::xml )
> )
> ) AS "Registered Student"
>
> >from st.student s ;

Btw., I didn't forget this, but I haven't found an extended period of quiet
time to develop a proper solution.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Am Mittwoch, 31. Oktober 2007 schrieb Sheikh Amjad:
>> Following test case is crashing the postgresql-8.3-beta

> Btw., I didn't forget this, but I haven't found an extended period of quiet
> time to develop a proper solution.

I fixed it a few days ago...

regards, tom lane