pgsql-4822

Version:

8.4

How it is diagnosed:

Reproduced.

Links:

bug report: http://archives.postgresql.org/pgsql-bugs/2009-05/msg00232.php

Patch: http://archives.postgresql.org/pgsql-committers/2009-06/msg00137.php

Background:

xmlelement -- it is a postgres-specific function to produce XML elements (for easy storage of db result). So for example, the use of:

SELECT xmlelement(name foo, xmlattributes('xyz' as bar));

would create the result:


   xmlelement
------------------
<foo bar="xyz"/>

So the following statement:

SELECT xmlelement(name a, xmlattributes('./qa?a=1&b=2' as href), 'Q&A');

Should produce the result:

               xmlelement              

----------------------------------------

 <a href="./qa?a=1&amp;b=2">Q&amp;A</a>

Where ‘&’ is replaced with ‘&amp;’.

Symptom:

The statement:

=# SELECT xmlelement(name a, xmlattributes('./qa?a=1&b=2' as href), 'Q&A');
                xmlelement
Both input data and output data are helpful for wrong result.
--------------------------------------------
<a href="./qa?a=1&amp;
amp;b=2">Q&amp;A</a>
(1 row)

Where the highlighted ‘amp;’ should not be there (replacement occurred twice).

Root cause:

Developers wasn’t aware library function: “xmlTextWriterWriteAttribute” would replace “&” with “&amp;”, so they also did the conversion on their own. This results “&” is converted to “&amp;” twice!

xmltype* xmlelement(...) {

  … ...

  foreach (arg, xmlExpr->named_args) {

    … …

    /* Here: passed in string is: ‘./qa?a=1&b=2’. In the buggy version, ‘map_sql_value_to_xml_value’ converted it to: ‘./qa?a=1&amp;b=2’. Later, in ‘xmlTextWriterWriteAttribute’ would further convert to ‘./qa?a=1&amp;amp;b=2’*/

-    str = map_sql_value_to_xml_value(value, exprType((Node *) e->expr));

+    str = map_sql_value_to_xml_value(value, exprType((Node *) e->expr), false);

  }

    forboth(arg, named_arg_strings, narg, xexpr->arg_names) {

       char       *str = (char *) lfirst(arg);

       char       *argname = strVal(lfirst(narg));

      if (str)

          /* Second replacement occurred. */

          xmlTextWriterWriteAttribute(writer, (xmlChar *) argname, (xmlChar *) str);

    }

}

- char *map_sql_value_to_xml_value(Datum value, Oid type) {

+ char *map_sql_value_to_xml_value(Datum value, Oid type, bool xml_escape_strings) {

        … ...

        /* ... exactly as-is for XML, and when escaping is not wanted */

      /* In buggy version, it didn’t return here, thus fall through! */

-        if (type == XMLOID)

+       if (type == XMLOID || !xml_escape_strings)

              return str;

         for (p = str; *p; p++)   {

               switch (*p)

               {

                   case '&':

                        /* First replacement occurred. */

                   appendStringInfoString(&buf, "&amp;");

                       break;

                   … ...

               }

           }

         return buf.data;

}

#0  map_sql_value_to_xml_value (value=222109088, type=705,

    xml_escape_strings=0 '\000') at xml.c:1812

#1  0x0000000000771a1f in xmlelement (xmlExpr=0xd40e060, econtext=0xd40df30)

    at xml.c:576

#2  0x00000000005b0015 in ExecEvalXml (xmlExpr=0xd40e060, econtext=0xd40df30,

    isNull=0xd41fc60 "", isDone=0xd41fcd0) at execQual.c:3388

#3  0x00000000005b4224 in ExecTargetList (targetlist=0xd40e360, econtext=0xd40df30,

    values=0xd40fba8, isnull=0xd41fc60 "", itemIsDone=0xd41fcd0,

    isDone=0x7fffde4237a4) at execQual.c:5108

#4  0x00000000005b4813 in ExecProject (projInfo=0xd40e398, isDone=0x7fffde4237a4)

    at execQual.c:5323

======================

Output point:

static void

ExecSelect(TupleTableSlot *slot,

                   DestReceiver *dest,

                   EState *estate)

{

        (*dest->receiveSlot) (slot, dest);

        IncrRetrieved();

        (estate->es_processed)++;

}

======================

Trace(Up side down):

---------                

