#569 closed defect (fixed)
Potential crash with nquery
Reported by: | dmorissette | Owned by: | dmorissette |
---|---|---|---|
Priority: | high | Milestone: | 4.2 release |
Component: | MapServer C Library | Version: | 4.1 |
Severity: | normal | Keywords: | |
Cc: |
Description
Steve, I have a layer that crashes MapServer when doing a 'nquery' with the mapserv CGI (v4.1 or 4.0). I'll try to explain the issue to double-check my fix with you since I don't want to break something in fixing this. I could email you data to reproduce this if you want but can't attach it to this bug. Here is the layer that causes the crash: LAYER NAME "TEST.DGN" STATUS DEFAULT TYPE POLYGON CONNECTIONTYPE OGR CONNECTION "TEST.DGN" CLASSITEM "Level" CLASS EXPRESSION "60" OUTLINECOLOR 255 0 0 COLOR 0 255 0 # in polygon geen text? TEXT ([Level] [MSLink]) LABEL TYPE TRUETYPE FONT "fritqat" SIZE 12 COLOR 0 255 255 END END TOLERANCE 10 TEMPLATE dgn_xml.html END The crash happens in DrawQueryMap() but the actual problem happens in msQueryByPoint(). In this specific case, we have a template at the layer level, and some objects from the file don't match any of the CLASS expressions. So we end up with shapes in the resultcache that contain shape.classindex=-1, and then accessing layer->class[-1] crashes. I have copied below the code from msQueryByPoint() that lets the shapes with shape.classindex=-1 pass. I think that the correct fix would be to always reject a shape if shape.classindex == -1, even if lp->template is set. Is there a reason why the test below lets all shapes pass if lp->template is set? while((status = msLayerNextShape(lp, &shape)) == MS_SUCCESS) { // step through the shapes shape.classindex = msShapeGetClass(lp, &shape, map->scale); if(!(lp->template) && ((shape.classindex == -1) || (lp->class[shape.classindex].status == MS_OFF))) { // not a valid shape msFreeShape(&shape); continue; }
Change History (9)
comment:2 by , 20 years ago
One could write a layer like: LAYER NAME test TYPE POLYGON TEMPLATE 'test.html' END So it is conceivable that a shape could not belong to a class. This is real common when developing query-only mapfiles. I think I have an example at work. I gotta think about this. It's tough, is a template declared at the layer level global or not. As it sits now it is. What happens if you move the template into the class? I'm thinking the fix should be in the drawing code, make sure there is a valid class before drawing. That will ensure that queries don't break. Other thoughts? Steve
comment:3 by , 20 years ago
Oh... I didn't realize that we wanted objects to be queryable even if they don't belong in any class. I always thought that nothing could happen without a class in MapServer, and I'm sure that's the way many users have been thinking until now, so we should document this behavior properly for sure. About query-only layers, it would be easy enough to create one this way: LAYER NAME test TYPE POLYGON TEMPLATE 'test.html' CLASS END END It seems to me that the behavior that you suggest is not consistent with the drawing behavior which requires classes. There is also something odd that happens if you configure your layer for drawing with 3 classes (with 3 expressions), and set the template at the layer level. Then you can have objects that aren't drawn (not in any class) but that are displayed in query results. Actually, what happens in DrawQueryMap() when an object is not in any class but is part of query results this way? Should it be drawn or not?
comment:4 by , 20 years ago
Milestone: | → 4.2 release |
---|
comment:5 by , 20 years ago
We should probably try to fix this in the if/then sections of the various query functions. Gotta think what that condition should be though... Steve
comment:6 by , 20 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | assigned → new |
I'll take this one.
comment:7 by , 20 years ago
Status: | new → assigned |
---|
comment:8 by , 20 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Fixed. I couldn't find my old testcase for this, and as I was trying to build a new testcase based on the itasca demo, I realized that this crash happens *only if* you have a QUERYMAP enabled in your mapfile. I guess this was a good thing since I really didn't feel like going around all the mapserver code adding tests everywhere shape.classindex was used. All I did to fix this was skip any shape with classindex==-1 inside msDrawQueryLayer(). Later on I finally found my original testcase and could confirm that the fix works for this one as well. <testcase> To reproduce this using itasca: - Add a QUERYMAP to the mapfile: QUERYMAP STATUS ON STYLE HILITE END - Modify the mcd90py2 layer to move the template outside of the class, and increase tolerance to 50 pixels. - A nquery anywhere on the map would trigger the crash. </testcase>
comment:9 by , 20 years ago
We eventually may want to fix this in query functions so it can't happen. That is, if a layer is drawable then a class of -1 shouldn't be allowed. Your fix seems prudent at this point though. The query code should get an overhaul anyway to remove the multiple passes through sources and we can address then. Steve
Note:
See TracTickets
for help on using tickets.