php clone(): free(): invalid pointer

[cpurvis@…]

Using $cloneMap = $oMap->clone(); leads to a PHP error when run from the 
command line.

free(): invalid pointer 0x822b370!

PHP 4.3.2, MapServer version 4.0

Will attach demo files.

[cpurvis@…]

demo files to produce the error

[dmorissette]

Adding Sean to CC.

As I wrote on the list I am unable to reproduce this on Linux with PHP 4.3.3 and
php_mapscript 4.0.1, but Sean wrote that he saw this error before... do you have
any details to provide Sean?

Charlton: Are you using 4.0.0 or 4.0.1?  If you're using 4.0.0 then you should
probably try upgrading to 4.0.1 since it contains a fix that may be what you ran
into.  From the HISTORY.TXT:

-----
- Fixed problem with cloned mapObj's fontset->filename pointing to the
  original mapObj's fontset->filename
-----

[cpurvis@…]

OK.  Just upgraded to 4.0.1 php mapscript, and now instead of getting an 
error, I get a segfault.

If I commment out the call to clone_and_draw in my code, it runs fine.

[sgillies@…]

The msCopyMap and msCopy* code which is underneath clone() is quite fragile.

My hypothesis is that between 4.0 and 4.1 (CVS HEAD) something changed in 
one of the mapserver structs -- maybe a struct element was added or removed
or changed to another type -- and clone() is vulnerable to these changes.

Because of the way that MapServer is developed, people across the world who
can't always know what others are doing, clone() is likely to be broken at
any time in the CVS HEAD.

One thing that will help us out is some standard MapServer tests.  The other
open source software that we use almost always has tests that are executed
like

make; make test

It's well past time that we write test programs for MapServer that are to be
passed before code changes are committed to CVS.  This would guarantee 
(well, sort of guarantee :) that no one would commit changes that break 
methods like clone().

Meanwhile, I'll be trying to debug clone().

[sgillies@…]

Just a comment on status of the bug.

No fix yet, and I am actually thinking of pulling all the msCopy* functions
out of MapServer.  They are just too fragile and can't be maintained.

The problem is that _any_ addition or subtraction of members from the 
many Mapserver structures (layerObj, mapObj, etc) breaks the copy functions.
Developers are basically free to add members as they wish, never test the
impact on the copying functions, and it's too hard to keep up with the
changes.

I will miss having the ability to clone a mapObj since it is much faster
than writing to disk and reading back into a new instance.  Like milliseconds
vs seconds.

Does anybody else feel that clone() is important enough that we make developers
test that they don't break it?  If not, then we should probably just drop it
and leave implementation up to users.

[assefa]

I personnaly feel that clone functionnality are important in the mapscript world.
We use them in diffrent projects and I would really hate to see them gone.
I think developpers should be aware of the implications of adding a new member
in a structure and should upgrade the clone functionnalities to take the new
parameter into account (or at least make sure that the new memeber is set to a
valid default in the initXXXobj functions). 

[dmorissette]

How about putting an 'assert(sizeof(whateverObj) == what_it_should_be)' in each
of the copy functions?  That would tell you if something has changed. 

You could also put a big comment above the assert() in the code to tell the
developer that if he got this assert then he should do his homework and update
the copy functions.

[dmorissette]

Wait... I just realized that what I suggested wouldn't be cross-platform...
sorry for the noise... but a trick along those lines could possibly do.

[sgillies@…]

Which platforms don't have assert?  Isn't assert.h standard C?

[dmorissette]

assert() is on all platforms (AFAIK), it's the size of the structures that may
change depending on system architectures and byte alignment, etc.

[sgillies@…]

Update.  I've finally made Frank Warmerdam aware of clone() and msCopyMap.
He's going to keep an eye out for it in the future and has already made
some changes that are making it more stable.

Status right now is that msCopyMap, and therefore mapObj.clone, work without
any leaks for maps that do *not* have a fontset.  Getting the fontset to
copy properly and then free properly is evading me today, but I hope to 
have it fixed by the end of the week so that we can have it working in the
next release.

Am reassigning the bug to myself.

[fwarmerdam]

