Opened 18 years ago

Closed 18 years ago

Last modified 18 years ago

#1897 closed defect (fixed)

Crashes in multipoint and multiline handling in mapgeos.c

Reported by: warmerdam Owned by: warmerdam
Priority: high Milestone:
Component: MapServer C Library Version: unspecified
Severity: normal Keywords:
Cc:

Description

The msGEOSGeometry2Shape_multiline() function was setting numlines=2 in 
the working shape, and then using msAddLine() which added new lines to the 
end of the shape.  This mean there would be uninitialized lines at the 
beginning of the shape. 

I have corrected this as follows:
***************
*** 389,396 ****
    msInitShape(shape);
  
    shape->type = MS_SHAPE_LINE;
-   shape->line = (lineObj *) malloc(sizeof(lineObj)*numLines);
-   shape->numlines = numLines;
    shape->geometry = (GEOSGeom) g;
  
    for(j=0; j<numLines; j++) {
--- 389,394 ----
***************
*** 407,415 ****
        /* GEOSCoordSeq_getZ(coords, i, &(line.point[i].z)); */  	
      }
  
!     msAddLine(shape, &line);
! 
!     free(line.point);
    }
  
    return shape;
--- 405,411 ----
        /* GEOSCoordSeq_getZ(coords, i, &(line.point[i].z)); */  	
      }
  
!     msAddLineDirectly(shape, &line);
    }
  
    return shape;

Also, the msGEOSGeometry2Shape_multipoint() function was assuming that
a multipoint object should be treated as a shapeObj with one point
per line in the loop but that it was one line with several points everywhere
else.  This was fixed this way:
*** mapgeos.c.~1.5.~	2006-09-05 11:10:50.000000000 -0400
--- mapgeos.c	2006-09-05 16:36:02.802273096 -0400
***************
*** 333,341 ****
      point = GEOSGetGeometryN(g, i);
      coords = GEOSGeom_getCoordSeq(point);
  
!     GEOSCoordSeq_getX(coords, 0, &(shape->line[i].point[0].x));
!     GEOSCoordSeq_getY(coords, 0, &(shape->line[i].point[0].y));
!     /* GEOSCoordSeq_getZ(coords, 0, &(shape->line[i].point[0].z)); */
    }
    
    return shape;
--- 333,341 ----
      point = GEOSGetGeometryN(g, i);
      coords = GEOSGeom_getCoordSeq(point);
  
!     GEOSCoordSeq_getX(coords, 0, &(shape->line[0].point[i].x));
!     GEOSCoordSeq_getY(coords, 0, &(shape->line[0].point[i].y));
!     /* GEOSCoordSeq_getZ(coords, 0, &(shape->line[0].point[i].z)); */
    }
    
    return shape;

The changes are committed in time for MapServer 4.10 beta3. 

The msautotest/mspython/wkt.py script tests these code paths.

Change History (6)

comment:1 by fwarmerdam, 18 years ago

Cc: steve.lime@… added
Owner: changed from sdlime to fwarmerdam
I'm fixing this.

comment:2 by fwarmerdam, 18 years ago

Resolution: fixed
Status: newclosed
Fixes committed.  

Steve, could you go over my changes to make sure you don't see any problems?


comment:3 by sdlime, 18 years ago

Whoops, thanks for catching... Will go over this evening.

Steve

comment:4 by sdlime, 18 years ago

Frank: Should we be using msAddLineDirectly everywhere in mapgeos.c? We're using
msAddLine in some places and the other elsewhere. Seems like when converting
from a geometry to shapeObj we should be able to use the "directly" version like
you changed in the patch- better performance I assume.

Steve

comment:5 by fwarmerdam, 18 years ago

Steve, 

I think there are lots of places where using the AddLineDirectly() would save
some copying and allocation/deallocation.  But I'd advise against making
this change for 4.10 unless you can easily exercise the code under valgrind
or something else guaranteed to immediately identify heap corruption issues.


comment:6 by sdlime, 18 years ago

I did make the changes in mapgeos.c so that we are consistent across all the
conversion routines. I agree that other places in the code would probably need a
good bit more investigation.

Steve
Note: See TracTickets for help on using tickets.