Resolving duplicate statements in map file (fixing memory leaks)

[mturk@…]

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.

[dmorissette]

I think a macro to produce a fatal parsing error might not be a bad idea to
indicate to the user that they did something stupid.  Silently ignoring the
duplicate may lead to frustrations when the user doesn't know about the
duplicate and thinks that the first value is used when in fact it's the second
one that is used.

Note that we can detect duplicates only on strings but can't easily detect them
on other data types unless we start adding null flags or something like that...
which would add overhead for very little benefit.

[mturk@…]

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.

[dmorissette]

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);
}

[sdlime]

I agree that changing getString seems the best fix. Shall I do that and call 
this closed?

Steve

[sdlime]

Oh, one gotcha. I believe there is at least one example (PROCESSING) that 
allows for repeated statements. I don't believe there are others.

[dmorissette]

Works for me.

(Note that in my example above I forgot to change the return type of getString()
to int)

[mturk@…]

The new getString(char **value) is fine. Only the return type should be int.

[sdlime]

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

[mturk@…]

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?

[sdlime]

No, you're right. Fixed it...

Steve

[sgillies@…]

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

[dmorissette]

I assume we should reopen then?

[sdlime]

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...

[sgillies@…]

Is it just me?  Can anyone else confirm?

[sdlime]

Found the problem. Marking as closed...

[sgillies@…]

yep, that did it.