#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 , 16 years ago
comment:2 by , 16 years ago
Status: | new → assigned |
---|
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 , 16 years ago
Cc: | 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 , 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 , 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 , 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 , 16 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
Tested against SOS GetCapabilities and GetObservation. Works. I'll take this over and work on the namespace verification.
comment:8 by , 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?
follow-up: 10 comment:9 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.
comment:10 by , 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
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