NotificationsYou must be signed in to change notification settings
Fork6
Star31

Commit9fe285f

committed

Preserve memory context of VarStringSortSupport buffers.

When enlarging the work buffers of a VarStringSortSupport object,varstrfastcmp_locale was careful to keep them in the ssup_cxtmemory context; but varstr_abbrev_convert just used palloc().The latter creates a hazard that the buffers could be freed outfrom under the VarStringSortSupport object, resulting in stompingon whatever gets allocated in that memory later.In practice, because we only use this code for ICU collations(cf.3df9c37), the problem is confined to use of ICU collations.I believe it may have been unreachable before the introductionof incremental sort, too, as traditional sorting usually justuses one context for the duration of the sort.We could fix this by making the broken stanzas in varstr_abbrev_convertmatch the non-broken ones in varstrfastcmp_locale. However, it seemslike a better idea to dodge the issue altogether by replacing thepfree-and-allocate-anew coding with repalloc, which automaticallypreserves the chunk's memory context. This fix does add a few cyclesbecause repalloc will copy the chunk's content, which the existingcoding assumes is useless. However, we don't expect that these bufferenlargement operations are performance-critical. Besides that, it'sfar from obvious that copying the buffer contents isn't required, sincethese stanzas make no effort to mark the buffers invalid by resettinglast_returned, cache_blob, etc. That seems to be safe upon examination,but it's fragile and could easily get broken in future, which wouldn'tget revealed in testing with short-to-moderate-size strings.Per bug #17584 from James Inform. Whether or not the issue isreachable in the older branches, this code has been broken on itsown terms from its introduction, so patch all the way back.Discussion:https://postgr.es/m/17584-95c79b4a7d771f44@postgresql.org

1 parent4878ea7 commit9fe285fCopy full SHA for 9fe285f

File tree

1 file changed

-10

lines changed

src/backend/utils/adt
- varlena.c

1 file changed

-10

lines changed

`‎src/backend/utils/adt/varlena.c`

Lines changed: 6 additions & 10 deletions

Original file line number	Diff line number	Diff line change
`@@ -78,8 +78,8 @@ typedef struct`
`78`	`78`	`charbuf1;/ 1st string, or abbreviation original string`
`79`	`79`	`* buf */`
`80`	`80`	`charbuf2;/ 2nd string, or abbreviation strxfrm() buf */`
`81`		`-intbuflen1;`
`82`		`-intbuflen2;`
	`81`	`+intbuflen1;/* Allocated length of buf1 */`
	`82`	`+intbuflen2;/* Allocated length of buf2 */`
`83`	`83`	`intlast_len1;/* Length of last buf1 string/strxfrm() input */`
`84`	`84`	`intlast_len2;/* Length of last buf2 string/strxfrm() blob */`
`85`	`85`	`intlast_returned;/* Last comparison result (cache) */`
`@@ -2309,15 +2309,13 @@ varstrfastcmp_locale(char a1p, int len1, char a2p, int len2, SortSupport ssup)`
`2309`	`2309`
`2310`	`2310`	`if (len1 >=sss->buflen1)`
`2311`	`2311`	`{`
`2312`		`-pfree(sss->buf1);`
`2313`	`2312`	`sss->buflen1=Max(len1+1,Min(sss->buflen1*2,MaxAllocSize));`
`2314`		`-sss->buf1=MemoryContextAlloc(ssup->ssup_cxt,sss->buflen1);`
	`2313`	`+sss->buf1=repalloc(sss->buf1,sss->buflen1);`
`2315`	`2314`	`}`
`2316`	`2315`	`if (len2 >=sss->buflen2)`
`2317`	`2316`	`{`
`2318`		`-pfree(sss->buf2);`
`2319`	`2317`	`sss->buflen2=Max(len2+1,Min(sss->buflen2*2,MaxAllocSize));`
`2320`		`-sss->buf2=MemoryContextAlloc(ssup->ssup_cxt,sss->buflen2);`
	`2318`	`+sss->buf2=repalloc(sss->buf2,sss->buflen2);`
`2321`	`2319`	`}`
`2322`	`2320`
`2323`	`2321`	`/*`
`@@ -2518,9 +2516,8 @@ varstr_abbrev_convert(Datum original, SortSupport ssup)`
`2518`	`2516`	`/* By convention, we use buffer 1 to store and NUL-terminate */`
`2519`	`2517`	`if (len >=sss->buflen1)`
`2520`	`2518`	`{`
`2521`		`-pfree(sss->buf1);`
`2522`	`2519`	`sss->buflen1=Max(len+1,Min(sss->buflen1*2,MaxAllocSize));`
`2523`		`-sss->buf1=palloc(sss->buflen1);`
	`2520`	`+sss->buf1=repalloc(sss->buf1,sss->buflen1);`
`2524`	`2521`	`}`
`2525`	`2522`
`2526`	`2523`	`/* Might be able to reuse strxfrm() blob from last call */`
`@@ -2607,10 +2604,9 @@ varstr_abbrev_convert(Datum original, SortSupport ssup)`
`2607`	`2604`	`/*`
`2608`	`2605`	`* Grow buffer and retry.`
`2609`	`2606`	`*/`
`2610`		`-pfree(sss->buf2);`
`2611`	`2607`	`sss->buflen2=Max(bsize+1,`
`2612`	`2608`	`Min(sss->buflen2*2,MaxAllocSize));`
`2613`		`-sss->buf2=palloc(sss->buflen2);`
	`2609`	`+sss->buf2=repalloc(sss->buf2,sss->buflen2);`
`2614`	`2610`	`}`
`2615`	`2611`
`2616`	`2612`	`/*`

0 commit comments

Comments

(0)

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Commit9fe285f

File tree

1 file changed

1 file changed

`‎src/backend/utils/adt/varlena.c`

0 commit comments