#715 closed defect (fixed)
Resolving duplicate statements in map file (fixing memory leaks)
Reported by: | Owned by: | sdlime | |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | MapServer C Library | Version: | 4.3 |
Severity: | minor | Keywords: | |
Cc: | sgillies@… |
Description
mapfile parser does not propery handle valid duplicate items in the map file. For example: LAYER NAME mcd90py2_anno NAME cities ... END are valid map file statements, but they will cause memory leak cause in the mapfile.c (layer->name = getString()) are never chacked if the name or any other option was already set. The solution is either to report that as a parser bug (what probaly is) or to silently free the memory held by the property before it is set againg.
Change History (18)
comment:2 by , 20 years ago
Sure, the error is better then ignoring the duplicates. Here is my proposal for the macro that can be used instead getString(). #define MS_GETSTRING(_prop) \ do { \ if ((_prop)) { \ msSetError(MS_SYMERR, "Duplicate item (%s):(line %d)", "MS_GETSTRING ()", msyytext, msyylineno); \ return -1; \ } else if (msyylex() == MS_STRING) { \ (_prop) = strdup(msyytext); \ } else { \ msSetError(MS_SYMERR, "Parsing error near (%s):(line % d)", "MS_GETSTRING()", msyytext, msyylineno); \ return -1; \ } } while (0) The usage is quite simple.
comment:3 by , 20 years ago
Um... that's lots of code for a macro that would appear 65 times in the code. I guess I should have checked the code before suggesting a macro in my previous comment. Perhaps it would be better just changing getString() to the following. That would make it consistent with the other getDouble() and family anyway: /* ** Load a string from the map file. A "string" is defined ** in lexer.l. */ char *getString(char **value) { if (*value) { msSetError(MS_SYMERR, "Duplicate item (%s):(line %d)", "MS_GETSTRING()", msyytext, msyylineno); return(-1); } else if(msyylex() == MS_STRING) { *value = strdup(msyytext); if (*value == NULL) { msSetError(MS_MEMERR, NULL, "getString()"); return(-1); } return(0); /* Success */ } msSetError(MS_SYMERR, "Parsing error near (%s):(line %d)", "getString()", msyytext, msyylineno); return(-1); }
comment:4 by , 20 years ago
I agree that changing getString seems the best fix. Shall I do that and call this closed? Steve
comment:5 by , 20 years ago
Oh, one gotcha. I believe there is at least one example (PROCESSING) that allows for repeated statements. I don't believe there are others.
comment:6 by , 20 years ago
Works for me. (Note that in my example above I forgot to change the return type of getString() to int)
comment:7 by , 20 years ago
The new getString(char **value) is fine. Only the return type should be int.
comment:8 by , 20 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Man, what a pain in the ass. That function was all over the place in mapfile.c, plus you have to make sure the input string is NULL or you'd get the duplicate parameter error. In most places the change was trivial. In some spots where we are loading CONFIG or PROCESSING sections it was a touch more work (note that duplicate commands within those blocks, and those that use the hashTableObj loader do not throw an error). I've tested with some WCS mapfiles I have laying around that are fairly complex and it seems fine. Steve
comment:9 by , 20 years ago
Think that there is an error in the mapsymbol.c line 197. The: if(getString(s->name) == MS_FAILURE) return(-1); shlould be: if(getString(&s->name) == MS_FAILURE) return(-1); or am I wrong?
comment:11 by , 20 years ago
Hey, I mentioned to Steve that I'm finding problems with these changes. MapServer will build, but shp2img segfaults on my system (Linux-PC) when using the mapserver test data. Here's what valgrind says: [sean@lenny mapserver]$ valgrind ./shp2img -m tests/test.map -o test.png ==11704== Memcheck, a.k.a. Valgrind, a memory error detector for x86-linux. ==11704== Copyright (C) 2002-2003, and GNU GPL'd, by Julian Seward. ==11704== Using valgrind-2.0.0, a program supervision framework for x86-linux. ==11704== Copyright (C) 2000-2003, and GNU GPL'd, by Julian Seward. ==11704== Estimated CPU clock rate is 2396 MHz ==11704== For more details, rerun with: -v ==11704== ==11704== Invalid free() / delete / delete[] ==11704== at 0x40029961: free (vg_replace_malloc.c:231) ==11704== by 0x804E3A3: msFreeCharArray (mapfile.c:80) ==11704== by 0x805C179: msFreeShape (mapprimitive.c:90) ==11704== by 0x8066DCA: msDrawVectorLayer (mapdraw.c:796) ==11704== Address 0xBFFFACA0 is on thread 1's stack ==11704== ==11704== Invalid free() / delete / delete[] ==11704== at 0x40029961: free (vg_replace_malloc.c:231) ==11704== by 0x804E3A3: msFreeCharArray (mapfile.c:80) ==11704== by 0x80645A6: msLayerClose (maplayer.c:428) ==11704== by 0x8066EA4: msDrawVectorLayer (mapdraw.c:816) ==11704== Address 0xBFFFACA0 is on thread 1's stack ==11704== ==11704== Conditional jump or move depends on uninitialised value(s) ==11704== at 0x402321FE: tweenColorTest (gdft.c:554) ==11704== by 0x40231B6E: gdCacheGet (gdcache.c:108) ==11704== by 0x4023266C: gdft_draw_bitmap (gdft.c:783) ==11704== by 0x40232D38: gdImageStringFTEx (gdft.c:1191) ==11704== ==11704== Invalid free() / delete / delete[] ==11704== at 0x40029961: free (vg_replace_malloc.c:231) ==11704== by 0x804E3A3: msFreeCharArray (mapfile.c:80) ==11704== by 0x80758E9: msFreeOutputFormat (mapoutput.c:387) ==11704== by 0x80587DC: msFreeImage (maputil.c:378) ==11704== Address 0xBFFFAD00 is on thread 1's stack ==11704== ==11704== Invalid free() / delete / delete[] ==11704== at 0x40029961: free (vg_replace_malloc.c:231) ==11704== by 0x804E3A3: msFreeCharArray (mapfile.c:80) ==11704== by 0x804F21B: msFreeProjection (mapfile.c:683) ==11704== by 0x8086C42: msFreeMap (mapobject.c:100) ==11704== Address 0xBFFFECD0 is on thread 1's stack ==11704== ==11704== Invalid free() / delete / delete[] ==11704== at 0x40029961: free (vg_replace_malloc.c:231) ==11704== by 0x804E3A3: msFreeCharArray (mapfile.c:80) ==11704== by 0x804F21B: msFreeProjection (mapfile.c:683) ==11704== by 0x8086C50: msFreeMap (mapobject.c:101) ==11704== Address 0xBFFFECD0 is on thread 1's stack ==11704== ==11704== Invalid free() / delete / delete[] ==11704== at 0x40029961: free (vg_replace_malloc.c:231) ==11704== by 0x804E3A3: msFreeCharArray (mapfile.c:80) ==11704== by 0x804F21B: msFreeProjection (mapfile.c:683) ==11704== by 0x8051ED0: freeLayer (mapfile.c:2093) ==11704== Address 0xBFFFECA0 is on thread 1's stack ==11704== ==11704== ERROR SUMMARY: 412 errors from 7 contexts (suppressed: 0 from 0) ==11704== malloc/free: in use at exit: 60939 bytes in 124 blocks. ==11704== malloc/free: 2149 allocs, 2039 frees, 1414688 bytes allocated. ==11704== For a detailed leak analysis, rerun with: --leak-check=yes ==11704== For counts of detected errors, rerun with: -v
comment:12 by , 20 years ago
Cc: | added |
---|
comment:13 by , 20 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I assume we should reopen then?
comment:14 by , 20 years ago
Milestone: | → 4.4 release |
---|
comment:15 by , 20 years ago
I'm not sure how the one change could've caused the memory errors Sean has uncovered, but that's when they started showing up...
comment:17 by , 20 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Found the problem. Marking as closed...
Note:
See TracTickets
for help on using tickets.