double-free on query->filteritem with ogcapi (and africa.fgb)

extracted from https://github.com/MapServer/MapServer/issues/6550#issuecomment-1170493133

im chasing a double-free fully reproducible using OGCAPI, using a query trying to access a random value which would be 'outside' the datafile, eg

PATH_INFO=/ign/ogcapi/collections/africa/items/1244

the mapfile has 4 layers, the africa layer is the 3rd reuses the fgb present in the repo for msautotest:

    LAYER
        NAME "africa"
        TYPE polygon
        METADATA
        "ows_title" "africa"
          "oga_include_items" "pop_est,name_en"
          "oga_featureid" "id"
        END
                EXTENT -17.62 -34.81 51.139 37.4
        CONNECTIONTYPE flatgeobuf
      DATA "/home/landry/src/mapserver/msautotest/misc/data/africa.fgb"
        PROJECTION "init=epsg:4326" END
        CONNECTIONOPTIONS "VERIFY_BUFFERS" "NO" END
        TEMPLATE VOID
        END

at runtime on OpenBSD, i get this double-free message coming from our own malloc that does memory corruption detection:

$./mapserv PATH_INFO=/ign/ogcapi/collections/africa/items/1244                                                               
Content-Type: application/json
Status: 404

{"code":"NotFound","description":"Collection items id query failed."}
mapserv(53919) in free(): chunk is already free 0xc2646c44ff0
Abort trap (core dumped)

strangely, trying to access all items in the layer also fails:

./mapserv PATH_INFO=/ign/ogcapi/collections/africa/items 
Content-Type: application/json
Status: 500

{"code":"ConfigError","description":"Template rendering error. [inja.exception.render_error] (at 52:22) variable 'response.features.0.properties' not found (collection-items.html)."}

i have no issue accessing another fgb datafile (points layer) with the same kind of queries, be it all items or a single item by its id.

looking at the code, it seems filteritem pointers are at some point shared by the query object and the layer object:

$git grep filterit|grep -i free
mapfile.c:  msFree(layer->filteritem);
maplayer.c:    msFree(layer->filteritem);
maplayer.c:    if(lp->filteritem) free(lp->filteritem);
maplayer.c:      free(lp->filteritem);
mapquery.c:  if(query->filteritem) free(query->filteritem);

so maybe one of those is freeing too much ? apparently in the code using filteritem here and there, sometimes query-filteritem pointer is assigned to the layer object, which might already free it ?

Expected behavior and actual behavior.

crash with double-free

Steps to reproduce the problem.

./mapserv PATH_INFO=/ign/ogcapi/collections/africa/items/1244

Attach simple test case (drag/drop it here)

in mapserver.conf i have this section for ogcapi mapping to mapfile:

  MAPS
    "ign" "/home/landry/gis/mapfile/ign.map"
  END

zip with mapfile and two working fgb files below: data.zip

Operating system

OpenBSD - note that we have stronger memory constraints than on other operating systems, so would explain why the bug cant be reproduced on other like linux - but clang/llvm asan on valgrind should show the double-free too.

MapServer version and installation method

master

my intutition was good, because using gdb and putting a breakpoint on msFreeMap i can clearly see that the layer object and the query object share the same pointer for filteritem:

Content-Type: application/json
Status: 404

{"code":"NotFound","description":"Collection items id query failed."}

Breakpoint 1, msFreeMap (map=0x145dffe5c00) at /home/landry/src/mapserver/mapobject.c:78
78      {
(gdb) n
81        if(!map) return;
(gdb) p map->layers[2]->filteritem
$16 = 0x145a5851fd0 "id"
(gdb) p map->query->filteritem
$17 = 0x145a5851fd0 "id"

in my mapfile, layer[2] is the africa layer which has "oga_featureid" "id" - that probably explains where the filteritem value comes from.

so when the layers array is freed, layer->filteritem is freed and the pointer pointing at the id string has been garbage collected by the OS, and when msFreeQuery is called it tries again to free that same pointer.

dunno what would be specific to ogcapi there..

fwiw i cant reproduce the issue using WFS 2.0:

./mapserv 'QUERY_STRING=map=/home/landry/gis/mapfile/ign.map&request=GetFeature&typename=africa&service=wfs&version=2.0.0&filter=<Filter><PropertyIsEqualTo><PropertyName>id</PropertyName><Literal>1144</Literal></PropertyIsEqualTo></Filter>'
Content-Type: text/xml; subtype="gml/3.2.1"; charset=UTF-8

<?xml version='1.0' encoding="UTF-8" ?>
<wfs:FeatureCollection
   xmlns:ms="http://mapserver.gis.umn.edu/mapserver"
   xmlns:gml="http://www.opengis.net/gml/3.2"
   xmlns:wfs="http://www.opengis.net/wfs/2.0"
   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
   xsi:schemaLocation="http://mapserver.gis.umn.edu/mapserver http://localhost/cgi-bin/mapserv?SERVICE=WFS&amp;VERSION=2.0.0&amp;REQUEST=DescribeFeatureType&amp;TYPENAME=africa&amp;OUTPUTFORMAT=application%2Fgml%2Bxml%3B%20version%3D3.2 http://www.opengis.net/wfs/2.0 http://schemas.opengis.net/wfs/2.0/wfs.xsd http://www.opengis.net/gml/3.2 http://schemas.opengis.net/gml/3.2.1/gml.xsd"
   timeStamp="2022-08-03T19:37:39" numberMatched="0" numberReturned="0">
</wfs:FeatureCollection>

will have to check what filteritem is in this case.

single-stepping in msQueryByFilter, i see that there's a line that copies the query->filteritem ptr to lp->filteritem, while another line 'caches' the previous value in old_filteritem. In my experience, the line copying back old_filteritem value to lp->filteritem in https://github.com/MapServer/MapServer/blob/main/mapquery.c#L893 or https://github.com/MapServer/MapServer/blob/main/mapquery.c#L985 isnt called (why ? wild guess: because there's no overlap in the query bbox and the reprojected data bbox ?), which explains why the layer object filteritem member still points at query->filteritem value.

