Re: memory leak in libxml2 - fix

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2010/11/26 Itagaki Takahiro <itagaki.takahiro@gmail.com>:
>> Why did you change doctree and ctxt to global variables?
>> I'm not sure why /* xmlFreeDoc(doctree); */ is commented out
>> at the end of pgxml_xpath(), but is it enough to enable the code?

> I am thinking, so you must not to call xmlFreeDoc(doctree) early.
> Probably xmlXPathCastToXXX reading a doctree.

Those static variables are really ugly, and what's more this patch only
stops some of the leakage.  Per experimentation, the result object from
pgxml_xpath has to be freed too, once it's been safely converted to
whatever the end result type is.  You can see this by watching

select sum(xpath_number('<data>' || generate_series || '</data>','/data')) from
generate_series(1,500000);

which still shows leakage with the submitted patch.  I cleaned it up
as per attached, which doesn't show any leakage.

            regards, tom lane

diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index 0df7647..e92ab66 100644
*** a/contrib/xml2/xpath.c
--- b/contrib/xml2/xpath.c
*************** Datum        xpath_table(PG_FUNCTION_ARGS);
*** 40,45 ****
--- 40,54 ----

  void        pgxml_parser_init(void);

+ /* workspace for pgxml_xpath() */
+
+ typedef struct
+ {
+     xmlDocPtr    doctree;
+     xmlXPathContextPtr ctxt;
+     xmlXPathObjectPtr res;
+ } xpath_workspace;
+
  /* local declarations */

  static xmlChar *pgxmlNodeSetToText(xmlNodeSetPtr nodeset,
*************** static text *pgxml_result_to_text(xmlXPa
*** 51,57 ****

  static xmlChar *pgxml_texttoxmlchar(text *textstring);

! static xmlXPathObjectPtr pgxml_xpath(text *document, xmlChar *xpath);


  /*
--- 60,69 ----

  static xmlChar *pgxml_texttoxmlchar(text *textstring);

! static xmlXPathObjectPtr pgxml_xpath(text *document, xmlChar *xpath,
!                                      xpath_workspace *workspace);
!
! static void cleanup_workspace(xpath_workspace *workspace);


  /*
*************** PG_FUNCTION_INFO_V1(xpath_nodeset);
*** 221,245 ****
  Datum
  xpath_nodeset(PG_FUNCTION_ARGS)
  {
!     xmlChar    *xpath,
!                *toptag,
!                *septag;
!     int32        pathsize;
!     text       *xpathsupp,
!                *xpres;
!
!     /* PG_GETARG_TEXT_P(0) is document buffer */
!     xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */

!     toptag = pgxml_texttoxmlchar(PG_GETARG_TEXT_P(2));
!     septag = pgxml_texttoxmlchar(PG_GETARG_TEXT_P(3));

!     pathsize = VARSIZE(xpathsupp) - VARHDRSZ;

!     xpath = pgxml_texttoxmlchar(xpathsupp);

!     xpres = pgxml_result_to_text(pgxml_xpath(PG_GETARG_TEXT_P(0), xpath),
!                                  toptag, septag, NULL);

      pfree(xpath);

--- 233,254 ----
  Datum
  xpath_nodeset(PG_FUNCTION_ARGS)
  {
!     text       *document = PG_GETARG_TEXT_P(0);
!     text       *xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */
!     xmlChar    *toptag = pgxml_texttoxmlchar(PG_GETARG_TEXT_P(2));
!     xmlChar    *septag = pgxml_texttoxmlchar(PG_GETARG_TEXT_P(3));
!     xmlChar    *xpath;
!     text       *xpres;
!     xmlXPathObjectPtr res;
!     xpath_workspace workspace;

!     xpath = pgxml_texttoxmlchar(xpathsupp);

!     res = pgxml_xpath(document, xpath, &workspace);

!     xpres = pgxml_result_to_text(res, toptag, septag, NULL);

!     cleanup_workspace(&workspace);

      pfree(xpath);

*************** PG_FUNCTION_INFO_V1(xpath_list);
*** 257,279 ****
  Datum
  xpath_list(PG_FUNCTION_ARGS)
  {
!     xmlChar    *xpath,
!                *plainsep;
!     int32        pathsize;
!     text       *xpathsupp,
!                *xpres;
!
!     /* PG_GETARG_TEXT_P(0) is document buffer */
!     xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */

!     plainsep = pgxml_texttoxmlchar(PG_GETARG_TEXT_P(2));

!     pathsize = VARSIZE(xpathsupp) - VARHDRSZ;

!     xpath = pgxml_texttoxmlchar(xpathsupp);

!     xpres = pgxml_result_to_text(pgxml_xpath(PG_GETARG_TEXT_P(0), xpath),
!                                  NULL, NULL, plainsep);

      pfree(xpath);

--- 266,286 ----
  Datum
  xpath_list(PG_FUNCTION_ARGS)
  {
!     text       *document = PG_GETARG_TEXT_P(0);
!     text       *xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */
!     xmlChar    *plainsep = pgxml_texttoxmlchar(PG_GETARG_TEXT_P(2));
!     xmlChar    *xpath;
!     text       *xpres;
!     xmlXPathObjectPtr res;
!     xpath_workspace workspace;

!     xpath = pgxml_texttoxmlchar(xpathsupp);

!     res = pgxml_xpath(document, xpath, &workspace);

!     xpres = pgxml_result_to_text(res, NULL, NULL, plainsep);

!     cleanup_workspace(&workspace);

      pfree(xpath);

*************** PG_FUNCTION_INFO_V1(xpath_string);
*** 288,300 ****
  Datum
  xpath_string(PG_FUNCTION_ARGS)
  {
      xmlChar    *xpath;
      int32        pathsize;
!     text       *xpathsupp,
!                *xpres;
!
!     /* PG_GETARG_TEXT_P(0) is document buffer */
!     xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */

      pathsize = VARSIZE(xpathsupp) - VARHDRSZ;

--- 295,307 ----
  Datum
  xpath_string(PG_FUNCTION_ARGS)
  {
+     text       *document = PG_GETARG_TEXT_P(0);
+     text       *xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */
      xmlChar    *xpath;
      int32        pathsize;
!     text       *xpres;
!     xmlXPathObjectPtr res;
!     xpath_workspace workspace;

      pathsize = VARSIZE(xpathsupp) - VARHDRSZ;

*************** xpath_string(PG_FUNCTION_ARGS)
*** 305,317 ****
      /* We could try casting to string using the libxml function? */

      xpath = (xmlChar *) palloc(pathsize + 9);
-     memcpy((char *) (xpath + 7), VARDATA(xpathsupp), pathsize);
      strncpy((char *) xpath, "string(", 7);
      xpath[pathsize + 7] = ')';
      xpath[pathsize + 8] = '\0';

!     xpres = pgxml_result_to_text(pgxml_xpath(PG_GETARG_TEXT_P(0), xpath),
!                                  NULL, NULL, NULL);

      pfree(xpath);

--- 312,327 ----
      /* We could try casting to string using the libxml function? */

      xpath = (xmlChar *) palloc(pathsize + 9);
      strncpy((char *) xpath, "string(", 7);
+     memcpy((char *) (xpath + 7), VARDATA(xpathsupp), pathsize);
      xpath[pathsize + 7] = ')';
      xpath[pathsize + 8] = '\0';

!     res = pgxml_xpath(document, xpath, &workspace);
!
!     xpres = pgxml_result_to_text(res, NULL, NULL, NULL);
!
!     cleanup_workspace(&workspace);

      pfree(xpath);

*************** PG_FUNCTION_INFO_V1(xpath_number);
*** 326,346 ****
  Datum
  xpath_number(PG_FUNCTION_ARGS)
  {
      xmlChar    *xpath;
-     int32        pathsize;
-     text       *xpathsupp;
      float4        fRes;
-
      xmlXPathObjectPtr res;
!
!     /* PG_GETARG_TEXT_P(0) is document buffer */
!     xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */
!
!     pathsize = VARSIZE(xpathsupp) - VARHDRSZ;

      xpath = pgxml_texttoxmlchar(xpathsupp);

!     res = pgxml_xpath(PG_GETARG_TEXT_P(0), xpath);
      pfree(xpath);

      if (res == NULL)
--- 336,352 ----
  Datum
  xpath_number(PG_FUNCTION_ARGS)
  {
+     text       *document = PG_GETARG_TEXT_P(0);
+     text       *xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */
      xmlChar    *xpath;
      float4        fRes;
      xmlXPathObjectPtr res;
!     xpath_workspace workspace;

      xpath = pgxml_texttoxmlchar(xpathsupp);

!     res = pgxml_xpath(document, xpath, &workspace);
!
      pfree(xpath);

      if (res == NULL)
*************** xpath_number(PG_FUNCTION_ARGS)
*** 348,353 ****
--- 354,361 ----

      fRes = xmlXPathCastToNumber(res);

+     cleanup_workspace(&workspace);
+
      if (xmlXPathIsNaN(fRes))
          PG_RETURN_NULL();

*************** PG_FUNCTION_INFO_V1(xpath_bool);
*** 360,380 ****
  Datum
  xpath_bool(PG_FUNCTION_ARGS)
  {
      xmlChar    *xpath;
-     int32        pathsize;
-     text       *xpathsupp;
      int            bRes;
-
      xmlXPathObjectPtr res;
!
!     /* PG_GETARG_TEXT_P(0) is document buffer */
!     xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */
!
!     pathsize = VARSIZE(xpathsupp) - VARHDRSZ;

      xpath = pgxml_texttoxmlchar(xpathsupp);

!     res = pgxml_xpath(PG_GETARG_TEXT_P(0), xpath);
      pfree(xpath);

      if (res == NULL)
--- 368,384 ----
  Datum
  xpath_bool(PG_FUNCTION_ARGS)
  {
+     text       *document = PG_GETARG_TEXT_P(0);
+     text       *xpathsupp = PG_GETARG_TEXT_P(1);    /* XPath expression */
      xmlChar    *xpath;
      int            bRes;
      xmlXPathObjectPtr res;
!     xpath_workspace workspace;

      xpath = pgxml_texttoxmlchar(xpathsupp);

!     res = pgxml_xpath(document, xpath, &workspace);
!
      pfree(xpath);

      if (res == NULL)
*************** xpath_bool(PG_FUNCTION_ARGS)
*** 382,387 ****
--- 386,393 ----

      bRes = xmlXPathCastToBoolean(res);

+     cleanup_workspace(&workspace);
+
      PG_RETURN_BOOL(bRes);
  }

*************** xpath_bool(PG_FUNCTION_ARGS)
*** 390,438 ****
  /* Core function to evaluate XPath query */

  static xmlXPathObjectPtr
! pgxml_xpath(text *document, xmlChar *xpath)
  {
!     xmlDocPtr    doctree;
!     xmlXPathContextPtr ctxt;
      xmlXPathObjectPtr res;
      xmlXPathCompExprPtr comppath;
-     int32        docsize;

!     docsize = VARSIZE(document) - VARHDRSZ;

      pgxml_parser_init();

!     doctree = xmlParseMemory((char *) VARDATA(document), docsize);
!     if (doctree == NULL)
          return NULL;            /* not well-formed */

!     ctxt = xmlXPathNewContext(doctree);
!     ctxt->node = xmlDocGetRootElement(doctree);

      /* compile the path */
      comppath = xmlXPathCompile(xpath);
      if (comppath == NULL)
      {
!         xmlFreeDoc(doctree);
          xml_ereport(ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
                      "XPath Syntax Error");
      }

      /* Now evaluate the path expression. */
!     res = xmlXPathCompiledEval(comppath, ctxt);
      xmlXPathFreeCompExpr(comppath);

      if (res == NULL)
!     {
!         xmlXPathFreeContext(ctxt);
!         xmlFreeDoc(doctree);

-         return NULL;
-     }
-     /* xmlFreeDoc(doctree); */
      return res;
  }

  static text *
  pgxml_result_to_text(xmlXPathObjectPtr res,
                       xmlChar *toptag,
--- 396,456 ----
  /* Core function to evaluate XPath query */

  static xmlXPathObjectPtr
! pgxml_xpath(text *document, xmlChar *xpath, xpath_workspace *workspace)
  {
!     int32        docsize = VARSIZE(document) - VARHDRSZ;
      xmlXPathObjectPtr res;
      xmlXPathCompExprPtr comppath;

!     workspace->doctree = NULL;
!     workspace->ctxt = NULL;
!     workspace->res = NULL;

      pgxml_parser_init();

!     workspace->doctree = xmlParseMemory((char *) VARDATA(document), docsize);
!     if (workspace->doctree == NULL)
          return NULL;            /* not well-formed */

!     workspace->ctxt = xmlXPathNewContext(workspace->doctree);
!     workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);

      /* compile the path */
      comppath = xmlXPathCompile(xpath);
      if (comppath == NULL)
      {
!         cleanup_workspace(workspace);
          xml_ereport(ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
                      "XPath Syntax Error");
      }

      /* Now evaluate the path expression. */
!     res = xmlXPathCompiledEval(comppath, workspace->ctxt);
!     workspace->res = res;
!
      xmlXPathFreeCompExpr(comppath);

      if (res == NULL)
!         cleanup_workspace(workspace);

      return res;
  }

+ /* Clean up after processing the result of pgxml_xpath() */
+ static void
+ cleanup_workspace(xpath_workspace *workspace)
+ {
+     if (workspace->res)
+         xmlXPathFreeObject(workspace->res);
+     workspace->res = NULL;
+     if (workspace->ctxt)
+         xmlXPathFreeContext(workspace->ctxt);
+     workspace->ctxt = NULL;
+     if (workspace->doctree)
+         xmlFreeDoc(workspace->doctree);
+     workspace->doctree = NULL;
+ }
+
  static text *
  pgxml_result_to_text(xmlXPathObjectPtr res,
                       xmlChar *toptag,