Opened 5 months ago

Closed 5 months ago

#4646 closed defect (fixed)

Broken pointer arithmetic in gserialized_cmp leads crash/wrong results during ORDER BY

Reported by: dkvash Owned by: pramsey
Priority: critical Milestone: PostGIS 2.5.4
Component: postgis Version: 2.4.x
Keywords: Cc:

Description

Broken pointer arithmetic in gserialized_cmp leads crash/wrong results during ORDER BY

Introduced in https://trac.osgeo.org/postgis/ticket/3935

Author of https://trac.osgeo.org/postgis/changeset/16141 intended to write *(uint32_t*) ((char *)g1 + 8)

but ended up writing an equivalent of *(uint32_t*) ((char *) g1 + 8 * sizeof(void *))

Attachments (3)

Fixed_broken_pointer_arithmetic_leading_to_crash_wrong_results_during_ORDER_BY.patch (639 bytes) - added by dkvash 5 months ago.
Fix
gserialized_cmp_fix_2_4_x.patch (1.0 KB) - added by dkvash 5 months ago.
2.4.x patch
gserialized_cmp_fix_2_5_x.patch (969 bytes) - added by dkvash 5 months ago.
2.5.x patch

Download all attachments as: .zip

Change History (10)

comment:1 Changed 5 months ago by Algunenano

Can we use gserialized_get_type instead and avoid having pointer arithmetics in multiple places?

comment:2 Changed 5 months ago by Algunenano

Version: 2.5.x2.4.x

2.4 is also flawed:

	if (
		sz1 > 16 && // 16 is size of EMPTY, if it's larger - it has coordinates
		sz2 > 16 &&
		*(uint32_t*)(g1->data) == POINTTYPE &&
		*(uint32_t*)(g2->data) == POINTTYPE &&
		!FLAGS_GET_BBOX(g1->flags) &&
		!FLAGS_GET_GEODETIC(g1->flags) &&
		!FLAGS_GET_BBOX(g2->flags) &&
		!FLAGS_GET_GEODETIC(g2->flags)
	)

We shouldn't be dereferencing g1->data directly before checking if there is a bbox (which is checked after). Let's reorder that and use gserialized_get_type which already knows about the arithmetics.

Also, 3.0+ is not affected as that function was refactored.

Changed 5 months ago by dkvash

2.4.x patch

Changed 5 months ago by dkvash

2.5.x patch

comment:3 in reply to:  2 Changed 5 months ago by dkvash

Replying to Algunenano:

2.4 is also flawed:

	if (
		sz1 > 16 && // 16 is size of EMPTY, if it's larger - it has coordinates
		sz2 > 16 &&
		*(uint32_t*)(g1->data) == POINTTYPE &&
		*(uint32_t*)(g2->data) == POINTTYPE &&
		!FLAGS_GET_BBOX(g1->flags) &&
		!FLAGS_GET_GEODETIC(g1->flags) &&
		!FLAGS_GET_BBOX(g2->flags) &&
		!FLAGS_GET_GEODETIC(g2->flags)
	)

We shouldn't be dereferencing g1->data directly before checking if there is a bbox (which is checked after). Let's reorder that and use gserialized_get_type which already knows about the arithmetics.

Also, 3.0+ is not affected as that function was refactored.

Have added patches for both minors: gserialized_cmp_fix_2_4_x.patch​ and gserialized_cmp_fix_2_5_x.patch​

comment:4 Changed 5 months ago by Algunenano

LGTM. dkvash, how would you like to be attributed in the NEWS? Nickname, full name, any other?

comment:5 Changed 5 months ago by Algunenano

One note: the bbox checks need to be kept since the body of the if relies on the serialized point to not have bbox. I'll just re-add it before push it.

comment:6 Changed 5 months ago by Algunenano

dkvash, how would you like to be attributed in the NEWS? Nickname, full name, any other?

I'm pushing it as dkvash so it makes it into the new release due today. Let me know if you would like to change it.

comment:7 Changed 5 months ago by Raúl Marín <git@…>

Resolution: fixed
Status: newclosed

In 667a71a/git:

Fix gserialized_cmp incorrect comparison

Patch by dkvash and some tweaks by me

Closes #4646

Note: See TracTickets for help on using tickets.