Opened 5 years ago

Closed 5 years ago

#2772 closed defect (fixed)

Memory corruption in ST_BandPath

Reported by: strk Owned by: Bborie Park
Priority: high Milestone: PostGIS 2.0.7
Component: raster Version: 2.0.x
Keywords: history Cc:

Description

The ST_BandPath code contains memory error resulting in random memory being dumped for offline bands, see #2771.

Indeed the code seems to be destroying the memory containing the string before copying it to final value for return: https://github.com/postgis/postgis/blob/2.1.3/raster/rt_pg/rt_pg.c#L2619-L2628

Change History (8)

comment:1 Changed 5 years ago by Bborie Park

Status: newassigned

comment:2 Changed 5 years ago by strk

Milestone: PostGIS 2.1.4PostGIS 2.0.7
Version: 2.1.x2.0.x

2.0 is also affected: https://github.com/postgis/postgis/blob/2.0.6/raster/rt_pg/rt_pg.c#L2387-L2395

I'm taking a look at fixing it in 2.0

comment:3 Changed 5 years ago by strk

Forget it, I'm getting coredumps in testapi ! ... filing another ticket about that (could be GDAL version related)

comment:4 Changed 5 years ago by Bborie Park

Still worth fixing.

What stack are you using for 2.0's testapi?

comment:5 Changed 5 years ago by strk

My patch for 2.0 is this one:

diff --git a/raster/rt_pg/rt_pg.c b/raster/rt_pg/rt_pg.c
index 2ced796..e020e0b 100644
--- a/raster/rt_pg/rt_pg.c
+++ b/raster/rt_pg/rt_pg.c
@@ -2385,10 +2385,10 @@ Datum RASTER_getBandPath(PG_FUNCTION_ARGS)
     }
 
     bandpath = rt_band_get_ext_path(band);
-               rt_band_destroy(band);
-    rt_raster_destroy(raster);
     if ( ! bandpath )
     {
+        rt_band_destroy(band);
+        rt_raster_destroy(raster);
         PG_RETURN_NULL();
     }
 
@@ -2398,6 +2398,9 @@ Datum RASTER_getBandPath(PG_FUNCTION_ARGS)
 
     strcpy((char *) VARDATA(result), bandpath);
 
+    rt_band_destroy(band);
+    rt_raster_destroy(raster);
+
     PG_RETURN_TEXT_P(result);
 }

For the memory corruption I filed #2773

comment:6 Changed 5 years ago by Bborie Park

I'd have suggested moving the strcpy up instead...

comment:7 Changed 5 years ago by strk

your call, I can't test 2.0 and I'm getting ready for the weekend anyway, so it's all yours in all branches. Have fun ! :)

comment:8 Changed 5 years ago by Bborie Park

Keywords: history added
Resolution: fixed
Status: assignedclosed

I used your patch. Fixed in trunk as of r12631. Fixed in 2.1 as of r12632. Fixed in 2.0 as of r12633

Note: See TracTickets for help on using tickets.