Opened 20 years ago

Closed 20 years ago

Last modified 20 years ago

#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:1 by sdlime, 20 years ago

Status: newassigned
Hmmm... I thought I fixed an issue similar to this in 4.1 already. I'll have a 
look though ASAP.

comment:2 by sdlime, 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 dmorissette, 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 dmorissette, 20 years ago

Milestone: 4.2 release

comment:5 by sdlime, 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 dmorissette, 20 years ago

Cc: steve.lime@… added
Owner: changed from sdlime to morissette@…
Status: assignednew
I'll take this one.

comment:7 by dmorissette, 20 years ago

Status: newassigned

comment:8 by dmorissette, 20 years ago

Resolution: fixed
Status: assignedclosed
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 sdlime, 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.