Opened 7 years ago

Closed 6 years ago

#3786 closed defect (fixed)

OSS FUZZ fixes: ptarray fixes

Reported by: robe Owned by: robe
Priority: medium Milestone: PostGIS 2.2.7
Component: postgis Version: master
Keywords: Cc:

Description (last modified by robe)

I've started an attempt to resolve OSS FUZZ complaints.

Pull request here, for your input.

https://git.osgeo.org/gogs/postgis/postgis/pulls/15

strk doesn't think my change should have made a difference but I tested two times. without the change segmentation fault with change no segmentation fault.

My assumption is because the *_cp functions do more checking and internal does pretty much none.

In hind-sight I probably could have achieved the same affect by swapping out calls to getPoint2d_internal with getPoint2d_cp without having to resort to declaring POINT2D variables.

I still need to get the extra CPP flags enabled to confirm I got all the issues in this bug ticket.

https://oss-fuzz.com/v2/testcase-detail/4733660446064640?noredirect=1

Since it complained about others

note based on strk's urging, I switched back to using the earlier functions, but doing a pointer and npoints check.

Change History (12)

comment:1 by strk, 7 years ago

Beside checking, doesn't the *_cp version leaves the passed pointer untouched when the point is empty ? In that case, are you comparing arbitrary POINT values ? (as you never really initialize those points, rigth?)

comment:2 by robe, 7 years ago

Here is the thing I don't get about cc'.

The _cp code returns 0 in case of empty I presume, but what exactly does that mean when return type expected is const POINT2D.

http://postgis.net/docs/doxygen/2.4/da/de7/liblwgeom_8h_ab943e4f240882f2b76341a2948778d2d.html#ab943e4f240882f2b76341a2948778d2d

Do all 0s evaluate as nothing?

if ( ! pa ) return 0;
  488 
  489         if ( (n<0) || (n>=pa->npoints))
  490         {
  491                 lwerror("getPoint2D_const_p: point offset out of range");
  492                 return 0; /*error */
  493         }

As far as lwerror goes, doesn't that always hold execution of the function. I've never seen an lwerror show up in PostgreSQL where my function wasn't killed. So I presume I don't need to worry about catching that.

comment:3 by strk, 7 years ago

The return from that function is an object *pointer* so 0 means NULL pointer. What happens upon calling lwerror depends on the error handler installed. PostGIS installs an handler that interrupts the flow, but other clients (unit tests, fuzzers, foreign clients when allowed) are free to install an handler that continues execution, so it is important not to assume loss of flow on error.

comment:4 by robe, 7 years ago

Milestone: PostGIS 2.4.0PostGIS 2.2.6

comment:5 by robe, 7 years ago

Owner: changed from pramsey to robe

comment:6 by robe, 7 years ago

In 15487:

Credit to OSS-Fuzz
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=2590

References #3786 for PostGIS 2.4/trunk

comment:7 by robe, 7 years ago

Description: modified (diff)

comment:8 by robe, 7 years ago

Summary: OSS FUZZ fixesOSS FUZZ fixes: ptarray heap overflow on is_closed

there are still more it seems so I'll backport these and then check on the others why those dind't automatically close.

comment:9 by robe, 7 years ago

Summary: OSS FUZZ fixes: ptarray heap overflow on is_closedOSS FUZZ fixes: ptarray fixes

comment:10 by robe, 7 years ago

In 15489:

OSS-Fuzz detected issues with ptarray
References #3786 for PostGIS 2.3 (for 2.3.4)

comment:11 by pramsey, 7 years ago

Milestone: PostGIS 2.2.6PostGIS 2.2.7
Note: See TracTickets for help on using tickets.