gdb logs:

Breakpoint 8, msQueryByFilter (map=0xe09f4331000) at /home/landry/src/mapserver/mapquery.c:818                                                                                                                       
818         old_filteritem = lp->filteritem; /* cache the existing filter/filteritem */                                                                                                                              
(gdb) p map->query                                                                                                                                                                                                   
$10 = {type = 6, mode = 0, layer = 2, point = {x = -1, y = -1, z = 0, m = 0}, buffer = 0, maxresults = 0, rect = {minx = 655770, miny = 6557370, maxx = 771860, maxy = 6619120}, shape = 0x0, shapeindex = -1,       
  tileindex = -1, clear_resultcache = 1, maxfeatures = -1, startindex = -1, only_cache_result_count = 0, filter = {string = 0xe09390dcac0 "1244", type = 2002, flags = 0, tokens = 0x0, curtoken = 0x0, regex = {    
      sys_regex = 0x0}, compiled = 0, native_string = 0x0}, filteritem = 0xe09c857ff60 "id", slayer = -1, cache_shapes = 0, max_cached_shape_count = 0, max_cached_shape_ram_amount = 0}
(gdb) p map->layers[1]->filteritem
$11 = 0x0
(gdb) p map->layers[2]->filteritem                                                                         
$12 = 0x0
(gdb) n
819         msInitExpression(&old_filter);
(gdb) 
820         msCopyExpression(&old_filter, &lp->filter);
(gdb) 
825         lp->filteritem = map->query.filteritem; /* re-point lp->filteritem */
(gdb) 
826         if(old_filter.string != NULL) { /* need to merge filters to create one logical expression */
(gdb) 
834           msCopyExpression(&lp->filter, &map->query.filter); /* apply new filter */
(gdb) 
838         status = msLayerWhichItems(lp, MS_TRUE, NULL);
(gdb) p lp->filteritem
$13 = 0xe09c857ff60 "id"
(gdb) n
839         if(status != MS_SUCCESS) goto restore_old_filter;
(gdb) 
841         search_rect = map->query.rect;
(gdb) 
845         if( map->query.only_cache_result_count &&
(gdb) 
902         lp->project = msProjectionsDiffer(&(lp->projection), &(map->projection));
(gdb) 
903         if(lp->project && memcmp( &search_rect, &invalid_rect, sizeof(search_rect) ) != 0 )
(gdb) 
904           msProjectRect(&(map->projection), &(lp->projection), &search_rect); /* project the searchrect to source coords */
(gdb) 
906         status = msLayerWhichShapes(lp, search_rect, MS_TRUE);
(gdb) 
907         if(status == MS_DONE) { /* no overlap */
(gdb) 
908           msLayerClose(lp);
(gdb) 
909           continue;
(gdb) 
997       for(l=start; l>=stop; l--) {
(gdb) 
998         if(GET_LAYER(map, l)->resultcache && GET_LAYER(map, l)->resultcache->numresults > 0)

as for bboxes, hint from the mapfile: the MAP EXTENT is 655770 6557370 771860 6619120 in EPSG:2154 (that's the values seen in query->rect) and the data EXTENT is -17.62 -34.81 51.139 37.4 in EPSG:4326 so cant work ?

more hints from gdb when looking at bboxes:

841         search_rect = map->query.rect;
(gdb) 
845         if( map->query.only_cache_result_count &&
(gdb) 
902         lp->project = msProjectionsDiffer(&(lp->projection), &(map->projection));
(gdb) 
903         if(lp->project && memcmp( &search_rect, &invalid_rect, sizeof(search_rect) ) != 0 )
(gdb) 
904           msProjectRect(&(map->projection), &(lp->projection), &search_rect); /* project the searchrect to source coords */
(gdb) p search_rect
$19 = {minx = 655770, miny = 6557370, maxx = 771860, maxy = 6619120}
(gdb) n
906         status = msLayerWhichShapes(lp, search_rect, MS_TRUE);
(gdb) p search_rect
$20 = {minx = 2.421444208436268, miny = 46.112316136776123, maxx = 3.939945725106849, maxy = 46.672163228253922}
(gdb) p lp->extent
$21 = {minx = -17.620000000000001, miny = -34.810000000000002, maxx = 51.139000000000003, maxy = 37.399999999999999}
(gdb) n
907         if(status == MS_DONE) { /* no overlap */
(gdb) p status
$22 = 2 '\002'

the double-free disappears if i set the global MAP EXTENT to something which includes the africa.fgb dataset bbox (like EXTENT -1816726 -5701989 9313866 6619120 still w/ EPSG:2154) and i can properly browse items from this dataset using ogcapi.

so the bug is caused by invalid incompatible extents between the map and the layer - in which case the old_filteritem should be restored anyway. i think goto restore_old_filter should be called in the MS_DONE case here: https://github.com/MapServer/MapServer/blob/main/mapquery.c#L907 ?

MapServer / MapServer