Check error returns from mapstring functions.

[russellmcormond]

Most of the string functions in mapstring.c return either a malloc'd string (possibly via strdup()) or a NULL if there was an error. Unfortunately, not all usage of these strings checks for the NULL return, and there are cases where this NULL is used as a string. Using NULL as a string will cause a core dump.

Example:

There is a configuration problem with older versions of the gd graphics library which will incorrectly enable a stub for iconv_open() which always returns (-1)  http://cvs.php.net/viewvc.cgi/gd/libgd/src/gdkanji.c?view=markup#l27 . This means that any mapstring function that tries to convert character encoding will return NULL.

I experienced this in the context of data stored in ArcSDE which is encoded in Unicode and thus uses msConvertWideStringToUTF8(). We then tried to use an EXPRESSION in the mapfile. When msEvalExpression() tried to manipulate the NULL via strdup() the mapserver would core dump.

I simulated the error by editing the function msConvertWideStringToUTF8 and changing the line that opens iconv to:

cd = (iconv_t)-1; /* iconv_open("UTF-8", encoding) */

I then compiled (with debug symbols enabled), and ran shp2img within gdb.

(gdb) run -m /data/ndevl18/apps/basemap/map/basemap_wms_en.map 
-o /home/russell/Desktop/out.png
Starting program: /server/downloads/src/mapserver-5.2.2/shp2img 
-m /data/ndevl18/apps/basemap/map/basemap_wms_en.map -o /home/russell/Desktop/out.png
[Thread debugging using libthread_db enabled]
[New Thread 0xb7f066e0 (LWP 5252)]

Program received signal SIGSEGV, Segmentation fault.
0x01568993 in strlen () from /server/ndevl18/apache-2.2.9/lib/libc.so.6
(gdb) backtrace
#0  0x01568993 in strlen () from /server/ndevl18/apache-2.2.9/lib/libc.so.6
#1  0x015686d5 in strdup () from /server/ndevl18/apache-2.2.9/lib/libc.so.6
#2  0x08064244 in msEvalExpression (expression=0x0, itemindex=1,
    items=0x9742940, numitems=3) at maputil.c:367
#3  0x08064543 in msShapeGetClass (layer=0x964bf48, shape=0xbf960ba8,
    scaledenom=29968673.278235532, classgroup=0x0, numclasses=0)
    at maputil.c:518
#4  0x08077d14 in msDrawVectorLayer (map=0x9636aa8, layer=0x964bf48,
    image=0x9678720) at mapdraw.c:881
#5  0x08078243 in msDrawLayer (map=0x9636aa8, layer=0x964bf48, image=0x9678720)
    at mapdraw.c:738
#6  0x0807922b in msDrawMap (map=0x9636aa8, querymap=0) at mapdraw.c:441
#7  0x08052286 in main (argc=5, argv=0xbf960e94) at shp2img.c:295
(gdb)

We would never see the msSetError() message as the system would core dump before being able to flush the error message to the log. We need to ensure that whenever error conditions are detected that mapserver will fail gracefully and allow the user to fix what caused the error condition.

[russellmcormond]

I'm wondering if this is something that will be looked at. We appear to have problems with character encoding again (iconv not quite set up right) and rather than sending out an error about the issue, Mapserver is just core dumping with a segmentation fault.

/mapserv/httpd/cgi-bin/mapserv -v MapServer version 5.4.0 OUTPUT=GIF OUTPUT=PNG OUTPUT=JPEG OUTPUT=WBMP OUTPUT=PDF OUTPUT=SVG SUPPORTS=PROJ SUPPORTS=AGG SUPPORTS=FREETYPE SUPPORTS=ICONV SUPPORTS=WMS_SERVER SUPPORTS=WMS_CLIENT SUPPORTS=WFS_SERVER SUPPORTS=WFS_CLIENT INPUT=EPPL7 INPUT=SDE INPUT=ORACLESPATIAL INPUT=OGR INPUT=GDAL INPUT=SHAPEFILE

(gdb) run "QUERY_STRING=mode=map&layers=apa_reu100"
Starting program: /home/mcormondr/test-mapserv/mapserv/httpd-2.2.11/cgi-bin/mapserv "QUERY_STRING=mode=map&layers=apa_reu100"
...
Program received signal SIGSEGV, Segmentation fault.
0x08064700 in msConvertWideStringToUTF8 ()
(gdb) backtrace
#0  0x08064700 in msConvertWideStringToUTF8 ()
#1  0x08136e77 in sdeGetRecord ()
#2  0x08137671 in msSDELayerNextShape ()
#3  0x0808f357 in msLayerNextShape ()
#4  0x080a06b5 in msDrawVectorLayer ()
#5  0x080a0eaf in msDrawLayer ()
#6  0x080a215b in msDrawMap ()
#7  0x08054ea9 in main ()
(gdb)

[russellmcormond]

The following are some examples that I am thinking of.

The following puts a 0-length string as the value rather than a NULL, given later functions assume that the value is a string and just work with it (and if NULL, then would cause a segfault).

