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

[dkvash]

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 *))

[dkvash]

Fix

[Algunenano]

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

[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.

[dkvash]

2.4.x patch

[dkvash]

2.5.x patch

[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​

[Algunenano]

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

[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.

[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.

[Raúl Marín <git@…>]

In 667a71a/git:

Fix gserialized_cmp incorrect comparison

Patch by dkvash and some tweaks by me

Closes #4646