Mapserver 4.2.0 Template Problem

[ml.dje@…]

The processLine function in maptemplate.c does not replace templates like
[layers] correctly. This is unique to the Linux version. There are no problems
with the Windows version.

If "...&layers=layer1+layer2+layer3..." is passed to mapserver via the URL, all
layers are  displayed, but only the last (layer3) is returned to the [layers]
variable in the template. The same happens with [layer_esc], [toggle_layers] etc.

Possible cause and solution:
The snprintf function used to replace [layers] with the actual values does not
work the way it should (on linux systems). I replaced snprintf by strcat, which
works perfectly for me, e.g.

-- strcpy(repstr, ""); // Layer list for a "POST" request
-- for(i=0;i<msObj->NumLayers;i++)
--    snprintf(repstr, PROCESSLINE_BUFLEN, "%s%s ", repstr, msObj->Layers[i]);
++ strcpy(repstr, ""); // Layer list for a "POST" request
++ for(i=0;i<msObj->NumLayers;i++) {
++    strcat(repstr, msObj->Layers[i]);
++    strcat(repstr, " "); // Layer list for a "POST" request
++ }

This fixes the problem on my system.

[sdlime]

Very wierd, that's quite old code. I'll go back a version or two to see what
changed. I don't see why the snprintf wouldn't work. It's certainly safer.

Steve

[m.spring@…]

template variable [layers] contains only last selected layer instead of all
layers. Reason is a snprintf error in maptemplate.c 

[dmorissette]

Using snprintf() this way is definitely not a good idea: it's a waste of CPU
cycles to copy the buffer over itself and as we see here it can lead to
problems. I see three instances of this problem in maptemplate.c

To prevent a buffer overflow, we could change the loop to something like the
following. I could take care of it if you want, just let me know:

char *repstrptr;
...

repstr[0] = '\0'; // Layer list for a "POST" request
repstrptr = repstr;
for(i=0;i<msObj->NumLayers;i++) {
   int itemlen;
   itemlen = strlen(msObj->Layers[i]);
   if ((repstrptr - repstr)+itemlen+1 >= PROCESSLINE_BUFLEN)
     break;  // Prevent buffer overflow!
   if (repstrptr != repstr)
     strcpy(repstrptr++, " ");  // Insert delimiter unless this is first item
   strcpy(repstrptr, msObj->Layers[i]);
   repstrptr += itemlen;
}

[sdlime]

Is this just a snprintf problem or an issue with sprintf as well? Someone 
changed all references from sprintf to snprintf all over the MapServer 
source. Maybe we need low-level function, a smarter strcat, instead. Something 
analogous to strcatalloc but for static buffers.

[sdlime]

Dan: Are you familiar with the function strlcat? Seems to do what we want. I'm
thinking it's not real common though, only available on BSDish systems. I'm
thinking your code could simply morph into a msStrlcat. I'll try that tonite...

Steve

[sdlime]

In absence of strlcat how about simply:

for(i=0;i<msObj->NumLayers;i++) {
  strncat(repstr, msObj->Layers[i], sizeof(repstr) - strlen(msObj->Layers[i]) - 1);
  strncat(repstr, " ", sizeof(repstr) - 2);
}

[dmorissette]

Ouch!  Mid-air collision!

I don't think using strncat as you suggest would work since the size argument to
strncat is the number of chars from the source string to copy: it has nothing to
do with the size of the target buffer. However using strlcat I think we could
turn the loop into:

repstr[0] = '\0'; // Layer list for a "POST" request
for(i=0;i<msObj->NumLayers;i++) {
  if (i>0) strlcat(repstr, " ", sizeof(repstr));
  strlcat(repstr, msObj->Layers[i], sizeof(repstr));
}


---

Here is what I had written before reading your last comment:

I had never heard of it, but strlcat() seems to be a nice/clean option. My code
maintains a pointer to the end of the string between iterations in the loop so
it couldn't be turned into a strlcat directly, but you can find a strlcat
implementation at ftp://ftp.openbsd.org/pub/OpenBSD/src/lib/libc/string/strlcat.c

There may be other places in the code where strlcat and strlcpy could be used to
deal more safely with string manipulations.

If we go that route we should also add a test to the configure script that would
use the system's strlcat/strlcpy if available.

[sdlime]

Plus I screwed up that code snippet. It should be:

repstr[0] = '\0';
for(i=0;i<msObj->NumLayers;i++) {
  if (i>0) strncat(repstr, " ", sizeof(repstr) - strlen(repstr) - 1);
  strncat(repstr, msObj->Layers[i], sizeof(repstr) - strlen(repstr) - 1);
}

It's easy to find strlcat implementations and we could handle just like 
strcasecmp in configure. However, what are the licensing implications of using 
it, are you familiar with the BSD license? Strncat is not too bad an option if 
we're careful with the size computation (always leaving room for the \0), and 
it's everywhere. If the license issue is no big deal then using strlcat seems 
like the safest way to go.

Steve

[sdlime]

Ok, the license for the strlcat looks fine. I added the function to mapstring.c
and updated configure.in to check for a local copy. The changes have been
checked into the development version. I still need to test on a non-BSD'ish
system. Will do tomorrow with Linux. Works great on my Mac. So, if others could
test that would be great. Once verified I'll apply the changes to the 4.2 branch.

Steve

[dmorissette]

Things seem to build fine on Linux.

Note to Assefa: you'll need to add -DNEED_STRLCAT to the Windows makefiles
(unless VC++ provides strlcat() which I doubt)

[sdlime]

Great, thanks for catching the Windows fix. I'll make the changes to the 4.2 
branch tonite.

[assefa]

Build properly on windows. makefile updated.

[m.spring@…]

Steve at al: This fix didn't make it into 4.2.1?
This would mean that 4.0.2 is the latest version usable as cgi under linux.

[sdlime]

Guess we'll need a 4.2.2 release soon, perhaps tonite. It may be that not all
linuxes are succeptible.

Steve

[dmorissette]

Makes sense since the fix was done only in 4.3 (CVS)... that's what I tested
when I wrote that things worked on Linux on comment #10.

Steve, does this mean that you'll backport this to the 4.2 branch?

[sdlime]

Yes. I had planned to originally but spaced it...

Steve

[sdlime]

I copied the changes from the 4.3 development version to the 4.2 branch. Seems
to work fine here, but should be testing on Windows. Once tested we can cut a
4.2.2 release (after Sean's tests ;-))...

Steve

[sdlime]

Ain't gonna backport anymore... ;-) Closing.

Steve