Opened 16 years ago

Closed 16 years ago

Last modified 16 years ago

#2375 closed defect (fixed)

endemic leaks of namespace objects in mapowscommon.c

Reported by: warmerdam Owned by: tomkralidis
Priority: low Milestone: 5.2 release
Component: SOS Server Version: 5.0
Severity: trivial Keywords: sos leak namespace
Cc: tomkralidis, assefa

Description

A valgrind run of mapserv using mapowscommon services suggests there are many problems with namespace objects memory leaks. For example:

==14505== 6 bytes in 1 blocks are indirectly lost in loss record 27 of 93
==14505==    at 0x4A1AD5E: malloc (vg_replace_malloc.c:149)
==14505==    by 0x832E88E: xmlStrndup (in /usr/lib/libxml2.so.2.6.21)
==14505==    by 0x82DF84A: xmlNewNs (in /usr/lib/libxml2.so.2.6.21)
==14505==    by 0x4EDA9D: msOWSCommonOperationsMetadataOperation (mapowscommon.c
:352)
==14505==    by 0x4D29C9: msWCSGetCapabilities11 (mapwcs.c:786)
==14505==    by 0x4D2D6A: msWCSGetCapabilities (mapwcs.c:886)
==14505==    by 0x4D58E4: msWCSDispatch (mapwcs.c:1625)
==14505==    by 0x4B3ACE: msOWSDispatch (mapows.c:80)
==14505==    by 0x413372: main (mapserv.c:1147)

The actual amounts of memory leaked are modest, so I have set the severity very low, and I think the issue should be pursued in 5.1/5.2 instead of 5.0. I think we need to reuse the primary ows namespace object (created at document creation time) instead of creating local name spaces in the various ows convenience functions. I'm willing to address this as part of my WCS work.

Change History (10)

comment:1 by tomkralidis, 16 years ago

Frank: do you think the calling document should define said namespace and pass it to the various mapowscommon.c functions? The reason I did this was to guarantee that the correct namespace decls were always being written out of mapowscommon.c

comment:2 by warmerdam, 16 years ago

Status: newassigned

Tom,

Essentially yes. There is no way for the local functions to create the name space locally since they can't delete it till the document referencing it is destroyed. I've restructured the functions to pass in the document, and parent node which allows use of a search function to find the namespace needed.

eg.

void msOWSCommonWGS84BoundingBox( 
    xmlDocPtr doc, xmlNodePtr parent, int dimensions, 
    double minx, double miny, double maxx, double maxy) {

  char LowerCorner[100];
  char UpperCorner[100];
  char dim_string[100];

  xmlNsPtr   psNs       = NULL;
  xmlNodePtr psRootNode = NULL;

  psNs = xmlSearchNs( doc, parent, BAD_CAST "ows" );

  /* create element name */
  psRootNode = xmlNewChild(parent, psNs, BAD_CAST "WGS84BoundingBox",NULL);

  sprintf( dim_string, "%d", dimensions );
  xmlNewProp(psRootNode, BAD_CAST "dimensions", BAD_CAST dim_string);

  sprintf(LowerCorner, "%g %g", minx, miny);
  sprintf(UpperCorner, "%g %g", maxx, maxy);

  /* add child elements */
  xmlNewChild(psRootNode, psNs, BAD_CAST "LowerCorner", BAD_CAST LowerCorner);
  xmlNewChild(psRootNode, psNs, BAD_CAST "UpperCorner", BAD_CAST UpperCorner);
}

The code likely ought to issue an error or something if the ows namespace isn't found.

I think, but am not completely sure, that we should move to the model of passing the doc/parent around to functions that create new stuff. I'm trying that out on the wcs code now. Do you think this is a bad model? Do you think it would be better instead to explicitly pass around the namespace objects?

comment:3 by tomkralidis, 16 years ago

Cc: assefa added

Frank: If we add a check to see that the correct namespace is being used, I would be okay with this, since this would lessen the need create the xmlNsPtr so many times throughout the life of the process.

Could a calling function just pass xmlNsPtr, then the functions check for something like:

if (strcmp((char *)psNs->prefix,"ows") == 0)

This would eliminate the need to pass the parent and doc to the functions, I think.

I've cc'd Assefa for comment as well.

comment:4 by assefa, 16 years ago

I like the idea of pasing the name space directly to the functions. Although It feels like passing the doc/root and searching for the name space guarantees more validity, the ows common functions are utility functions and I don't anticipate any problems to impose on the callers to ensure the name space is valid in the document and pass it.

comment:5 by tomkralidis, 16 years ago

As discussed on irc, agreed. We can just pass the namespace and test in mapowscommon.c instead of passing the doc.

I'll wait on your commits then I'll do a once over.

comment:6 by warmerdam, 16 years ago

I've restructured some code to pass in namespaces to owscommon functions (r6986) and my wcs GetCapabilities is "leak clean", but I have not been able to test the SOS changes so I'd appreciate you doing that as well as adding "namespace verification".

comment:7 by tomkralidis, 16 years ago

Owner: changed from warmerdam to tomkralidis
Status: assignednew

Tested against SOS GetCapabilities and GetObservation. Works. I'll take this over and work on the namespace verification.

comment:8 by tomkralidis, 16 years ago

I've tested a small utility function:

/**
 * _validateNamespace()
 *
 * validates the namespace passed to this module's functions
 *
 * @param psOwsNs namespace object
 *
 * @return MS_SUCCESS or MS_FAILURE
 *
 */

static int _validateNamespace(xmlNsPtr psOwsNs) {
  char namespace_prefix[10];
  sprintf(namespace_prefix, "%s", psOwsNs->prefix);
  if (strcmp(namespace_prefix, MS_OWSCOMMON_OWS_NAMESPACE_PREFIX) == 0)
    return MS_SUCCESS;
  else
    return MS_FAILURE;
}

Thinking about this a bit more, as per comment:4, is it problematic to impose this on the callers? Maybe doing valid namespace checking between code inside the same codebase is a bit overkill?

comment:9 by tomkralidis, 16 years ago

Resolution: fixed
Status: newclosed

Fixed in r7057. I ran an SOS GetCapabilities through valgrind and there are now leaks attributed to mapowscommon.c.

There is a small validating function now in mapowscommon.c which is used internally for now. If this is too constraining, we can change at a later date.

I also cleaned up some of the variable names to be consistent.

in reply to:  9 comment:10 by tomkralidis, 16 years ago

Fixed in r7057. I ran an SOS GetCapabilities through valgrind and there are now leaks attributed to mapowscommon.c.

Sorry, I meant there are now no leaks attributed to mapowscommon.c

Note: See TracTickets for help on using tickets.