Opened 5 months ago

Closed 2 months ago

#5088 closed defect (fixed)

Memory corruption in mvt_agg_transfn

Reported by: multun Owned by: pramsey
Priority: high Milestone: PostGIS 3.3.0
Component: postgis Version: 3.2.x
Keywords: Cc: git@…

Description

Hello!

We've had frequent crashes in mvt_agg_transfn, with this message: could not find block containing chunk. I thought it could be a buffer overflow, compiled postgis and postgres with debug and valgrind support enabled, and sure enough:

==26== Invalid write of size 4
==26==    at 0x10B244C6: add_value_as_string_with_size (mvt.c:481)
==26==    by 0x10B295C6: add_value_as_string (mvt.c:490)
==26==    by 0x10B295C6: parse_jsonb (mvt.c:551)
==26==    by 0x10B295C6: parse_values (mvt.c:686)
==26==    by 0x10B295C6: mvt_agg_transfn (mvt.c:1052)
==26==    by 0x10B2BA47: pgis_asmvt_transfn (lwgeom_out_mvt.c:172)
==26==    by 0x4840EA: ExecAggPlainTransByVal (execExprInterp.c:4298)
==26==    by 0x47E974: ExecInterpExpr (execExprInterp.c:1696)
==26==    by 0x47ED9E: ExecInterpExprStillValid (execExprInterp.c:1807)
==26==    by 0x4A3F2F: ExecEvalExprSwitchContext (executor.h:322)
==26==    by 0x4A49E3: advance_aggregates (nodeAgg.c:850)
==26==    by 0x4A79AA: agg_retrieve_direct (nodeAgg.c:2462)
==26==    by 0x4A7417: ExecAgg (nodeAgg.c:2187)
==26==    by 0x4948C1: ExecProcNodeFirst (execProcnode.c:450)
==26==    by 0x4884F2: ExecProcNode (executor.h:248)
==26==  Address 0x1086e418 is 5,608 bytes inside a block of size 8,192 alloc'd
==26==    at 0x48A26D9: malloc (vg_replace_malloc.c:380)
==26==    by 0x8A67B2: AllocSetContextCreateInternal (aset.c:468)
==26==    by 0x10B2BB55: pgis_asmvt_transfn (lwgeom_out_mvt.c:158)
==26==    by 0x4840EA: ExecAggPlainTransByVal (execExprInterp.c:4298)
==26==    by 0x47E974: ExecInterpExpr (execExprInterp.c:1696)
==26==    by 0x47ED9E: ExecInterpExprStillValid (execExprInterp.c:1807)
==26==    by 0x4A3F2F: ExecEvalExprSwitchContext (executor.h:322)
==26==    by 0x4A49E3: advance_aggregates (nodeAgg.c:850)
==26==    by 0x4A79AA: agg_retrieve_direct (nodeAgg.c:2462)
==26==    by 0x4A7417: ExecAgg (nodeAgg.c:2187)
==26==    by 0x4948C1: ExecProcNodeFirst (execProcnode.c:450)
==26==    by 0x4884F2: ExecProcNode (executor.h:248)
==26==

In this specific instance, when the overflow occurs on line 481 of mvt.c:

tags[ctx->row_columns * 2] = k

tags has room for 10 items (I deduced this from allocation metadata headers), yet ctx->row_columns is 5.

So the tags array is too small. It is initially allocated with the number of already known hash keys as an initial size: uint32_t *tags = palloc(ctx->keys_hash_i * 2 * sizeof(*tags)); It may very well be a good guess, yet nothing ensures the array is big enough with properties are added to the feature.

The array is only resized when a new key is met in parse_jsonb, which does not seem to be enough.

Change History (2)

comment:2 by Regina Obe <lr@…>, 2 months ago

Resolution: fixed
Status: newclosed

In 8176e19e/git:

mvt: introduce feature_builder and fix a buffer overflow

The previous code tried to manually handle resizing the tags array
whenever used, but failed to keep it properly sized.

This commit adds an abstraction over unfinished features, which
enables centralized array size bookkeeping.

Closes https://git.osgeo.org/gitea/postgis/postgis/pulls/92 for PostGIS
3.3.0
Closes #5088 for PostGIS 3.3.0

Note: See TracTickets for help on using tickets.