POSITION (null) after mapfile write

[tbonfort]

Reporter: stuarteve@lparchaeology.com Date: 2006/05/30 - 19:10 After upgrading to the CVS version (4.9?) of mapserver (30th May 2006) - I have discovered a bug in the $map->save() function. In order to pass the map object between page reloads we save it out to a temporary file and then reintialise it again on the next page reload. Using the 4.8.3 mapscript this seemed to work fine - however, in the CVS version it seems to put a POSITION (null) in the label section (even when it is set using $labelObj->set("position", "CC");)

This causes the map to not load and it crashes out with the error:

<b>Warning</b>:  Failed to open map file
/srv/www/htdocs/ark/mapserver/tmp/5c7975a60654928b0921582c87969c4c.map in
<b>/srv/www/htdocs/ark/php/map/getmap.php</b> on line <b>62</b><br />

[tbonfort]

Author: dmorissette Date: 2006/05/30 - 19:22

Actualy this is a mapfile.c (msSaveMap()) issue, not specific to PHP MapScript.
Switching component.

[tbonfort]

Author: dmorissette Date: 2006/05/30 - 19:35

Oh wait, this may partly be a PHP issue after all... you pass "CC" as the value
to set the position. You should use the constant MS_CC: 
   $labelObj->set("position", MS_CC)

Can you please try that and confirm that "POSITION CC" is written to your
mapfile after that.

There remains an issue in msSaveMap() should not write "POSITION (null)" when
POSITION is not set properly.

[tbonfort]

Author: stuarteve@lparchaeology.com Date: 2006/05/30 - 19:53

Daniel,

Yep sorry that was a mistake on my part - when I pass MS_CC it works fine.
However, I will have times when thats not set - so hopefully the mapfile.c bug
will get sorted out - thanks for your help!

[tbonfort]

Author: dmorissette Date: 2006/05/30 - 23:27

I had a look in the code and I'm not sure if we should call that a mapfile.c
bug, a PHP MapScript bug, or a generic MapScript bug. I am documenting it here
since similar issues may affect other MapScripts.

What happens is that when you call $labelObj->set("position", "CC"), the PHP
MapScript implementation is very forgiving and attempts to convert the string
"CC" to an integer... which turns out to be 0 in this case. Then it sets
label->position in the C structure to 0.

Then in mapfile.c we call:
fprintf(stream, "  %sPOSITION %s\n", tab, msPositionsText[label->position - MS_UL]);

Since label->position is zero, the array index is negative and pointing to an
invalid memory area. Having label->position (or any other struct member) set to
zero by defautl is usually not an issue since 0 is almost always a valid enum
value (POSITION is an exception on that front).

We could modify PHP MapScript to be less forgiving and complain right away if a
string is passed for an integer parameter like this. However there are cases
where a string may contain a valid value (e.g. "123") and adding type validation
at this late point in PHP MapScript's life may cause grief to lots of existing
code. Plus that would not prevent users from passing arbitrary integer values ot
the set() method and still cause the same crash. Also I fear that other
MapScripts may suffer from the same level of forgiveness and allow invalid
values to be set too.

We could fix the mapfile.c code to check that label->position contains a valid
value before accessing the msPositionText[] array. This would be an easy fix,
but there may be many places that do similar things without any validation that
should be fixed too.

What do you think is best Steve?

[tbonfort]

Author: sdlime Date: 2006/05/31 - 00:10

Ugh... Ideally we'd do both.

Seems like PHP/MapScript should be accepting a integer there (or symbol, MS_CC) 
and complain otherwise. SWIG enforces the underlying type (position is an int) 
so that would be consistent with how it behaves. (I need to test that theory.)

That said, as Dan mentions, one could always set a value that is out of range 
so the mapfile writer should make sure the position is in range before 
attempting to write the text equivalent. We're probably woefully inadequate 
with those type of checks.

In general I like the idea of catching these types of issues as early as 
possible (bad values may cause problems elsewhere), but perhaps the latter fix 
to the writer gives us ultimately more protection.

Steve

[tbonfort]

Author: sdlime Date: 2008/06/25 - 08:09 Moving to 5.4... Progress has been made so perhaps we should kill this bug and create two that are more meaningful. One for data typing in PHP/MapScript and another for bounds checking in the write...() functions in mapfile.c.

Steve