New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infinite Loop(?) with msSHPLayerNextShape #5108

Closed
akrherz opened this Issue Jun 17, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@akrherz
Contributor

akrherz commented Jun 17, 2015

I am testing out current master mapserver on RHEL7 64 bit. I am having a problem with what appears to be an infinite loop with mapshape.c#msSHPLayerNextShape. I suspect that the underlying shapefile is being updated while mapserver attempts to read from it.

The current mapshape.c has this:

int msSHPLayerNextShape(layerObj *layer, shapeObj *shape)
{
 //snipped

  msSHPReadShape(shpfile->hSHP, i, shape);
  if(shape->type == MS_SHAPE_NULL) {
    msFreeShape(shape);
    msSHPLayerNextShape(layer, shape); /* skip NULL shapes */
  }

whereas 6.4.1 has this (which I don't see this issue with)

  do {
  //snipped

    msSHPReadShape(shpfile->hSHP, i, shape);
    if(shape->type == MS_SHAPE_NULL) {
      msFreeShape(shape);
      continue; /* skip NULL shapes */
    }
}

So I am wondering if msSHPLayerNextShape is in a recursive loop. The mapserv CGI process spins 100% CPU and eats more and more memory when this happens. Thanks

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Jun 17, 2015

Member

The change in the code (removing the do-while loop) is because FILTERing was moved out to the layer-level msLayerNextShape() function as it applies to all driver types. Functionally it should be the same. This one is going to be impossible to debug with out a test case - any chance you have something that is repeatable?

Steve

Member

sdlime commented Jun 17, 2015

The change in the code (removing the do-while loop) is because FILTERing was moved out to the layer-level msLayerNextShape() function as it applies to all driver types. Functionally it should be the same. This one is going to be impossible to debug with out a test case - any chance you have something that is repeatable?

Steve

@rouault

This comment has been minimized.

Show comment
Hide comment
@rouault

rouault Jun 17, 2015

Contributor

@sdlime Probably that prefixing "msSHPLayerNextShape(layer, shape); /* skip NULL shapes */" with "return" would be better (otherwise you're going to set shape->values multiple times). But this this assumes that the compiler handles tail recursion correctly. The loop approach was safer in this respect

Contributor

rouault commented Jun 17, 2015

@sdlime Probably that prefixing "msSHPLayerNextShape(layer, shape); /* skip NULL shapes */" with "return" would be better (otherwise you're going to set shape->values multiple times). But this this assumes that the compiler handles tail recursion correctly. The loop approach was safer in this respect

@akrherz

This comment has been minimized.

Show comment
Hide comment
@akrherz

akrherz Jun 17, 2015

Contributor

@sdlime I can reproduce this at-will with my production system testing, but not on my dev laptop in isolation :). I am currently testing with this patch as suggested by @rouault

diff --git a/mapshape.c b/mapshape.c
index e139b5e..174407c 100644
--- a/mapshape.c
+++ b/mapshape.c
@@ -2688,7 +2688,7 @@ int msSHPLayerNextShape(layerObj *layer, shapeObj *shape)
   msSHPReadShape(shpfile->hSHP, i, shape);
   if(shape->type == MS_SHAPE_NULL) {
     msFreeShape(shape);
-    msSHPLayerNextShape(layer, shape); /* skip NULL shapes */
+    return msSHPLayerNextShape(layer, shape); /* skip NULL shapes */
   }
   shape->numvalues = layer->numitems;
   shape->values = msDBFGetValueList(shpfile->hDBF, i, layer->iteminfo, layer->numitems);

and am not seeing the issue, but will watch it closely.

Contributor

akrherz commented Jun 17, 2015

@sdlime I can reproduce this at-will with my production system testing, but not on my dev laptop in isolation :). I am currently testing with this patch as suggested by @rouault

diff --git a/mapshape.c b/mapshape.c
index e139b5e..174407c 100644
--- a/mapshape.c
+++ b/mapshape.c
@@ -2688,7 +2688,7 @@ int msSHPLayerNextShape(layerObj *layer, shapeObj *shape)
   msSHPReadShape(shpfile->hSHP, i, shape);
   if(shape->type == MS_SHAPE_NULL) {
     msFreeShape(shape);
-    msSHPLayerNextShape(layer, shape); /* skip NULL shapes */
+    return msSHPLayerNextShape(layer, shape); /* skip NULL shapes */
   }
   shape->numvalues = layer->numitems;
   shape->values = msDBFGetValueList(shpfile->hDBF, i, layer->iteminfo, layer->numitems);

and am not seeing the issue, but will watch it closely.

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Jun 17, 2015

Member

Ignore my first comment. Actually returning NULL isn't possible given the function sig. Could return MS_SUCCESS if shape->type=MS_NULL_SHAPE then add the:

if(shape->type == MS_SHAPE_NULL) {
msFreeShape(shape);
msLayerNextShape(layer, shape); /* skip NULL shapes */
}

Check to msLayerNextShape()...However, null shapes see to be a shapefile phenomenon only aren't they?

Steve

Member

sdlime commented Jun 17, 2015

Ignore my first comment. Actually returning NULL isn't possible given the function sig. Could return MS_SUCCESS if shape->type=MS_NULL_SHAPE then add the:

if(shape->type == MS_SHAPE_NULL) {
msFreeShape(shape);
msLayerNextShape(layer, shape); /* skip NULL shapes */
}

Check to msLayerNextShape()...However, null shapes see to be a shapefile phenomenon only aren't they?

Steve

@rouault

This comment has been minimized.

Show comment
Hide comment
@rouault

rouault Jun 17, 2015

Contributor

OGR datasources could return NULL shapes (haven't checked the OGR mapserver provider, but OGR can certainly return a feature without geometry)

Contributor

rouault commented Jun 17, 2015

OGR datasources could return NULL shapes (haven't checked the OGR mapserver provider, but OGR can certainly return a feature without geometry)

@akrherz

This comment has been minimized.

Show comment
Hide comment
@akrherz

akrherz Jun 18, 2015

Contributor

I have been running the patch mentioned above and am not seeing the CPU/memory usage issue. I am unsure if there are other side effects or not.

Contributor

akrherz commented Jun 18, 2015

I have been running the patch mentioned above and am not seeing the CPU/memory usage issue. I am unsure if there are other side effects or not.

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Jun 18, 2015

Member

What do you think @rouault, should we go with the simple patch? Probably safest... --Steve

Member

sdlime commented Jun 18, 2015

What do you think @rouault, should we go with the simple patch? Probably safest... --Steve

@rouault

This comment has been minimized.

Show comment
Hide comment
@rouault

rouault Jun 18, 2015

Contributor

The patch with the added return, you mean ? Yes, that shouldn't hurt (and hopefully in optimized mode the compiler will turn the call into a jump)

Contributor

rouault commented Jun 18, 2015

The patch with the added return, you mean ? Yes, that shouldn't hurt (and hopefully in optimized mode the compiler will turn the call into a jump)

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Jun 25, 2015

Member

Calling this one closed.@akherz, thanks for the detailed bug report and @rouault for the easy fix. --Steve

Member

sdlime commented Jun 25, 2015

Calling this one closed.@akherz, thanks for the detailed bug report and @rouault for the easy fix. --Steve

@sdlime sdlime closed this Jun 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment