Prevent crashes and leaks on allocation failure #606

aaronpuchert · 2017-10-16T12:49:46Z

Memory allocation can fail. We need to gracefully handle this case and prevent dereferencing null pointers.

busstoptaktik · 2017-10-17T07:43:44Z

Aaron, this appears to be excellent! If no one else does, I will do a proper review Wednesday or Thursday (I'm away from office at the moment, and commenting code on a phone screen is tedious and error prone).

But from what I can see on the tiny screen, you have squashed some really nasty bugs, some of them even in dark, convoluted, and seldom touched parts of the code.

busstoptaktik

@aaronpuchert, I have left a number of comments in your code for feedback. I still think you have done some excellent work, and have been crawling around in some of the darker corners of PROJ.4, while repairing some major flaws.

The changes suggested are minor, and should not take long to implement - hope you'll have time to look over the suggestions soon, as your work is a real improvement, that I (and, I'm sure, the other PROJ.4 developers) are eager to merge, once you have touched it up.

busstoptaktik · 2017-10-18T10:30:47Z

src/PJ_bonne.c

@@ -111,6 +111,8 @@ PJ *PROJECTION(bonne) {

    if (P->es != 0.0) {
        Q->en = pj_enfn(P->es);
+        if (!Q->en)


I would prefer if you use if (0==Q->en) here. Not to claim that it is a more common idiom than the if (!Q->en) you use, but because the former is used twice in the destructor just a few lines above.

busstoptaktik · 2017-10-18T11:06:32Z

src/pj_datum_set.c

@@ -102,6 +102,8 @@ int pj_datum_set(projCtx ctx, paralist *pl, PJ *projdef)

        projdef->datum_type = PJD_GRIDSHIFT;
        projdef->catalog_name = strdup(catalog);
+        if (!projdef->catalog_name)
+            return 1;   /* TODO: Should we set errno? */


Yes - I believe calling pj_ctx_set_errno (ctx, ENOMEM) would be the proper thing to do here.

busstoptaktik · 2017-10-18T11:08:25Z

src/pj_fileapi.c

+        fclose(fp);
+        return NULL;
+    }
+


... and also call pj_ctx_set_errno (ctx, ENOMEM) here

busstoptaktik · 2017-10-18T11:08:52Z

src/pj_gc_reader.c

+    if (!catalog->catalog_name) {
+        free(catalog);
+        return NULL;
+    }


... and here

busstoptaktik · 2017-10-18T11:10:15Z

src/pj_gc_reader.c

-            tokens[token_count++] = strdup(start);
+            token = strdup(start);
+            if (!token)
+                return token_count; /* TODO: report allocation error */


busstoptaktik · 2017-10-18T11:14:01Z

src/pj_gc_reader.c

@@ -210,7 +223,7 @@ static int pj_gc_readentry(projCtx ctx, PAFile fid, PJ_GridCatalogEntry *entry)
            entry->date = pj_gc_parsedate( ctx, tokens[6] );
    }

-    for( i = 0; i < token_count; i++ )
+    for( i = (token_count < 5) ? 0 : 1; i < token_count; i++ )


A comment would be helpful here. I agree that given the structure of the existing code, your solution takes the right approach, by eliminating a strdup. Making it optimally clear would mean a major restructuring.

But perhaps a slightly more clear way (and an alternative to a comment) would be to duplicate the deallocation inside the if (token_count < 5) branch, returning 1 from inside the if. Then deleting the else line and let the code block do its thing, knowing that everything is OK. This would eliminate the ternary assignment to i inside the for loop, and only explicitly refer to the boundary value that is relevant in each separate case of failure vs. success.

I agree, that is pretty ugly. I think a better solution is to set tokens[0] = NULL, because then free is a no-op.

busstoptaktik · 2017-10-18T11:37:36Z

src/pj_gc_reader.c

@@ -199,7 +212,7 @@ static int pj_gc_readentry(projCtx ctx, PAFile fid, PJ_GridCatalogEntry *entry)
    }


After the token_count = pj_gc_read_csv_line(...), and given your focus on memory handling in this PR, you probably should loop over all tokens to check for 0, since pj_gc_read_csv_line does not check whether its calls to strdup() succeeds.

In pj_gc_read_csv_line I added code to stop if strdup ever fails. However, we should probably make sure that error is propagated properly.

However, no more work should be necessary here, all tokens from 0 to token_count - 1 should be non-null.

Yes - that's a better solution!

aaronpuchert · 2017-10-18T15:47:49Z

The build failure on Windows seems unrelated to my change, no idea why it is popping up now. I opened issue #609 to track this.

busstoptaktik · 2017-10-18T16:26:29Z

The build failure on Windows seems unrelated to my change, no idea why it is popping up now.

When seeing all the strdup cases during review, I was wondering why it hadn't provoked MSVC before, since it is well known to try to scare developers from using POSIX functionality by disinformative, scaremongering warning messages (perhaps a leftover from a Gates/Ballmer-induced multidecadal age of corporate disenlightenment over at MS?).

The function strdup is not part of ANSI C 89, but a POSIX extension. Therefore we can not rely on it being available on all platforms.

aaronpuchert · 2017-10-18T17:12:32Z

It seems there are still problems in pj_gridinfo.c. I'll have a look.

There are quite a few allocations there, which doesn't make it easy to roll back.

aaronpuchert · 2017-10-18T18:03:14Z

On second thought, the problems in pj_gridinfo.c probably deserve a pull request of their own. There are lots of issues, and I'm not sure I'll get it right on the first try.

kbevers · 2017-10-18T18:17:52Z

On second thought, the problems in pj_gridinfo.c probably deserve a pull request of their own. There are lots of issues, and I'm not sure I'll get it right on the first try.

What are the problems? Not questioning wether there are problems, just curious.

aaronpuchert · 2017-10-18T18:30:55Z

What are the problems? Not questioning wether there are problems, just curious.

Search for pj_malloc and pj_strdup, there are a few allocations. To prevent leaks, I have to rollback all previous allocations, release locks, etc. This is how it looks like:

diff --git a/src/pj_gridinfo.c b/src/pj_gridinfo.c
index 332a0ffb..15e68c0e 100644
--- a/src/pj_gridinfo.c
+++ b/src/pj_gridinfo.c
@@ -212,7 +212,9 @@ int pj_gridinfo_load( projCtx ctx, PJ_GRIDINFO *gi )
         ct_tmp.cvs = (FLP *) pj_malloc(gi->ct->lim.lam*gi->ct->lim.phi*sizeof(FLP));
         if( row_buf == NULL || ct_tmp.cvs == NULL )
         {
-            pj_ctx_set_errno( ctx, -38 );
+            pj_dalloc( row_buf );
+            pj_dalloc( ct_tmp.cvs );
+            pj_ctx_set_errno( ctx, ENOMEM );
             pj_release_lock();
             return 0;
         }
@@ -230,6 +232,7 @@ int pj_gridinfo_load( projCtx ctx, PJ_GRIDINFO *gi )
                 pj_dalloc( row_buf );
                 pj_dalloc( ct_tmp.cvs );
                 pj_ctx_set_errno( ctx, -38 );
+                pj_release_lock();
                 return 0;
             }
 
@@ -290,6 +293,8 @@ int pj_gridinfo_load( projCtx ctx, PJ_GRIDINFO *gi )
         ct_tmp.cvs = (FLP *) pj_malloc(gi->ct->lim.lam*gi->ct->lim.phi*sizeof(FLP));
         if( row_buf == NULL || ct_tmp.cvs == NULL )
         {
+            pj_dalloc( row_buf );
+            pj_dalloc( ct_tmp.cvs );
             pj_ctx_set_errno( ctx, -38 );
             pj_release_lock();
             return 0;
@@ -371,6 +376,7 @@ int pj_gridinfo_load( projCtx ctx, PJ_GRIDINFO *gi )
             != (size_t)words )
         {
             pj_dalloc( ct_tmp.cvs );
+            pj_ctx_set_errno( ctx, ENOMEM );
             pj_release_lock();
             return 0;
         }
@@ -519,6 +525,10 @@ static int pj_gridinfo_init_ntv2( projCtx ctx, PAFile fid, PJ_GRIDINFO *gilist )
 /*      Initialize a corresponding "ct" structure.                      */
 /* -------------------------------------------------------------------- */
         ct = (struct CTABLE *) pj_malloc(sizeof(struct CTABLE));
+        if (!ct) {
+            pj_ctx_set_errno(ctx, ENOMEM);
+            return 0;
+        }
         strncpy( ct->id, (const char *) header + 8, 8 );
         ct->id[8] = '\0';
 
@@ -553,6 +563,7 @@ static int pj_gridinfo_init_ntv2( projCtx ctx, PAFile fid, PJ_GRIDINFO *gilist )
                     "GS_COUNT(%d) does not match expected cells (%dx%d=%d)\n",
                     gs_count, ct->lim.lam, ct->lim.phi,
                     ct->lim.lam * ct->lim.phi );
+            pj_dalloc(ct);
             pj_ctx_set_errno( ctx, -38 );
             return 0;
         }
@@ -568,10 +579,23 @@ static int pj_gridinfo_init_ntv2( projCtx ctx, PAFile fid, PJ_GRIDINFO *gilist )
         else
         {
             gi = (PJ_GRIDINFO *) pj_malloc(sizeof(PJ_GRIDINFO));
+            if (!gi) {
+                pj_dalloc(ct);
+                pj_gridinfo_free(ctx, gilist);
+                pj_ctx_set_errno(ctx, ENOMEM);
+                return 0;
+            }
             memset( gi, 0, sizeof(PJ_GRIDINFO) );
 
             gi->gridname = pj_strdup( gilist->gridname );
             gi->filename = pj_strdup( gilist->filename );
+            if (!gi->gridname || gi->filename) {
+                pj_gridinfo_free(ctx, gi);
+                pj_dalloc(ct);
+                pj_gridinfo_free(ctx, gilist);
+                pj_ctx_set_errno(ctx, ENOMEM);
+                return 0;
+            }
             gi->next = NULL;
         }
 
@@ -699,6 +723,10 @@ static int pj_gridinfo_init_ntv1( projCtx ctx, PAFile fid, PJ_GRIDINFO *gi )
 /*      Fill in CTABLE structure.                                       */
 /* -------------------------------------------------------------------- */
     ct = (struct CTABLE *) pj_malloc(sizeof(struct CTABLE));
+    if (!ct) {
+        pj_ctx_set_errno(ctx, ENOMEM);
+        return 0;
+    }
     strcpy( ct->id, "NTv1 Grid Shift File" );
 
     ct->ll.lam = - *((double *) (header+72));
@@ -799,6 +827,10 @@ static int pj_gridinfo_init_gtx( projCtx ctx, PAFile fid, PJ_GRIDINFO *gi )
 /*      Fill in CTABLE structure.                                       */
 /* -------------------------------------------------------------------- */
     ct = (struct CTABLE *) pj_malloc(sizeof(struct CTABLE));
+    if (!ct) {
+        pj_ctx_set_errno(ctx, ENOMEM);
+        return 0;
+    }
     strcpy( ct->id, "GTX Vertical Grid Shift File" );
 
     ct->ll.lam = xorigin;
@@ -864,9 +896,18 @@ PJ_GRIDINFO *pj_gridinfo_init( projCtx ctx, const char *gridname )
 /*      cannot be loaded.                                               */
 /* -------------------------------------------------------------------- */
     gilist = (PJ_GRIDINFO *) pj_malloc(sizeof(PJ_GRIDINFO));
+    if (!gilist) {
+        pj_ctx_set_errno(ctx, ENOMEM);
+        return NULL;
+    }
     memset( gilist, 0, sizeof(PJ_GRIDINFO) );
 
     gilist->gridname = pj_strdup( gridname );
+    if (!gilist->gridname) {
+        pj_dalloc(gilist);
+        pj_ctx_set_errno(ctx, ENOMEM);
+        return NULL;
+    }
     gilist->filename = NULL;
     gilist->format = "missing";
     gilist->grid_offset = 0;
@@ -883,6 +924,12 @@ PJ_GRIDINFO *pj_gridinfo_init( projCtx ctx, const char *gridname )
     }
 
     gilist->filename = pj_strdup(fname);
+    if (!gilist->filename) {
+        pj_dalloc(gilist->gridname);
+        pj_dalloc(gilist);
+        pj_ctx_set_errno(ctx, ENOMEM);
+        return NULL;
+    }
 
 /* -------------------------------------------------------------------- */
 /*      Load a header, to determine the file type.                      */

aaronpuchert · 2017-10-18T18:35:01Z

Long story short: there is a lot to do, and it's easy to miss something. As you can see, there is some cleanup code, but someone forgot to release the lock.

kbevers · 2017-10-18T19:15:21Z

Ah yes, I see. There's a bunch of problems in there. Thanks for putting in the effort to fix them!

In case you're not already aware, all the negative (and PROJ.4-specific) error codes used with pj_ctx_set_errno have recently been defined as constants as well. They are in projects.h and start with PJD_ERR_, e.g. #define PJD_ERR_FAILED_TO_LOAD_GRID -38. Since you're touching these files anyway you might as well update the error codes to there constant-counterparts and improve the readability of the code.

busstoptaktik · 2017-10-19T07:45:58Z

@aaronpuchert, so if you leave the pj_gridinfo.c stuff for a separate PR, I assume this is ready for merging now. Do you agree?

aaronpuchert · 2017-10-19T11:17:13Z

Yes, please go ahead. I don't want to include too much in this pull request. Basically I have it, but we may have to iterate because I missed something.

* Handle allocation failure when loading grid files Continuing #606, we tackle the same issues in the loading and processing of grid files. This should fix potential crashes and memory leaks, and makes sure the global lock is always released. * Use pj_calloc when zero-initialized memory is wanted

Prevent crashes and leaks on allocation failure

c4ed544

Memory allocation can fail. We need to gracefully handle this case and prevent dereferencing null pointers.

busstoptaktik requested changes Oct 18, 2017

View reviewed changes

aaronpuchert added 3 commits October 18, 2017 15:26

Make NULL checks consistent within a file

fe225d2

Properly report allocation errors

7090eb2

Improve cleanup in pj_gc_reader.c

b75b0b2

Implement pj_strdup and use instead of strdup

6fec872

The function strdup is not part of ANSI C 89, but a POSIX extension. Therefore we can not rely on it being available on all platforms.

aaronpuchert mentioned this pull request Oct 18, 2017

Usage of strdup is not C89-compliant #609

Closed

kbevers approved these changes Oct 19, 2017

View reviewed changes

busstoptaktik merged commit 3ef0837 into OSGeo:master Oct 19, 2017

aaronpuchert deleted the handle-allocation-failure branch October 19, 2017 17:18

aaronpuchert mentioned this pull request Oct 20, 2017

Handle allocation failure when loading grid files #616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent crashes and leaks on allocation failure #606

Prevent crashes and leaks on allocation failure #606

aaronpuchert commented Oct 16, 2017

busstoptaktik commented Oct 17, 2017

busstoptaktik left a comment •

edited

busstoptaktik Oct 18, 2017

busstoptaktik Oct 18, 2017

busstoptaktik Oct 18, 2017

busstoptaktik Oct 18, 2017

busstoptaktik Oct 18, 2017

busstoptaktik Oct 18, 2017

aaronpuchert Oct 18, 2017

busstoptaktik Oct 18, 2017

aaronpuchert Oct 18, 2017

busstoptaktik Oct 18, 2017

aaronpuchert commented Oct 18, 2017

busstoptaktik commented Oct 18, 2017

aaronpuchert commented Oct 18, 2017

aaronpuchert commented Oct 18, 2017

kbevers commented Oct 18, 2017

aaronpuchert commented Oct 18, 2017

aaronpuchert commented Oct 18, 2017

kbevers commented Oct 18, 2017

busstoptaktik commented Oct 19, 2017

aaronpuchert commented Oct 19, 2017

		@@ -199,7 +212,7 @@ static int pj_gc_readentry(projCtx ctx, PAFile fid, PJ_GridCatalogEntry *entry)
		}

Prevent crashes and leaks on allocation failure #606

Prevent crashes and leaks on allocation failure #606

Conversation

aaronpuchert commented Oct 16, 2017

busstoptaktik commented Oct 17, 2017

busstoptaktik left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronpuchert commented Oct 18, 2017

busstoptaktik commented Oct 18, 2017

aaronpuchert commented Oct 18, 2017

aaronpuchert commented Oct 18, 2017

kbevers commented Oct 18, 2017

aaronpuchert commented Oct 18, 2017

aaronpuchert commented Oct 18, 2017

kbevers commented Oct 18, 2017

busstoptaktik commented Oct 19, 2017

aaronpuchert commented Oct 19, 2017

busstoptaktik left a comment •

edited