Opened 5 weeks ago

Closed 4 weeks 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 weeks ago.
Fix
gserialized_cmp_fix_2_4_x.patch (1.0 KB) - added by dkvash 4 weeks ago.
2.4.x patch
gserialized_cmp_fix_2_5_x.patch (969 bytes) - added by dkvash 4 weeks ago.
2.5.x patch

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 weeks ago by Algunenano

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

comment:2 Changed 4 weeks 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 4 weeks ago by dkvash

2.4.x patch

Changed 4 weeks ago by dkvash

2.5.x patch

comment:3 in reply to:  2 Changed 4 weeks 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 4 weeks ago by Algunenano

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

comment:5 Changed 4 weeks 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 4 weeks 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 4 weeks 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.