switch (operation)

                {

                        case CMD_SELECT:

                                ExecSelect(slot, dest, estate);

                                result = slot;

---------

                if (estate->es_useEvalPlan)

                {

                        planSlot = EvalPlanQualNext(estate);

                        if (TupIsNull(planSlot))

                                planSlot = ExecProcNode(planstate);

                }

                else

                        planSlot = ExecProcNode(planstate);

                /*

                 * if the tuple is null, then we assume there is nothing more to

                 * process so we just return null...

                 */

                if (TupIsNull(planSlot))

                {

                        result = NULL;

                        break;

                }

                slot = planSlot;

-------

ExecProcNode(PlanState *node)

{

        TupleTableSlot *result;

        CHECK_FOR_INTERRUPTS();

        if (node->chgParam != NULL) /* something changed */

                ExecReScan(node, NULL); /* let ReScan handle this */

        if (node->instrument)

                InstrStartNode(node->instrument);

        switch (nodeTag(node))

        {

                        /*

                         * control nodes

                         */

                case T_ResultState:

                        result = ExecResult((ResultState *) node);

                        break;

        ...

        return result;

}

-------

ExecResult(ResultState *node)

{

        ...

        while (!node->rs_done)

        {

                ...

                resultSlot = ExecProject(node->ps.ps_ProjInfo, &isDone);

                if (isDone != ExprEndResult)

                {

                        node->ps.ps_TupFromTlist = (isDone == ExprMultipleResult);

                        return resultSlot;

                }

        }

        return NULL;

}

-----

ExecProject(...)

{

        if (projInfo->pi_isVarList)

        {

                /* simple Var list: this always succeeds with one result row */

                if (isDone)

                        *isDone = ExprSingleResult;

                ExecVariableList(projInfo,

                                                 slot->tts_values,

                                                 slot->tts_isnull);

                ExecStoreVirtualTuple(slot);

        }

        else

        {

                if (ExecTargetList(projInfo->pi_targetlist,                <--- actual tainting point!

                                                   projInfo->pi_exprContext,

                                                   slot->tts_values,

                                                   slot->tts_isnull,

                                                   projInfo->pi_itemIsDone,

                                                   isDone))

                        ExecStoreVirtualTuple(slot);

        }

        return slot;

}

-------

ExecStoreVirtualTuple(TupleTableSlot *slot)

{

        /*

         * sanity checks

         */

        Assert(slot != NULL);

        Assert(slot->tts_tupleDescriptor != NULL);

        Assert(slot->tts_isempty);

        

        slot->tts_isempty = false; <---if we just track slot, we will mark it as last tp!

<---but if we just track slot->values, we will mark it as last tp!

        slot->tts_nvalid = slot->tts_tupleDescriptor->natts;

        return slot;

}

-------

ExecTargetList(List *targetlist, ExprContext *econtext,  Datum *values...)

{

        …

        foreach(tl, targetlist)

        {

                values[resind] = ExecEvalExpr(gstate->arg,

                                                                          econtext,

                                                                          &isnull[resind],

                                                                          &itemIsDone[resind]);

}

------

#define ExecEvalExpr(expr, econtext, isNull, isDone) \

        ((*(expr)->evalfunc) (expr, econtext, isNull, isDone))

-----

If we can resolve the function pointer to ExecEvalXml

----

ExecEvalXml(XmlExprState *xmlExpr, ExprContext *econtext,

                        bool *isNull, ExprDoneCond *isDone)

{

        switch (xexpr->op)

        {

                ...

                case IS_XMLELEMENT:

                        *isNull = false;

                        return PointerGetDatum(xmlelement(xmlExpr, econtext));

                        break;

                ...

}

----

#define PointerGetDatum(X) ((Datum) (X))

----

xmlelement(XmlExprState *xmlExpr, ExprContext *econtext)

{

...

        i = 0;

        foreach(arg, xmlExpr->named_args)

        {

                ExprState  *e = (ExprState *) lfirst(arg);

                Datum                value;

                bool                isnull;

                char           *str;

                value = ExecEvalExpr(e, econtext, &isnull, NULL);

                if (isnull)

                        str = NULL;

                else

                        str = OutputFunctionCall(&xmlExpr->named_outfuncs[i], value);

                named_arg_strings = lappend(named_arg_strings, str);

                i++;

        }

        arg_strings = NIL;

        foreach(arg, xmlExpr->args)

        {

                ExprState  *e = (ExprState *) lfirst(arg);

                Datum                value;

                bool                isnull;

                char           *str;

                value = ExecEvalExpr(e, econtext, &isnull, NULL);

                if (!isnull)

                {

                        str = map_sql_value_to_xml_value(value,

                                exprType((Node *) e->expr));

                        arg_strings = lappend(arg_strings, str);

                }

        }

        /* now safe to run libxml */

        xml_init();

        buf = xmlBufferCreate(); <-- this seems to be writing point of but, but....

        writer = xmlNewTextWriterMemory(buf, 0); <---this is the murder

        xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name);

        forboth(arg, named_arg_strings, narg, xexpr->arg_names)

        {

                char           *str = (char *) lfirst(arg);

                char           *argname = strVal(lfirst(narg));

                if (str)

                {

                        xmlTextWriterWriteAttribute(writer,(xmlChar *) argname,(xmlChar *) str); // it’s an internal writing point!

                        pfree(str);

                }

        }

        foreach(arg, arg_strings)

        {

                char           *str = (char *) lfirst(arg);

                xmlTextWriterWriteRaw(writer, (xmlChar *) str);

        }

        xmlTextWriterEndElement(writer);

        xmlFreeTextWriter(writer);

        result = xmlBuffer_to_xmltype(buf);

        xmlBufferFree(buf);

        return result;

#else

        NO_XML_SUPPORT();

        return NULL;

#endif

}

---

xmlBuffer_to_xmltype(xmlBufferPtr buf)

{

        int32                len;

        xmltype    *result;

        len = xmlBufferLength(buf) + VARHDRSZ;

        result = palloc(len);

        SET_VARSIZE(result, len);

        memcpy(VARDATA(result), xmlBufferContent(buf), len - VARHDRSZ);

        return result;

}

Is there any log message?:

No.

Can ErrLog help?

No. Additional function. Did something unnecessary.

Invariant value might work:

for (p = str; *p; p++)   {

               switch (*p)

               {

                   case '&':

                        /* First replacement occurred. */

                   appendStringInfoString(&buf, "&amp;");

                       break;

                   … ...

               }

           }

}

Once ‘p’ == ‘&’, the execution will become flawed.