Skip to content
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

Memleak fixes #516

Merged
merged 2 commits into from May 22, 2017

Conversation

Projects
None yet
2 participants
@rouault
Copy link
Member

commented May 20, 2017

@busstoptaktik I'm not completely sure about the change in pj_init(). It could be a problem if a (*proj)(PIN) function needed the ->geod member, but it doesn't look like this is the case. Annther issue, which existed before, is that in the unlikely case PIN->geod = pj_calloc(...) would fail, we would return a valid PIN pointer. So code later that would need geod could crash

@rouault rouault requested a review from busstoptaktik May 20, 2017

@busstoptaktik
Copy link
Member

left a comment

Two interesting bugs, and two nice and simple repairs. I do, however, suggest to change the freeup reapir to be more in line with my ongoing attempts to simplify the overall memory management (see comment inlined in the patch)

@@ -30,6 +30,11 @@ static LP s_inverse (XY xy, PJ *P) { /* Spheroidal, inverse */


static void *freeup_new (PJ *P) { /* Destructor */

This comment has been minimized.

Copy link
@busstoptaktik

busstoptaktik May 22, 2017

Member

I suggest using the simplified approach used in PJ_cart.c: Remove freeup_new(), and replace freeup() with the version from PJ_cart.c:

static void freeup (PJ *P) {
    pj_freeup_plain (P);
    return;
}

Later on, I expect to improve the PROJECTION macro, to replace freeup with pj_freeup_plain. It will, however require one pass over the code, cleaning up every PJ_*.c file (but it will result in removing most cases of freeup-functions, and a bit of init-code for each projection, at no cost at all (in the few cases where freeup does more advanced things than freeing P->opaque, then freeing P, we will have to add one line: P->pfree = freeup;)

This comment has been minimized.

Copy link
@rouault

rouault May 22, 2017

Author Member

OK, I pasted that from another file. Fixed from your suggestion

@@ -685,6 +680,12 @@ pj_init_ctx(projCtx ctx, int argc, char **argv) {
}
PIN = 0;
}
else {

This comment has been minimized.

Copy link
@busstoptaktik

busstoptaktik May 22, 2017

Member

Yes - this seems to repair a genuine blunder (although it would be nice if geod_init included a check for null pointers, but I think, given that the geod-code seems to be autotranslated from the C++-code of GeographicLib, would be too much to ask for, so this solution is definitely preferrable)

pj_init(): fix memory leak of pj->geod
Whe PIN = (*proj)(PIN) fails, it doesn't free the geod member.
So allocate it afterwards.
Credit to OSS Fuzz

@rouault rouault force-pushed the rouault:memleak_fixes branch from 5bcdda4 to f224edc May 22, 2017

@rouault rouault force-pushed the rouault:memleak_fixes branch from f224edc to 9422539 May 22, 2017

@rouault rouault merged commit 4e3bd52 into OSGeo:master May 22, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@rouault rouault deleted the rouault:memleak_fixes branch May 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.