I have fixed some serious errors in the handling of outputformats. 
I also made a copyStringPropertyRealloc() that will free an existing
destination string, and then allocate a new string for the new value. 
I noticed this in particular with the mapname was defaults to "MS" in 
the map initializer.  But when a long name was assigned in msCopyMap() 
it was copied over the existing name, writing right past the end of
the allocated memory. 

For some simple examples the msCopyMap() now works (again?) without
any problems apparent in valgrind. 

[sgillies@…]

Frank,

Did you commit the changes?  I grepped and didn't find any
copyStringPropertyRealloc in the source.

Sean

[fwarmerdam]

Sean,

mapcopy.c 1.23 should contain this.  This appears to be the most recent
trunk revision.  Are you sure you aren't looking at the 4.2 branch?

[sgillies@…]

Thanks, Frank.

I copied your new work in mapcopy.c into the 4.2 branch as well and have added
in each branch a testcopy program (in testcopy.c) that I intend to use like

   valgrind ./testcopy

to check for memory issues of msCopyMap.  Valgrind reports no errors for
testcopy in the case that the test.map file has _no_ fontset file.  With a
fontset file all hell breaks loose.  For example:

==9585== Invalid free() / delete / delete[]
==9585==    at 0x40029961: free (vg_replace_malloc.c:231)
==9585==    by 0x805731C: msFreeFontSet (maplabel.c:131)
==9585==    by 0x805544E: msFreeMap (mapfile.c:3908)
==9585==    by 0x804D074: main (testcopy.c:35)
==9585==    Address 0x423254F8 is 0 bytes inside a block of size 10 free'd
==9585==    at 0x40029961: free (vg_replace_malloc.c:231)
==9585==    by 0x805731C: msFreeFontSet (maplabel.c:131)
==9585==    by 0x805544E: msFreeMap (mapfile.c:3908)
==9585==    by 0x804D06C: main (testcopy.c:34)
==9585==
==9585== Invalid read of size 4
==9585==    at 0x8077738: msFreeHashTable (maphash.c:88)
==9585==    by 0x805730C: msFreeFontSet (maplabel.c:134)
==9585==    by 0x805544E: msFreeMap (mapfile.c:3908)
==9585==    by 0x804D074: main (testcopy.c:35)
==9585==    Address 0x4238B1F8 is 0 bytes inside a block of size 164 free'd
==9585==    at 0x40029961: free (vg_replace_malloc.c:231)
==9585==    by 0x8077779: msFreeHashTable (maphash.c:96)
==9585==    by 0x805730C: msFreeFontSet (maplabel.c:134)
==9585==    by 0x805544E: msFreeMap (mapfile.c:3908)
==9585==
==9585== Invalid read of size 4
==9585==    at 0x8077747: msFreeHashTable (maphash.c:90)
==9585==    by 0x805730C: msFreeFontSet (maplabel.c:134)
==9585==    by 0x805544E: msFreeMap (mapfile.c:3908)
==9585==    by 0x804D074: main (testcopy.c:35)
==9585==    Address 0x4238B46C is 4 bytes inside a block of size 12 free'd
==9585==    at 0x40029961: free (vg_replace_malloc.c:231)
==9585==    by 0x8077763: msFreeHashTable (maphash.c:89)
==9585==    by 0x805730C: msFreeFontSet (maplabel.c:134)
==9585==    by 0x805544E: msFreeMap (mapfile.c:3908)
==9585==

Hopefully I'll be able to learn from your recent fixes and fix these 
remaining issues.

To date I've been doing all my development on OS X (aka FreeBSD) using the
program named leaks.  My Apple computer has been recalled so I am back to
developing on Linux.  Valgrind works much like leaks, but seems to be
catching errors on i686 that I wasn't seeing on the PowerPC.

[sgillies@…]

I've reached a temporary resolution of this issue for the 4.2 branch.
Map cloning WILL NOT include map fontset and symbolset.  It is left
to the user to take care of these.  Example:

    map_clone = map_orig.clone()
    map_clone.setFontSet("absolute/path/to/fontset/file")
    map_clone.setSymbolSet("absolute/path/to/symbolset/file")
    img = map_clone.draw()

Getting the fontset and symbolset properly cloned will be achieved in the 4.4
release.  Issue 640 is created to deal with this.