Re: Possible crash fix for xmlCleanupParser - Mailing list pgadmin-hackers

From Neel Patel
Subject Re: Possible crash fix for xmlCleanupParser
Date
Msg-id CAMcbDBE1YvipfYkMsfYJmzWUwc5Ua7QuOzEtfFeQ-cO+1hC73g@mail.gmail.com
Whole thread Raw
In response to Re: Possible crash fix for xmlCleanupParser  (Dave Page <dave.page@enterprisedb.com>)
Responses Re: Possible crash fix for xmlCleanupParser  (Dave Page <dave.page@enterprisedb.com>)
List pgadmin-hackers
Hi Dave,

Thanks for the comments.

Find the updated patch after fixing the below review comments.
-- Removed the cleanup call when we exit the application.

Thanks,
Neel Patel



On Mon, Aug 12, 2013 at 5:58 PM, Dave Page <dave.page@enterprisedb.com> wrote:
On Mon, Aug 12, 2013 at 12:28 PM, Neel Patel
<neel.patel@enterprisedb.com> wrote:
> Hi Dave,
>
> Please find attached patch for the below issue in pgAdmin.
>
> "As per libxml2 document (below link) we should call xmlCleanupParser() only
> one time when the application exists not every time when we compute the xml
> parser. As per the document application may crash if another thread or
> plugin still using the libxml2".
>
> http://xmlsoft.org/html/libxml-parser.html#xmlCleanupParser

For the benefit of the list archives and readers, this fixes an issue
in an EDB product that's derived from pgAdmin. The crash we have isn't
present in pgAdmin at the moment, but because we're mis-using a
libxml2 API, it could be in the future.

> Kindly review it and let me know for any modification.

Is there any need to cleanup when we're exiting anyway? Memory will be
freed then. Unless there's some reason I'm unaware of, we should just
remove the call from frmReport.

A couple of minor stylistic gripes:

- When you have a comment section of code, it's almost always a
logical block, even if it's just the comment and one line. This should
be illustrated with blank lines before and after, i.e. instead of:

  wxWindow *fr;
  windowList::Node *node;
  // Clean up the memory allocated by the library ( libxml2 )
  xmlCleanupParser();
  while ((node = frames.GetFirst()) != NULL)
  {

You should use:

  wxWindow *fr;
  windowList::Node *node;

  // Clean up the memory allocated by the library ( libxml2 )
  xmlCleanupParser();

  while ((node = frames.GetFirst()) != NULL)
  {

- Try to make your comments short, and to the point. In this case why
not just name the library instead of putting it in braces after what
on it's own is an unhelpful comment? e.g.

  // Cleanup the memory allocated by libxml2.

--
Dave Page
Chief Architect, Tools & Installers
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

Attachment

pgadmin-hackers by date:

Previous
From: Dave Page
Date:
Subject: Re: Possible crash fix for xmlCleanupParser
Next
From: Dave Page
Date:
Subject: pgAdmin III commit: Don't try to cleanup the libxml2 parser when closin