Skip to content

Commit 9e7d87c

Browse files
tglsfdctglsfdc
committed
Fix integer-overflow corner cases in substring() functions.
If the substring start index and length overflow when added together, substring() misbehaved, either throwing a bogus "negative substring length" error on a case that should succeed, or failing to complain that a negative length is negative (and instead returning the whole string, in most cases). Unsurprisingly, the text, bytea, and bit variants of the function all had this issue. Rearrange the logic to ensure that negative lengths are always rejected, and add an overflow check to handle the other case. Also install similar guards into detoast_attr_slice() (nee heap_tuple_untoast_attr_slice()), since it's far from clear that no other code paths leading to that function could pass it values that would overflow. Patch by myself and Pavel Stehule, per bug #16804 from Rafi Shamim. Back-patch to v11. While these bugs are old, the common/int.h infrastructure for overflow-detecting arithmetic didn't exist before commit 4d6ad31, and it doesn't seem like these misbehaviors are bad enough to justify developing a standalone fix for the older branches. Discussion: https://postgr.es/m/16804-f4eeeb6c11ba71d4@postgresql.org
1 parent c09f688 commit 9e7d87c

File tree

7 files changed

+195
-63
lines changed

7 files changed

+195
-63
lines changed

‎src/backend/access/common/detoast.c

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "access/table.h"
1818
#include "access/tableam.h"
1919
#include "access/toast_internals.h"
20+
#include "common/int.h"
2021
#include "common/pg_lzcompress.h"
2122
#include "utils/expandeddatum.h"
2223
#include "utils/rel.h"
@@ -196,7 +197,8 @@ detoast_attr(struct varlena *attr)
196197
* Public entry point to get back part of a toasted value
197198
* from compression or external storage.
198199
*
199-
* Note: When slicelength is negative, return suffix of the value.
200+
* sliceoffset is where to start (zero or more)
201+
* If slicelength < 0, return everything beyond sliceoffset
200202
* ----------
201203
*/
202204
struct varlena *
@@ -206,8 +208,21 @@ detoast_attr_slice(struct varlena *attr,
206208
struct varlena *preslice;
207209
struct varlena *result;
208210
char *attrdata;
211+
int32 slicelimit;
209212
int32 attrsize;
210213

214+
if (sliceoffset < 0)
215+
elog(ERROR, "invalid sliceoffset: %d", sliceoffset);
216+
217+
/*
218+
* Compute slicelimit = offset + length, or -1 if we must fetch all of the
219+
* value. In case of integer overflow, we must fetch all.
220+
*/
221+
if (slicelength < 0)
222+
slicelimit = -1;
223+
else if (pg_add_s32_overflow(sliceoffset, slicelength, &slicelimit))
224+
slicelength = slicelimit = -1;
225+
211226
if (VARATT_IS_EXTERNAL_ONDISK(attr))
212227
{
213228
struct varatt_external toast_pointer;
@@ -223,15 +238,15 @@ detoast_attr_slice(struct varlena *attr,
223238
* at least the requested part (when a prefix is requested).
224239
* Otherwise, just fetch all slices.
225240
*/
226-
if (slicelength > 0 && sliceoffset >= 0)
241+
if (slicelimit >= 0)
227242
{
228243
int32 max_size;
229244

230245
/*
231246
* Determine maximum amount of compressed data needed for a prefix
232247
* of a given length (after decompression).
233248
*/
234-
max_size = pglz_maximum_compressed_size(sliceoffset + slicelength,
249+
max_size = pglz_maximum_compressed_size(slicelimit,
235250
toast_pointer.va_extsize);
236251

237252
/*
@@ -270,8 +285,8 @@ detoast_attr_slice(struct varlena *attr,
270285
struct varlena *tmp = preslice;
271286

272287
/* Decompress enough to encompass the slice and the offset */
273-
if (slicelength > 0 && sliceoffset >= 0)
274-
preslice = toast_decompress_datum_slice(tmp, slicelength + sliceoffset);
288+
if (slicelimit >= 0)
289+
preslice = toast_decompress_datum_slice(tmp, slicelimit);
275290
else
276291
preslice = toast_decompress_datum(tmp);
277292

@@ -297,8 +312,7 @@ detoast_attr_slice(struct varlena *attr,
297312
sliceoffset = 0;
298313
slicelength = 0;
299314
}
300-
301-
if (((sliceoffset + slicelength) > attrsize) || slicelength < 0)
315+
else if (slicelength < 0 || slicelimit > attrsize)
302316
slicelength = attrsize - sliceoffset;
303317

304318
result = (struct varlena *) palloc(slicelength + VARHDRSZ);
@@ -410,6 +424,11 @@ toast_fetch_datum_slice(struct varlena *attr, int32 sliceoffset,
410424
if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer) && slicelength > 0)
411425
slicelength = slicelength + sizeof(int32);
412426

427+
/*
428+
* Adjust length request if needed. (Note: our sole caller,
429+
* detoast_attr_slice, protects us against sliceoffset + slicelength
430+
* overflowing.)
431+
*/
413432
if (((sliceoffset + slicelength) > attrsize) || slicelength < 0)
414433
slicelength = attrsize - sliceoffset;
415434

‎src/backend/utils/adt/varbit.c

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,7 +1059,7 @@ bitsubstring(VarBit *arg, int32 s, int32 l, bool length_not_specified)
10591059
len,
10601060
ishift,
10611061
i;
1062-
int e,
1062+
int32 e,
10631063
s1,
10641064
e1;
10651065
bits8 *r,
@@ -1072,18 +1072,24 @@ bitsubstring(VarBit *arg, int32 s, int32 l, bool length_not_specified)
10721072
{
10731073
e1 = bitlen + 1;
10741074
}
1075-
else
1075+
else if (l < 0)
1076+
{
1077+
/* SQL99 says to throw an error for E < S, i.e., negative length */
1078+
ereport(ERROR,
1079+
(errcode(ERRCODE_SUBSTRING_ERROR),
1080+
errmsg("negative substring length not allowed")));
1081+
e1 = -1; /* silence stupider compilers */
1082+
}
1083+
else if (pg_add_s32_overflow(s, l, &e))
10761084
{
1077-
e = s + l;
1078-
10791085
/*
1080-
* A negative value for L is the only way for the end position to be
1081-
* before the start. SQL99 says to throw an error.
1086+
* L could be large enough for S + L to overflow, in which case the
1087+
* substring must run to end of string.
10821088
*/
1083-
if (e < s)
1084-
ereport(ERROR,
1085-
(errcode(ERRCODE_SUBSTRING_ERROR),
1086-
errmsg("negative substring length not allowed")));
1089+
e1 = bitlen + 1;
1090+
}
1091+
else
1092+
{
10871093
e1 = Min(e, bitlen + 1);
10881094
}
10891095
if (s1 > bitlen || e1 <= s1)

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

Lines changed: 64 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -851,29 +851,38 @@ text_substring(Datum str, int32 start, int32 length, bool length_not_specified)
851851
int32 S = start; /* start position */
852852
int32 S1; /* adjusted start position */
853853
int32 L1; /* adjusted substring length */
854+
int32 E; /* end position */
855+
856+
/*
857+
* SQL99 says S can be zero or negative, but we still must fetch from the
858+
* start of the string.
859+
*/
860+
S1 = Max(S, 1);
854861

855862
/* life is easy if the encoding max length is 1 */
856863
if (eml == 1)
857864
{
858-
S1 = Max(S, 1);
859-
860865
if (length_not_specified) /* special case - get length to end of
861866
* string */
862867
L1 = -1;
863-
else
868+
else if (length < 0)
869+
{
870+
/* SQL99 says to throw an error for E < S, i.e., negative length */
871+
ereport(ERROR,
872+
(errcode(ERRCODE_SUBSTRING_ERROR),
873+
errmsg("negative substring length not allowed")));
874+
L1 = -1; /* silence stupider compilers */
875+
}
876+
else if (pg_add_s32_overflow(S, length, &E))
864877
{
865-
/* end position */
866-
int E = S + length;
867-
868878
/*
869-
* A negative value for L is the only way for the end position to
870-
* be before the start. SQL99 says to throw an error.
879+
* L could be large enough for S + L to overflow, in which case
880+
* the substring must run to end of string.
871881
*/
872-
if (E < S)
873-
ereport(ERROR,
874-
(errcode(ERRCODE_SUBSTRING_ERROR),
875-
errmsg("negative substring length not allowed")));
876-
882+
L1 = -1;
883+
}
884+
else
885+
{
877886
/*
878887
* A zero or negative value for the end position can happen if the
879888
* start was negative or one. SQL99 says to return a zero-length
@@ -887,8 +896,8 @@ text_substring(Datum str, int32 start, int32 length, bool length_not_specified)
887896

888897
/*
889898
* If the start position is past the end of the string, SQL99 says to
890-
* return a zero-length string -- PG_GETARG_TEXT_P_SLICE() will do
891-
* that for us. Convert to zero-based starting position
899+
* return a zero-length string -- DatumGetTextPSlice() will do that
900+
* for us. We need only convert S1 to zero-based starting position.
892901
*/
893902
return DatumGetTextPSlice(str, S1 - 1, L1);
894903
}
@@ -909,12 +918,6 @@ text_substring(Datum str, int32 start, int32 length, bool length_not_specified)
909918
char *s;
910919
text *ret;
911920

912-
/*
913-
* if S is past the end of the string, the tuple toaster will return a
914-
* zero-length string to us
915-
*/
916-
S1 = Max(S, 1);
917-
918921
/*
919922
* We need to start at position zero because there is no way to know
920923
* in advance which byte offset corresponds to the supplied start
@@ -925,19 +928,24 @@ text_substring(Datum str, int32 start, int32 length, bool length_not_specified)
925928
if (length_not_specified) /* special case - get length to end of
926929
* string */
927930
slice_size = L1 = -1;
928-
else
931+
else if (length < 0)
932+
{
933+
/* SQL99 says to throw an error for E < S, i.e., negative length */
934+
ereport(ERROR,
935+
(errcode(ERRCODE_SUBSTRING_ERROR),
936+
errmsg("negative substring length not allowed")));
937+
slice_size = L1 = -1; /* silence stupider compilers */
938+
}
939+
else if (pg_add_s32_overflow(S, length, &E))
929940
{
930-
int E = S + length;
931-
932941
/*
933-
* A negative value for L is the only way for the end position to
934-
* be before the start. SQL99 says to throw an error.
942+
* L could be large enough for S + L to overflow, in which case
943+
* the substring must run to end of string.
935944
*/
936-
if (E < S)
937-
ereport(ERROR,
938-
(errcode(ERRCODE_SUBSTRING_ERROR),
939-
errmsg("negative substring length not allowed")));
940-
945+
slice_size = L1 = -1;
946+
}
947+
else
948+
{
941949
/*
942950
* A zero or negative value for the end position can happen if the
943951
* start was negative or one. SQL99 says to return a zero-length
@@ -955,8 +963,10 @@ text_substring(Datum str, int32 start, int32 length, bool length_not_specified)
955963
/*
956964
* Total slice size in bytes can't be any longer than the start
957965
* position plus substring length times the encoding max length.
966+
* If that overflows, we can just use -1.
958967
*/
959-
slice_size = (S1 + L1) * eml;
968+
if (pg_mul_s32_overflow(E, eml, &slice_size))
969+
slice_size = -1;
960970
}
961971

962972
/*
@@ -3278,9 +3288,13 @@ bytea_substring(Datum str,
32783288
int L,
32793289
bool length_not_specified)
32803290
{
3281-
int S1; /* adjusted start position */
3282-
int L1; /* adjusted substring length */
3291+
int32 S1; /* adjusted start position */
3292+
int32 L1; /* adjusted substring length */
3293+
int32 E; /* end position */
32833294

3295+
/*
3296+
* The logic here should generally match text_substring().
3297+
*/
32843298
S1 = Max(S, 1);
32853299

32863300
if (length_not_specified)
@@ -3291,20 +3305,24 @@ bytea_substring(Datum str,
32913305
*/
32923306
L1 = -1;
32933307
}
3294-
else
3308+
else if (L < 0)
3309+
{
3310+
/* SQL99 says to throw an error for E < S, i.e., negative length */
3311+
ereport(ERROR,
3312+
(errcode(ERRCODE_SUBSTRING_ERROR),
3313+
errmsg("negative substring length not allowed")));
3314+
L1 = -1; /* silence stupider compilers */
3315+
}
3316+
else if (pg_add_s32_overflow(S, L, &E))
32953317
{
3296-
/* end position */
3297-
int E = S + L;
3298-
32993318
/*
3300-
* A negative value for L is the only way for the end position to be
3301-
* before the start. SQL99 says to throw an error.
3319+
* L could be large enough for S + L to overflow, in which case the
3320+
* substring must run to end of string.
33023321
*/
3303-
if (E < S)
3304-
ereport(ERROR,
3305-
(errcode(ERRCODE_SUBSTRING_ERROR),
3306-
errmsg("negative substring length not allowed")));
3307-
3322+
L1 = -1;
3323+
}
3324+
else
3325+
{
33083326
/*
33093327
* A zero or negative value for the end position can happen if the
33103328
* start was negative or one. SQL99 says to return a zero-length
@@ -3319,7 +3337,7 @@ bytea_substring(Datum str,
33193337
/*
33203338
* If the start position is past the end of the string, SQL99 says to
33213339
* return a zero-length string -- DatumGetByteaPSlice() will do that for
3322-
* us. Convert to zero-based starting position
3340+
* us. We need only convert S1 to zero-based starting position.
33233341
*/
33243342
return DatumGetByteaPSlice(str, S1 - 1, L1);
33253343
}

‎src/test/regress/expected/bit.out

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,35 @@ SELECT v,
106106
01010101010 | 1010 | 01010 | 101010
107107
(4 rows)
108108

109+
-- test overflow cases
110+
SELECT SUBSTRING('01010101'::bit(8) FROM 2 FOR 2147483646) AS "1010101";
111+
1010101
112+
---------
113+
1010101
114+
(1 row)
115+
116+
SELECT SUBSTRING('01010101'::bit(8) FROM -10 FOR 2147483646) AS "01010101";
117+
01010101
118+
----------
119+
01010101
120+
(1 row)
121+
122+
SELECT SUBSTRING('01010101'::bit(8) FROM -10 FOR -2147483646) AS "error";
123+
ERROR: negative substring length not allowed
124+
SELECT SUBSTRING('01010101'::varbit FROM 2 FOR 2147483646) AS "1010101";
125+
1010101
126+
---------
127+
1010101
128+
(1 row)
129+
130+
SELECT SUBSTRING('01010101'::varbit FROM -10 FOR 2147483646) AS "01010101";
131+
01010101
132+
----------
133+
01010101
134+
(1 row)
135+
136+
SELECT SUBSTRING('01010101'::varbit FROM -10 FOR -2147483646) AS "error";
137+
ERROR: negative substring length not allowed
109138
--- Bit operations
110139
DROP TABLE varbit_table;
111140
CREATE TABLE varbit_table (a BIT VARYING(16), b BIT VARYING(16));

0 commit comments

Comments
 (0)