--- mapsde.c-orig       2009-02-05 11:20:16.000000000 -0500
+++ mapsde.c    2010-03-17 16:14:55.000000000 -0400
@@ -753,6 +753,13 @@
                     else
                         shape->values[i] = msConvertWideStringToUTF8((const wchar_t*) wide, "UTF-16LE");
                     msFree(wide);
+                   if (!shape->values[i]) {  /* There was an error */
+                     msSetError( MS_SDEERR,
+                                 "msConvertWideStringToUTF8()==NULL.",
+                                 "sdeGetRecord()");
+                     shape->values[i] = (char *)malloc(itemdefs[i].size*sizeof(char)+1);
+                     shape->values[i][0] = '\0'; /* empty string */
+                   }
                 }
                 break;
 #endif

In this example an error was detected, but it doesn't return NULL. Instead if continued with the next thing being an attempt to null-terminate the string which was a null pointer. This would also cause a segfault.

--- mapstring.c-orig    2010-03-17 16:43:19.000000000 -0400
+++ mapstring.c 2010-03-18 09:59:16.000000000 -0400
@@ -1556,6 +1556,7 @@
             msSetError(MS_MISCERR, "Unable to convert string in encoding '%s' to UTF8",
                                    "msConvertWideStringToUTF8()",
                                    encoding);
+           return NULL;
         }
     } else {
         /* we were given a NULL wide string, nothing we can do here */

[russellmcormond]

I have re-arranged things a bit, and added in some additional error reporting to mapstring.c as indicated below. This should be done in addition to the patch suggested to mapsde.c in the previous comment.

--- mapstring.c-orig    2010-03-17 16:43:19.000000000 -0400
+++ mapstring.c 2010-03-18 11:54:02.000000000 -0400
@@ -1504,8 +1504,8 @@
 char* msConvertWideStringToUTF8 (const wchar_t* string, const char* encoding) {
 #ifdef USE_ICONV

-    int bconvFailed = MS_TRUE;
     char* output = NULL;
+    char* errormessage = NULL;
     iconv_t cd = NULL;
     size_t nStr;
     size_t nInSize;
@@ -1540,23 +1540,36 @@
            pszUTF8 = output;
            pwszWide = string;
            nConv = iconv(cd, (char **)&pwszWide, &nInSize, &pszUTF8, &nOutSize);
-           if ((size_t)-1 != nConv &&  nOutSize != nBufferSize)
-               bconvFailed = MS_FALSE;
+           if ((size_t)-1 != nConv &&  nOutSize != nBufferSize) {
+            switch (errno) {
+              case E2BIG:
+                errormessage = "There is not sufficient room in buffer";
+                break;
+              case EILSEQ:
+                errormessage = "An invalid multibyte sequence has been encountered in the input";
+                break;
+              case EINVAL:
+                errormessage = "An incomplete multibyte sequence has been encountered in the input";
+                break;
+              default:
+                errormessage = "Unknown";
+                break;
+            }
+            msSetError(MS_MISCERR, "Unable to convert string in encoding '%s' to UTF8 %s",
+                                   "msConvertWideStringToUTF8()",
+                       encoding,errormessage);
+            iconv_close(cd);
+            msFree(output);
+            return NULL;
+          }
            iconv_close(cd);
         } else {
             msSetError(MS_MISCERR, "Encoding not supported by libiconv (%s).",
                                    "msConvertWideStringToUTF8()",
                                    encoding);
+           msFree(output);
             return NULL;
         }
-
-        if (bconvFailed) {
-            msFree(output);
-            output = NULL;
-            msSetError(MS_MISCERR, "Unable to convert string in encoding '%s' to UTF8",
-                                   "msConvertWideStringToUTF8()",
-                                   encoding);
-        }
     } else {
         /* we were given a NULL wide string, nothing we can do here */
         return NULL;

This deals with a few things:

a) there was a missing msFree(output) when the encoding wasn't supported by iconv() (IE: memory had already been allocated at that point).

b) I moved the msSetError to being where the error was caused, rather than lower -- the bconvFailed variable was only set in this one place. I also made sure it returned NULL, something that was previously missing.

c) I added in a switch (errno) to set an error string so that we would know the reason why iconv() was failing, which helps with diagnostics of data/etc issues.

[pramsey]

I've committed the patches referenced here into 5.4 (r9955) 5.6 (r9956) and trunk (r9957). There is one other place where iconv is called and the errors are caught but not reported in mapstring.c. I will open up a new ticket (#3395) on that, this one is kind of crufty.

[russellmcormond]

I'm not certain if the patch we made to mapstring.c is the right one. I didn't do extensive enough testing at my end. There are scenarios which fail with this patch which shouldn't, and the logic doesn't quite make sense.

Looking at  http://www.gnu.org/software/libiconv/documentation/libiconv/iconv.3.html the logic of the line:

if ((size_t)-1 != nConv && nOutSize != nBufferSize)

isn't right to determine when the conversion failed. In fact, we should only be looking at errno when (nConv == (size_t)-1).

I may have more to report later, but thought I would pass this onward sooner rather than waiting on a resolution. I need to be spending my time in the short term figuring out the specific problem my client is having.

[pramsey]

Thanks for re-looking at this, it was indeed backwards which would not be helpful at all. Fixed up in trunk (r9976), 5.4 (r9977) and 5.6 (r9975).