pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: movead.li@highgo.ca <movead.li@highgo.ca>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: ashutosh.bapat <ashutosh.bapat@2ndquadrant.com>
Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: A bug when use get_bit() function for a long bytea string
Date: Thu, 2 Apr 2020 14:26:08 +0800
Message-ID: <2020040214260375403022@highgo.ca> (raw )
References: <20200312115135445367128@highgo.ca >
<CAExHW5vUSi4-oPvgMdFWvzuyaTmMtbY=JgdZ+u3T7ZdzQmubig@mail.gmail.com >
<20200313111842791351178@highgo.ca >
<CAG-ACPXeQEuvdVBzz+NWgFYoBNwcJfv1W=pJExp4LM_3fgar6w@mail.gmail.com >
<2020031810480643107833@highgo.ca >
<CAG-ACPUbQwpCHhn_KkLAGEc08A91yZhARazj2BGjXvK+eFCVog@mail.gmail.com >
<2020032817093521193529@highgo.ca >
<CAG-ACPWtjb2y5RaoWP8f76OmvSvNP3xwT82spa9VDLkNJEOdDg@mail.gmail.com >
<2020040111593809632067@highgo.ca >
<20077.1585757544@sss.pgh.pa.us >
>I don't think this has really solved the overflow hazards. For example,
>in binary_encode we've got
>resultlen = enc->encode_len(VARDATA_ANY(data), datalen);
>result = palloc(VARHDRSZ + resultlen);
>and all you've done about that is changed resultlen from int to int64.
>On a 64-bit machine, sure, palloc will be able to detect if the
>result exceeds what can be allocated --- but on a 32-bit machine
>it'd be possible for the size_t argument to overflow altogether.
>(Or if you want to argue that it can't overflow because no encoder
>expands the data more than 4X, then we don't need to be making this
>change at all.)
>I don't think there's really any way to do that safely without an
>explicit check before we call palloc.
I am sorry I do not very understand these words, and especially
what's the mean by 'size_t'.
Here I change resultlen from int to int64, is because we can get a right
error report value other than '-1' or another strange number.
>Why did you change the outputs from unsigned to signed? Why didn't
>you change the dlen inputs? I grant that we know that the input
>can't exceed 1GB in Postgres' usage, but these signatures are just
>randomly inconsistent, and you didn't even add a comment explaining
>why. Personally I think I'd make them like
>uint64 (*encode_len) (const char *data, size_t dlen);
>which makes it explicit that the dlen argument describes the length
>of a chunk of allocated memory, while the result might exceed that.
I think it makes sence and followed.
>Lastly, there is a component of this that can be back-patched and
>a component that can't --- we do not change system catalog contents
>in released branches. Cramming both parts into the same patch
>is forcing the committer to pull them apart, which is kind of
>unfriendly.
Sorry about that, attached is the changed patch for PG13, and the one
for older branches will send sooner.
Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
Attachments:
[application/octet-stream] long_bytea_string_bug_fix_ver6.patch (10.9K, ../2020040214260375403022@highgo.ca/3-long_bytea_string_bug_fix_ver6.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/encode.c b/src/backend/utils/adt/encode.c
index b8d9ec7e00..5be5739501 100644
--- a/src/backend/utils/adt/encode.c
+++ b/src/backend/utils/adt/encode.c
@@ -20,10 +20,10 @@
struct pg_encoding
{
- unsigned (*encode_len) (const char *data, unsigned dlen);
- unsigned (*decode_len) (const char *data, unsigned dlen);
- unsigned (*encode) (const char *data, unsigned dlen, char *res);
- unsigned (*decode) (const char *data, unsigned dlen, char *res);
+ uint64 (*encode_len) (const char *data, size_t dlen);
+ uint64 (*decode_len) (const char *data, size_t dlen);
+ uint64 (*encode) (const char *data, size_t dlen, char *res);
+ uint64 (*decode) (const char *data, size_t dlen, char *res);
};
static const struct pg_encoding *pg_find_encoding(const char *name);
@@ -39,9 +39,9 @@ binary_encode(PG_FUNCTION_ARGS)
Datum name = PG_GETARG_DATUM(1);
text *result;
char *namebuf;
- int datalen,
- resultlen,
- res;
+ size_t datalen;
+ uint64 res;
+ uint64 resultlen;
const struct pg_encoding *enc;
datalen = VARSIZE_ANY_EXHDR(data);
@@ -75,9 +75,9 @@ binary_decode(PG_FUNCTION_ARGS)
Datum name = PG_GETARG_DATUM(1);
bytea *result;
char *namebuf;
- int datalen,
- resultlen,
- res;
+ size_t datalen;
+ uint64 resultlen;
+ uint64 res;
const struct pg_encoding *enc;
datalen = VARSIZE_ANY_EXHDR(data);
@@ -122,8 +122,8 @@ static const int8 hexlookup[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
};
-unsigned
-hex_encode(const char *src, unsigned len, char *dst)
+uint64
+hex_encode(const char *src, size_t len, char *dst)
{
const char *end = src + len;
@@ -133,7 +133,7 @@ hex_encode(const char *src, unsigned len, char *dst)
*dst++ = hextbl[*src & 0xF];
src++;
}
- return len * 2;
+ return (int64)len * 2;
}
static inline char
@@ -152,8 +152,8 @@ get_hex(char c)
return (char) res;
}
-unsigned
-hex_decode(const char *src, unsigned len, char *dst)
+uint64
+hex_decode(const char *src, size_t len, char *dst)
{
const char *s,
*srcend;
@@ -184,16 +184,16 @@ hex_decode(const char *src, unsigned len, char *dst)
return p - dst;
}
-static unsigned
-hex_enc_len(const char *src, unsigned srclen)
+static uint64
+hex_enc_len(const char *src, size_t srclen)
{
- return srclen << 1;
+ return (uint64)srclen << 1;
}
-static unsigned
-hex_dec_len(const char *src, unsigned srclen)
+static uint64
+hex_dec_len(const char *src, size_t srclen)
{
- return srclen >> 1;
+ return (uint64)srclen >> 1;
}
/*
@@ -214,8 +214,8 @@ static const int8 b64lookup[128] = {
41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, -1, -1, -1, -1, -1,
};
-static unsigned
-pg_base64_encode(const char *src, unsigned len, char *dst)
+static uint64
+pg_base64_encode(const char *src, size_t len, char *dst)
{
char *p,
*lend = dst + 76;
@@ -261,8 +261,8 @@ pg_base64_encode(const char *src, unsigned len, char *dst)
return p - dst;
}
-static unsigned
-pg_base64_decode(const char *src, unsigned len, char *dst)
+static uint64
+pg_base64_decode(const char *src, size_t len, char *dst)
{
const char *srcend = src + len,
*s = src;
@@ -331,17 +331,17 @@ pg_base64_decode(const char *src, unsigned len, char *dst)
}
-static unsigned
-pg_base64_enc_len(const char *src, unsigned srclen)
+static uint64
+pg_base64_enc_len(const char *src, size_t srclen)
{
/* 3 bytes will be converted to 4, linefeed after 76 chars */
- return (srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4);
+ return ((uint64)srclen + 2) * 4 / 3 + (uint64)srclen / (76 * 3 / 4);
}
-static unsigned
-pg_base64_dec_len(const char *src, unsigned srclen)
+static uint64
+pg_base64_dec_len(const char *src, size_t srclen)
{
- return (srclen * 3) >> 2;
+ return ((uint64)srclen * 3) >> 2;
}
/*
@@ -361,12 +361,12 @@ pg_base64_dec_len(const char *src, unsigned srclen)
#define VAL(CH) ((CH) - '0')
#define DIG(VAL) ((VAL) + '0')
-static unsigned
-esc_encode(const char *src, unsigned srclen, char *dst)
+static uint64
+esc_encode(const char *src, size_t srclen, char *dst)
{
const char *end = src + srclen;
char *rp = dst;
- int len = 0;
+ uint64 len = 0;
while (src < end)
{
@@ -400,12 +400,12 @@ esc_encode(const char *src, unsigned srclen, char *dst)
return len;
}
-static unsigned
-esc_decode(const char *src, unsigned srclen, char *dst)
+static uint64
+esc_decode(const char *src, size_t srclen, char *dst)
{
const char *end = src + srclen;
char *rp = dst;
- int len = 0;
+ uint64 len = 0;
while (src < end)
{
@@ -448,11 +448,11 @@ esc_decode(const char *src, unsigned srclen, char *dst)
return len;
}
-static unsigned
-esc_enc_len(const char *src, unsigned srclen)
+static uint64
+esc_enc_len(const char *src, size_t srclen)
{
const char *end = src + srclen;
- int len = 0;
+ uint64 len = 0;
while (src < end)
{
@@ -469,11 +469,11 @@ esc_enc_len(const char *src, unsigned srclen)
return len;
}
-static unsigned
-esc_dec_len(const char *src, unsigned srclen)
+static uint64
+esc_dec_len(const char *src, size_t srclen)
{
const char *end = src + srclen;
- int len = 0;
+ uint64 len = 0;
while (src < end)
{
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 907b5ab7b0..47aa5be3a9 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -388,7 +388,7 @@ byteaout(PG_FUNCTION_ARGS)
{
/* Print traditional escaped format */
char *vp;
- int len;
+ int64 len;
int i;
len = 1; /* empty string has 1 char */
@@ -3455,7 +3455,7 @@ Datum
byteaGetBit(PG_FUNCTION_ARGS)
{
bytea *v = PG_GETARG_BYTEA_PP(0);
- int32 n = PG_GETARG_INT32(1);
+ int64 n = PG_GETARG_INT64(1);
int byteNo,
bitNo;
int len;
@@ -3463,14 +3463,17 @@ byteaGetBit(PG_FUNCTION_ARGS)
len = VARSIZE_ANY_EXHDR(v);
- if (n < 0 || n >= len * 8)
+ if (n < 0 || n >= (int64)len * 8)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
- errmsg("index %d out of valid range, 0..%d",
- n, len * 8 - 1)));
-
- byteNo = n / 8;
- bitNo = n % 8;
+ errmsg("index "INT64_FORMAT" out of valid range, 0.."INT64_FORMAT,
+ n, (int64)len * 8 - 1)));
+ /*
+ * The value of 'n' is smaller than 'len * 8', and 'len' is by int type,
+ * so it is safe to downcasting 'n' from int64 to int.
+ */
+ byteNo = (int)(n / 8);
+ bitNo = (int)(n % 8);
byte = ((unsigned char *) VARDATA_ANY(v))[byteNo];
@@ -3524,7 +3527,7 @@ Datum
byteaSetBit(PG_FUNCTION_ARGS)
{
bytea *res = PG_GETARG_BYTEA_P_COPY(0);
- int32 n = PG_GETARG_INT32(1);
+ int64 n = PG_GETARG_INT64(1);
int32 newBit = PG_GETARG_INT32(2);
int len;
int oldByte,
@@ -3534,14 +3537,17 @@ byteaSetBit(PG_FUNCTION_ARGS)
len = VARSIZE(res) - VARHDRSZ;
- if (n < 0 || n >= len * 8)
+ if (n < 0 || n >= (int64)len * 8)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
- errmsg("index %d out of valid range, 0..%d",
- n, len * 8 - 1)));
-
- byteNo = n / 8;
- bitNo = n % 8;
+ errmsg("index "INT64_FORMAT" out of valid range, 0.."INT64_FORMAT,
+ n, (int64)len * 8 - 1)));
+ /*
+ * The value of 'n' is smaller than 'len * 8', and 'len' is by int type,
+ * so it is safe to downcasting 'n' from int64 to int.
+ */
+ byteNo = (int64)(n / 8);
+ bitNo = (int64)(n % 8);
/*
* sanity check!
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index a6a708cca9..218af5ab2f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1403,10 +1403,10 @@
proname => 'set_byte', prorettype => 'bytea',
proargtypes => 'bytea int4 int4', prosrc => 'byteaSetByte' },
{ oid => '723', descr => 'get bit',
- proname => 'get_bit', prorettype => 'int4', proargtypes => 'bytea int4',
+ proname => 'get_bit', prorettype => 'int4', proargtypes => 'bytea int8',
prosrc => 'byteaGetBit' },
{ oid => '724', descr => 'set bit',
- proname => 'set_bit', prorettype => 'bytea', proargtypes => 'bytea int4 int4',
+ proname => 'set_bit', prorettype => 'bytea', proargtypes => 'bytea int8 int4',
prosrc => 'byteaSetBit' },
{ oid => '749', descr => 'substitute portion of string',
proname => 'overlay', prorettype => 'bytea',
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index e2016a8bc2..a352a8b773 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -32,8 +32,8 @@ extern int errdatatype(Oid datatypeOid);
extern int errdomainconstraint(Oid datatypeOid, const char *conname);
/* encode.c */
-extern unsigned hex_encode(const char *src, unsigned len, char *dst);
-extern unsigned hex_decode(const char *src, unsigned len, char *dst);
+extern uint64 hex_encode(const char *src, size_t len, char *dst);
+extern uint64 hex_decode(const char *src, size_t len, char *dst);
/* int.c */
extern int2vector *buildint2vector(const int16 *int2s, int n);
diff --git a/src/test/regress/expected/bit.out b/src/test/regress/expected/bit.out
index a1fab7ebcb..8e56289968 100644
--- a/src/test/regress/expected/bit.out
+++ b/src/test/regress/expected/bit.out
@@ -656,6 +656,40 @@ SELECT set_bit(B'0101011000100100', 15, 1);
SELECT set_bit(B'0101011000100100', 16, 1); -- fail
ERROR: bit index 16 out of valid range (0..15)
+SELECT get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 0)
+ ,0);
+ get_bit
+---------
+ 0
+(1 row)
+
+SELECT get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 1)
+ ,0);
+ get_bit
+---------
+ 1
+(1 row)
+
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea,
+ 512::bigint * 1024 * 1024 * 8 - 1, 0)
+ ,512::bigint * 1024 * 1024 * 8 - 1);
+ get_bit
+---------
+ 0
+(1 row)
+
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea,
+ 512::bigint * 1024 * 1024 * 8 - 1, 1)
+ ,512::bigint * 1024 * 1024 * 8 - 1);
+ get_bit
+---------
+ 1
+(1 row)
+
-- Overlay
SELECT overlay(B'0101011100' placing '001' from 2 for 3);
overlay
diff --git a/src/test/regress/sql/bit.sql b/src/test/regress/sql/bit.sql
index 7681d4ab4d..5914095941 100644
--- a/src/test/regress/sql/bit.sql
+++ b/src/test/regress/sql/bit.sql
@@ -200,6 +200,21 @@ DROP TABLE VARBIT_SHIFT_TABLE;
SELECT get_bit(B'0101011000100', 10);
SELECT set_bit(B'0101011000100100', 15, 1);
SELECT set_bit(B'0101011000100100', 16, 1); -- fail
+SELECT get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 0)
+ ,0);
+SELECT get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 1)
+ ,0);
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea,
+ 512::bigint * 1024 * 1024 * 8 - 1, 0)
+ ,512::bigint * 1024 * 1024 * 8 - 1);
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea,
+ 512::bigint * 1024 * 1024 * 8 - 1, 1)
+ ,512::bigint * 1024 * 1024 * 8 - 1);
+
-- Overlay
SELECT overlay(B'0101011100' placing '001' from 2 for 3);
view thread (27+ messages) latest in thread
Message-ID: <2020040214260375403022@highgo.ca>
Permalink: ../2020040214260375403022@highgo.ca/
Also on: postgresql.org/message-id/2020040214260375403022@highgo.ca
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: movead.li@highgo.ca, tgl@sss.pgh.pa.us, ashutosh.bapat@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: A bug when use get_bit() function for a long bytea string
In-Reply-To: <2020040214260375403022@highgo.ca>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox