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

strdup() is deprecated under Windows #5277

Closed
wants to merge 1 commit into
base: branch-7-0
from

Conversation

Projects
None yet
3 participants
@tbonfort
Member

tbonfort commented Aug 27, 2016

While stepping through the MapServer source code in a Visual Studio debugger, every call to msStrdup caused the debugger to crash with heap errors.

I saw in the code comments that msStrdup was originally copied from GDAL. Looking at the current GDAL source and issues I found that CPLStrdup() had been updated for the same issue I am having with memory errors in Windows.

See - OSGeo/gdal#51 for details and discussions.

Could the MapServer source also be changed to match GDAL and allow debugging on Windows? I've not noticed any issues in production, but the VS debugger is much stricter with memory leaks.

I think only msStrdup need be changed as strdup is always wrapped in msStrdup, although free is used in several places, and should perhaps be msFree?

tbonfort added a commit to tbonfort/mapserver that referenced this pull request Aug 27, 2016

tbonfort added a commit to tbonfort/mapserver that referenced this pull request Aug 27, 2016

@tbonfort

This comment has been minimized.

Member

tbonfort commented Aug 27, 2016

@geographika please test this PR and comment

@geographika

This comment has been minimized.

Contributor

geographika commented Aug 28, 2016

@tbonfort - many thanks for looking into this.

I got the branch-7-0 MapServer branch and ran the following command to get your changes:

git pull git://github.com/tbonfort/mapserver.git issues/5277-free

I then rebuilt (in debug mode with no optimizations). The debugger got stuck in a loop, after checking some of the changes I noticed there was a self-calling function in mapfile.c included as part of the pull request:

#ifdef USE_MSFREE
void msFree(void *p)
{
  if(p) msFree(p);
}
#endif

I changed it to if(p) free(p); rebuilt and was able to step through the entire process of creating a WFS response from start to end, so it appears the issues will be resolved with this patch.

It will be a huge help to try and debug any other issues on Windows - so thanks again for looking at this.

tbonfort added a commit to tbonfort/mapserver that referenced this pull request Aug 29, 2016

@tbonfort tbonfort added this to the 7.0.2 Release milestone Aug 29, 2016

@tbonfort tbonfort self-assigned this Aug 29, 2016

@geographika

This comment has been minimized.

Contributor

geographika commented Aug 29, 2016

I have run through a few more code paths, which have triggered the same exceptions as I found originally. I was incorrect in my first post that all strdup calls were wrapped in msStrdup - there are several cases where strdup is used on its own e.g.

https://github.com/mapserver/mapserver/blob/branch-7-0/maputil.c#L1508
https://github.com/mapserver/mapserver/blob/branch-7-0/maputil.c#L1481

Would there be any side effects in replacing all these with msStrdup throughout the codebase?

@tbonfort

This comment has been minimized.

Member

tbonfort commented Aug 30, 2016

Seth, you can open a pr with the changes. There should be no side effect.

@jmckenna

This comment has been minimized.

Member

jmckenna commented Sep 19, 2016

I've stepped through branch-7-0 with Visual Studio 2015 with the Debugger, with no issues. Thanks @geographika !

@jmckenna jmckenna closed this Sep 20, 2016

sdlime added a commit to sdlime/mapserver that referenced this pull request Jul 31, 2017

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