pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17847: Unaligned memory access in ltree_gist
20+ messages / 5 participants
[nested] [flat]
* BUG #17847: Unaligned memory access in ltree_gist
@ 2023-03-16 18:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 20+ messages in thread
From: PG Bug reporting form @ 2023-03-16 18:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 17847
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 15.2
Operating system: Ubuntu 22.04
Description:
When the following query executed with address sanitizers (and
-fsanitize=alignment):
CREATE EXTENSION ltree;
CREATE TABLE lt (t ltree);
INSERT INTO lt SELECT format('%s.%s', i / 10, i % 10)::ltree FROM
generate_series(1, 200) i;
CREATE INDEX ltidx ON lt USING gist (t gist_ltree_ops(siglen=99));
An incorrect memory access is detected:
ltree_gist.c:66:12: runtime error: member access within misaligned address
0x62500019bfd3 for type 'varattrib_4b', which requires 4 byte alignment
That line contains:
memcpy(LTG_RNODE(result, siglen), right, VARSIZE(right));
Here the following macros are used:
#define VARSIZE(PTR)
VARSIZE_4B(PTR)
/* VARSIZE_4B() should only be used on known-aligned data */
#define VARSIZE_4B(PTR) \
((((varattrib_4b *) (PTR))->va_4byte.va_header >> 2) & 0x3FFFFFFF)
#define LTG_RNODE(x, siglen) ( LTG_ISNORIGHT(x) ? LTG_LNODE(x, siglen) :
LTG_RENODE(x, siglen) )
#define LTG_LNODE(x, siglen) ( (ltree*)( ( ((char*)(x))+LTG_HDRSIZE ) + (
LTG_ISALLTRUE(x) ? 0 : (siglen) ) ) )
#define LTG_RENODE(x, siglen) ( (ltree*)( ((char*)LTG_LNODE(x, siglen)) +
VARSIZE(LTG_LNODE(x, siglen))) )
that can be expanded as:
... + VARSIZE( (ltree*)( ( ((char*)(result))+LTG_HDRSIZE ) + (
LTG_ISALLTRUE(result) ? 0 : (siglen) ) ) )
Even though result and LTG_HDRSIZE are aligned, with an arbitrary siglen
value we get a pointer that is not suitable for VARSIZE_4B.
BTW, d952373a9 made the comment in src/include/utils/memutils.h outdated:
* ... See VARSIZE_4B() and related macros in
* postgres.h. ...
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-03-16 19:35 Tom Lane <tgl@sss.pgh.pa.us>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 2 replies; 20+ messages in thread
From: Tom Lane @ 2023-03-16 19:35 UTC (permalink / raw)
To: exclusion@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org, Alexander Korotkov <aekorotkov@gmail.com>
PG Bug reporting form <noreply@postgresql.org> writes:
> When the following query executed with address sanitizers (and
> -fsanitize=alignment):
> CREATE EXTENSION ltree;
> CREATE TABLE lt (t ltree);
> INSERT INTO lt SELECT format('%s.%s', i / 10, i % 10)::ltree FROM
> generate_series(1, 200) i;
> CREATE INDEX ltidx ON lt USING gist (t gist_ltree_ops(siglen=99));
> An incorrect memory access is detected:
> ltree_gist.c:66:12: runtime error: member access within misaligned address
> 0x62500019bfd3 for type 'varattrib_4b', which requires 4 byte alignment
Yeah. So if you ask me, the problem here is that the option for
user-selectable siglen was added with no thought for the possibility
that there might be undocumented implementation restrictions on the
value. The code is assuming that siglen is MAXALIGN'd (or at least
int-aligned, I did not look too closely), and there was nothing wrong
with that assumption before.
What I'm inclined to do about this is add a restriction that the siglen
value be a multiple of MAXALIGN. It doesn't look like the reloption
mechanism has a way to specify that declaratively, but we could probably
get close enough by just making LTREE_GET_SIGLEN throw an error if it's
wrong. That's not ideal because you could probably get through making
an empty index without hitting the error, but I don't offhand see a
way to make it better.
If we decide that we don't need to back-patch a fix for this, maybe
we could instead extend the reloption mechanism to allow stronger
checks on supplied values. That might be tolerable given how few
alignment-picky machines there are these days.
I wonder which other opclasses besides ltree have the same issue.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-03-18 07:00 Alexander Lakhin <exclusion@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 20+ messages in thread
From: Alexander Lakhin @ 2023-03-18 07:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org, Alexander Korotkov <aekorotkov@gmail.com>
16.03.2023 22:35, Tom Lane wrote:
> I wonder which other opclasses besides ltree have the same issue.
I found no other similar places: siglen is also accepted by opclasses
gist__intbig_ops, gist_hstore_ops, gist_trgm_ops, tsvector_ops,
which implementations use functions
_intbig_alloc, ghstore_alloc, gtrgm_alloc, gtsvector_alloc,
and none of them adds siglen to a pointer.
Quick testing confirms that.
(gist__ltree_ops uses ltree_gist_alloc(), but the problematic branch
"if (left) ..." not reached for gist__ltree_ops)
Best regards,
Alexander
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-03-19 17:08 Alexander Korotkov <aekorotkov@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 20+ messages in thread
From: Alexander Korotkov @ 2023-03-19 17:08 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Mar 16, 2023 at 10:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> PG Bug reporting form <noreply@postgresql.org> writes:
> > When the following query executed with address sanitizers (and
> > -fsanitize=alignment):
> > CREATE EXTENSION ltree;
> > CREATE TABLE lt (t ltree);
> > INSERT INTO lt SELECT format('%s.%s', i / 10, i % 10)::ltree FROM
> > generate_series(1, 200) i;
> > CREATE INDEX ltidx ON lt USING gist (t gist_ltree_ops(siglen=99));
>
> > An incorrect memory access is detected:
> > ltree_gist.c:66:12: runtime error: member access within misaligned address
> > 0x62500019bfd3 for type 'varattrib_4b', which requires 4 byte alignment
>
> Yeah. So if you ask me, the problem here is that the option for
> user-selectable siglen was added with no thought for the possibility
> that there might be undocumented implementation restrictions on the
> value. The code is assuming that siglen is MAXALIGN'd (or at least
> int-aligned, I did not look too closely), and there was nothing wrong
> with that assumption before.
>
> What I'm inclined to do about this is add a restriction that the siglen
> value be a multiple of MAXALIGN. It doesn't look like the reloption
> mechanism has a way to specify that declaratively, but we could probably
> get close enough by just making LTREE_GET_SIGLEN throw an error if it's
> wrong. That's not ideal because you could probably get through making
> an empty index without hitting the error, but I don't offhand see a
> way to make it better.
Sorry for missing this.
Please, note that there are infrastructure of reltoption validators.
I think this is the most appropriate place to check for alignment of
siglen. That works even for empty indexes. See the attached patch.
------
Regards,
Alexander Korotkov
Attachments:
[application/octet-stream] 0001-Validate-ltree-siglen-v1.patch (2.8K, ../../CAPpHfds=s6CmMzB1pO2UX9QnUvz6nOWofdnK84NXPQdVp0VWEw@mail.gmail.com/2-0001-Validate-ltree-siglen-v1.patch)
download | inline diff:
From 6b820bbec62d863d95cd9ab7378d9024a2665c19 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 19 Mar 2023 19:58:27 +0300
Subject: [PATCH] Validate ltree siglen
---
contrib/ltree/expected/ltree.out | 4 +++-
contrib/ltree/ltree_gist.c | 13 +++++++++++++
contrib/ltree/sql/ltree.sql | 1 +
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/contrib/ltree/expected/ltree.out b/contrib/ltree/expected/ltree.out
index 27122153c71..f2e30caadd9 100644
--- a/contrib/ltree/expected/ltree.out
+++ b/contrib/ltree/expected/ltree.out
@@ -7837,7 +7837,9 @@ create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
ERROR: value 0 out of bounds for option "siglen"
DETAIL: Valid values are between "1" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
-ERROR: value 2025 out of bounds for option "siglen"
+ERROR: siglen value must be integer-alignment
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+ERROR: value 2028 out of bounds for option "siglen"
DETAIL: Valid values are between "1" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
diff --git a/contrib/ltree/ltree_gist.c b/contrib/ltree/ltree_gist.c
index 3cba2269d86..1fc9e9fcf5d 100644
--- a/contrib/ltree/ltree_gist.c
+++ b/contrib/ltree/ltree_gist.c
@@ -719,6 +719,18 @@ ltree_consistent(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(res);
}
+static void
+ltree_gist_relopts_validator(void *parsed_options, relopt_value *vals,
+ int nvals)
+{
+ LtreeGistOptions *options = (LtreeGistOptions *) parsed_options;
+
+ if (options->siglen != INTALIGN(options->siglen))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("siglen value must be integer-alignment")));
+}
+
Datum
ltree_gist_options(PG_FUNCTION_ARGS)
{
@@ -729,6 +741,7 @@ ltree_gist_options(PG_FUNCTION_ARGS)
"signature length in bytes",
LTREE_SIGLEN_DEFAULT, 1, LTREE_SIGLEN_MAX,
offsetof(LtreeGistOptions, siglen));
+ register_reloptions_validator(relopts, ltree_gist_relopts_validator);
PG_RETURN_VOID();
}
diff --git a/contrib/ltree/sql/ltree.sql b/contrib/ltree/sql/ltree.sql
index 4623b57f7b5..57f9559b4ac 100644
--- a/contrib/ltree/sql/ltree.sql
+++ b/contrib/ltree/sql/ltree.sql
@@ -331,6 +331,7 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-03-19 17:09 Alexander Korotkov <aekorotkov@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Korotkov @ 2023-03-19 17:09 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
On Sat, Mar 18, 2023 at 10:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> 16.03.2023 22:35, Tom Lane wrote:
>
> I wonder which other opclasses besides ltree have the same issue.
>
>
> I found no other similar places: siglen is also accepted by opclasses
> gist__intbig_ops, gist_hstore_ops, gist_trgm_ops, tsvector_ops,
> which implementations use functions
> _intbig_alloc, ghstore_alloc, gtrgm_alloc, gtsvector_alloc,
> and none of them adds siglen to a pointer.
> Quick testing confirms that.
>
> (gist__ltree_ops uses ltree_gist_alloc(), but the problematic branch
> "if (left) ..." not reached for gist__ltree_ops)
Yep, ltree seems the only place storing aligned value *after*
signature. Thank you for validating this.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-03-20 07:00 Alexander Lakhin <exclusion@gmail.com>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Lakhin @ 2023-03-20 07:00 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
Hi,
19.03.2023 20:08, Alexander Korotkov wrote:
> On Thu, Mar 16, 2023 at 10:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I'm inclined to do about this is add a restriction that the siglen
>> value be a multiple of MAXALIGN. It doesn't look like the reloption
>> mechanism has a way to specify that declaratively, but we could probably
>> get close enough by just making LTREE_GET_SIGLEN throw an error if it's
>> wrong. That's not ideal because you could probably get through making
>> an empty index without hitting the error, but I don't offhand see a
>> way to make it better.
> Sorry for missing this.
>
> Please, note that there are infrastructure of reltoption validators.
> I think this is the most appropriate place to check for alignment of
> siglen. That works even for empty indexes. See the attached patch.
Thanks for the fix! It works for me.
Maybe it's worth to reflect this restriction in the documentation too?
<literal>gist_ltree_ops</literal> GiST opclass approximates a set of
path labels as a bitmap signature. Its optional integer parameter
<literal>siglen</literal> determines the
signature length in bytes. The default signature length is 8 bytes.
Valid values of signature length are between 1 and 2024 bytes.
How about "The length must be a multiple of <type>int</type> alignment between 4 and 2024."?
(There is a wording "<type>int</type> alignment (4 bytes on most machines)" in catalogs.sgml.)
Also maybe change the error message a little:
s/siglen value must be integer-alignment/siglen value must be integer-aligned/
or "int-aligned"? (this spelling can be found in src/)
(There is also a detail message, that probably should be corrected too:
DETAIL: Valid values are between "1" and "2024".
->
DETAIL: Valid values are int-aligned positive integers less than 2024.
?)
Best regards,
Alexander
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 10:16 Alexander Korotkov <aekorotkov@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Korotkov @ 2023-04-18 10:16 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
Hi Alexander,
Thank you for your feedback.
The revised patch is attached.
On Mon, Mar 20, 2023 at 10:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> 19.03.2023 20:08, Alexander Korotkov wrote:
> > On Thu, Mar 16, 2023 at 10:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> What I'm inclined to do about this is add a restriction that the siglen
> >> value be a multiple of MAXALIGN. It doesn't look like the reloption
> >> mechanism has a way to specify that declaratively, but we could probably
> >> get close enough by just making LTREE_GET_SIGLEN throw an error if it's
> >> wrong. That's not ideal because you could probably get through making
> >> an empty index without hitting the error, but I don't offhand see a
> >> way to make it better.
> > Sorry for missing this.
> >
> > Please, note that there are infrastructure of reltoption validators.
> > I think this is the most appropriate place to check for alignment of
> > siglen. That works even for empty indexes. See the attached patch.
>
> Thanks for the fix! It works for me.
>
> Maybe it's worth to reflect this restriction in the documentation too?
> <literal>gist_ltree_ops</literal> GiST opclass approximates a set of
> path labels as a bitmap signature. Its optional integer parameter
> <literal>siglen</literal> determines the
> signature length in bytes. The default signature length is 8 bytes.
> Valid values of signature length are between 1 and 2024 bytes.
>
> How about "The length must be a multiple of <type>int</type> alignment between 4 and 2024."?
> (There is a wording "<type>int</type> alignment (4 bytes on most machines)" in catalogs.sgml.)
I think it's a bit contradictory to say that int alignment is 4 bytes
on most machines, but the minimum value is exactly 4. The revised
patch says just that length is positive up to 2024.
> Also maybe change the error message a little:
> s/siglen value must be integer-alignment/siglen value must be integer-aligned/
> or "int-aligned"? (this spelling can be found in src/)
Thank you, accepted.
> (There is also a detail message, that probably should be corrected too:
> DETAIL: Valid values are between "1" and "2024".
> ->
> DETAIL: Valid values are int-aligned positive integers less than 2024.
> ?)
I can't edit directly the detail message for GUC min/max violation.
But I've corrected the min value to INTALIGN(1). Also, I've added
detail message for alignment validation.
I'm going to push this if no objections.
------
Regards,
Alexander Korotkov
Attachments:
[application/octet-stream] 0001-Validate-ltree-siglen-GiST-option-v2.patch (4.3K, ../../CAPpHfdvvB1-tGha_CXeN5ojWZ48=C77sSrVkgSLLFCVMfv0s1Q@mail.gmail.com/2-0001-Validate-ltree-siglen-GiST-option-v2.patch)
download | inline diff:
From 1ee1415880bf30889ce880c9c8da393cdc3ad783 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 19 Mar 2023 19:58:27 +0300
Subject: [PATCH] Validate ltree siglen GiST option
Unaligned siglen could lead to an unaligned access to subsequent key fields.
---
contrib/ltree/expected/ltree.out | 9 ++++++---
contrib/ltree/ltree_gist.c | 19 ++++++++++++++++++-
contrib/ltree/sql/ltree.sql | 1 +
doc/src/sgml/ltree.sgml | 3 ++-
4 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/contrib/ltree/expected/ltree.out b/contrib/ltree/expected/ltree.out
index 27122153c71..f0dcc4019f8 100644
--- a/contrib/ltree/expected/ltree.out
+++ b/contrib/ltree/expected/ltree.out
@@ -7835,10 +7835,13 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
ERROR: value 0 out of bounds for option "siglen"
-DETAIL: Valid values are between "1" and "2024".
+DETAIL: Valid values are between "4" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
-ERROR: value 2025 out of bounds for option "siglen"
-DETAIL: Valid values are between "1" and "2024".
+ERROR: siglen value must be integer-aligned
+DETAIL: Valid values are int-aligned positive integers up to 2024.
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+ERROR: value 2028 out of bounds for option "siglen"
+DETAIL: Valid values are between "4" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
count
diff --git a/contrib/ltree/ltree_gist.c b/contrib/ltree/ltree_gist.c
index 3cba2269d86..04c319b72d4 100644
--- a/contrib/ltree/ltree_gist.c
+++ b/contrib/ltree/ltree_gist.c
@@ -719,6 +719,20 @@ ltree_consistent(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(res);
}
+static void
+ltree_gist_relopts_validator(void *parsed_options, relopt_value *vals,
+ int nvals)
+{
+ LtreeGistOptions *options = (LtreeGistOptions *) parsed_options;
+
+ if (options->siglen != INTALIGN(options->siglen))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("siglen value must be integer-aligned"),
+ errdetail("Valid values are int-aligned positive integers up to %d.",
+ LTREE_SIGLEN_MAX)));
+}
+
Datum
ltree_gist_options(PG_FUNCTION_ARGS)
{
@@ -727,8 +741,11 @@ ltree_gist_options(PG_FUNCTION_ARGS)
init_local_reloptions(relopts, sizeof(LtreeGistOptions));
add_local_int_reloption(relopts, "siglen",
"signature length in bytes",
- LTREE_SIGLEN_DEFAULT, 1, LTREE_SIGLEN_MAX,
+ LTREE_SIGLEN_DEFAULT,
+ INTALIGN(1),
+ LTREE_SIGLEN_MAX,
offsetof(LtreeGistOptions, siglen));
+ register_reloptions_validator(relopts, ltree_gist_relopts_validator);
PG_RETURN_VOID();
}
diff --git a/contrib/ltree/sql/ltree.sql b/contrib/ltree/sql/ltree.sql
index 4623b57f7b5..57f9559b4ac 100644
--- a/contrib/ltree/sql/ltree.sql
+++ b/contrib/ltree/sql/ltree.sql
@@ -331,6 +331,7 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
diff --git a/doc/src/sgml/ltree.sgml b/doc/src/sgml/ltree.sgml
index bb66e33944b..00a6ae70da3 100644
--- a/doc/src/sgml/ltree.sgml
+++ b/doc/src/sgml/ltree.sgml
@@ -637,7 +637,8 @@ Europe & Russia*@ & !Transportation
path labels as a bitmap signature. Its optional integer parameter
<literal>siglen</literal> determines the
signature length in bytes. The default signature length is 8 bytes.
- Valid values of signature length are between 1 and 2024 bytes. Longer
+ The length must be a positive multiple of <type>int</type> alignment
+ (4 bytes on most machines)) up to 2024. Longer
signatures lead to a more precise search (scanning a smaller fraction of the index and
fewer heap pages), at the cost of a larger index.
</para>
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 10:34 Pavel Borisov <pashkin.elfe@gmail.com>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Pavel Borisov @ 2023-04-18 10:34 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
Hi, Alexander and Alexander!
On Tue, 18 Apr 2023 at 14:16, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi Alexander,
>
> Thank you for your feedback.
>
> The revised patch is attached.
>
> On Mon, Mar 20, 2023 at 10:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> > 19.03.2023 20:08, Alexander Korotkov wrote:
> > > On Thu, Mar 16, 2023 at 10:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> What I'm inclined to do about this is add a restriction that the siglen
> > >> value be a multiple of MAXALIGN. It doesn't look like the reloption
> > >> mechanism has a way to specify that declaratively, but we could probably
> > >> get close enough by just making LTREE_GET_SIGLEN throw an error if it's
> > >> wrong. That's not ideal because you could probably get through making
> > >> an empty index without hitting the error, but I don't offhand see a
> > >> way to make it better.
> > > Sorry for missing this.
> > >
> > > Please, note that there are infrastructure of reltoption validators.
> > > I think this is the most appropriate place to check for alignment of
> > > siglen. That works even for empty indexes. See the attached patch.
> >
> > Thanks for the fix! It works for me.
> >
> > Maybe it's worth to reflect this restriction in the documentation too?
> > <literal>gist_ltree_ops</literal> GiST opclass approximates a set of
> > path labels as a bitmap signature. Its optional integer parameter
> > <literal>siglen</literal> determines the
> > signature length in bytes. The default signature length is 8 bytes.
> > Valid values of signature length are between 1 and 2024 bytes.
> >
> > How about "The length must be a multiple of <type>int</type> alignment between 4 and 2024."?
> > (There is a wording "<type>int</type> alignment (4 bytes on most machines)" in catalogs.sgml.)
>
> I think it's a bit contradictory to say that int alignment is 4 bytes
> on most machines, but the minimum value is exactly 4. The revised
> patch says just that length is positive up to 2024.
>
> > Also maybe change the error message a little:
> > s/siglen value must be integer-alignment/siglen value must be integer-aligned/
> > or "int-aligned"? (this spelling can be found in src/)
>
> Thank you, accepted.
>
> > (There is also a detail message, that probably should be corrected too:
> > DETAIL: Valid values are between "1" and "2024".
> > ->
> > DETAIL: Valid values are int-aligned positive integers less than 2024.
> > ?)
>
> I can't edit directly the detail message for GUC min/max violation.
> But I've corrected the min value to INTALIGN(1). Also, I've added
> detail message for alignment validation.
>
> I'm going to push this if no objections.
I've looked into the patch v2 and there is a difference in DETAIL text
for the cases:
(1)
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
+ERROR: siglen value must be integer-aligned
+DETAIL: Valid values are int-aligned positive integers up to 2024.
(2)
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+ERROR: value 2028 out of bounds for option "siglen"
+DETAIL: Valid values are between "4" and "2024"
Could we stick to the DETAIL like in (2) for both cases?
Overall the patch seems good to be committed.
Regards,
Pavel Borisov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 10:57 Alexander Korotkov <aekorotkov@gmail.com>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Korotkov @ 2023-04-18 10:57 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
Hi, Pavel!
On Tue, Apr 18, 2023 at 1:34 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> I've looked into the patch v2 and there is a difference in DETAIL text
> for the cases:
>
> (1)
> create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
> +ERROR: siglen value must be integer-aligned
> +DETAIL: Valid values are int-aligned positive integers up to 2024.
>
> (2)
> +create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
> +ERROR: value 2028 out of bounds for option "siglen"
> +DETAIL: Valid values are between "4" and "2024"
>
> Could we stick to the DETAIL like in (2) for both cases?
Within ltree we don't have control over error messages, which GUC code
emits about min/max boundaries violation (for sure, we're not going to
patch GUC code in this fix). So the only thing I can do to match
these two DETAIL is to make both of them 'Valid values are between "4"
and "2024"'. However, this message would be kind of irrelevant for
"siglen value must be integer-aligned" error. It's strange for me
when an error mentions alignment, but DETAIL does not.
Do you think we can just remove the DETAIL for "siglen value must be
integer-aligned" error?
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 11:11 Pavel Borisov <pashkin.elfe@gmail.com>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 2 replies; 20+ messages in thread
From: Pavel Borisov @ 2023-04-18 11:11 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
Hi, Alexander!
On Tue, 18 Apr 2023 at 14:57, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi, Pavel!
>
> On Tue, Apr 18, 2023 at 1:34 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > I've looked into the patch v2 and there is a difference in DETAIL text
> > for the cases:
> >
> > (1)
> > create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
> > +ERROR: siglen value must be integer-aligned
> > +DETAIL: Valid values are int-aligned positive integers up to 2024.
> >
> > (2)
> > +create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
> > +ERROR: value 2028 out of bounds for option "siglen"
> > +DETAIL: Valid values are between "4" and "2024"
> >
> > Could we stick to the DETAIL like in (2) for both cases?
>
> Within ltree we don't have control over error messages, which GUC code
> emits about min/max boundaries violation (for sure, we're not going to
> patch GUC code in this fix). So the only thing I can do to match
> these two DETAIL is to make both of them 'Valid values are between "4"
> and "2024"'. However, this message would be kind of irrelevant for
> "siglen value must be integer-aligned" error. It's strange for me
> when an error mentions alignment, but DETAIL does not.
>
> Do you think we can just remove the DETAIL for "siglen value must be
> integer-aligned" error?
I'd just propose something like making DETAIL output in ltree look
similar to GUC validation (patch v3). But it's minor and could be done
by removing DETAIL at all or otherwise.
Regards,
Pavel.
Attachments:
[application/octet-stream] v3-0001-Validate-ltree-siglen-GiST-option.patch (4.3K, ../../CALT9ZEHCN686xmRGorzoAcAJfE6NnNwDuAE=TxAe1tcp5aQTiA@mail.gmail.com/2-v3-0001-Validate-ltree-siglen-GiST-option.patch)
download | inline diff:
From ba07fa65f67dd5204224c9d82cd73c7983a618b3 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 19 Mar 2023 19:58:27 +0300
Subject: [PATCH v3] Validate ltree siglen GiST option
Unaligned siglen could lead to an unaligned access to subsequent key fields.
---
contrib/ltree/expected/ltree.out | 9 ++++++---
contrib/ltree/ltree_gist.c | 19 ++++++++++++++++++-
contrib/ltree/sql/ltree.sql | 1 +
doc/src/sgml/ltree.sgml | 3 ++-
4 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/contrib/ltree/expected/ltree.out b/contrib/ltree/expected/ltree.out
index 27122153c71..d893f69c094 100644
--- a/contrib/ltree/expected/ltree.out
+++ b/contrib/ltree/expected/ltree.out
@@ -7835,10 +7835,13 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
ERROR: value 0 out of bounds for option "siglen"
-DETAIL: Valid values are between "1" and "2024".
+DETAIL: Valid values are between "4" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
-ERROR: value 2025 out of bounds for option "siglen"
-DETAIL: Valid values are between "1" and "2024".
+ERROR: siglen value must be integer-aligned
+DETAIL: Valid are int-aligned values between "4" and "2024".
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+ERROR: value 2028 out of bounds for option "siglen"
+DETAIL: Valid values are between "4" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
count
diff --git a/contrib/ltree/ltree_gist.c b/contrib/ltree/ltree_gist.c
index 3cba2269d86..578e9a89742 100644
--- a/contrib/ltree/ltree_gist.c
+++ b/contrib/ltree/ltree_gist.c
@@ -719,6 +719,20 @@ ltree_consistent(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(res);
}
+static void
+ltree_gist_relopts_validator(void *parsed_options, relopt_value *vals,
+ int nvals)
+{
+ LtreeGistOptions *options = (LtreeGistOptions *) parsed_options;
+
+ if (options->siglen != INTALIGN(options->siglen))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("siglen value must be integer-aligned"),
+ errdetail("Valid are int-aligned values between \"%d\" and \"%d\".",
+ ALIGNOF_INT, LTREE_SIGLEN_MAX)));
+}
+
Datum
ltree_gist_options(PG_FUNCTION_ARGS)
{
@@ -727,8 +741,11 @@ ltree_gist_options(PG_FUNCTION_ARGS)
init_local_reloptions(relopts, sizeof(LtreeGistOptions));
add_local_int_reloption(relopts, "siglen",
"signature length in bytes",
- LTREE_SIGLEN_DEFAULT, 1, LTREE_SIGLEN_MAX,
+ LTREE_SIGLEN_DEFAULT,
+ INTALIGN(1),
+ LTREE_SIGLEN_MAX,
offsetof(LtreeGistOptions, siglen));
+ register_reloptions_validator(relopts, ltree_gist_relopts_validator);
PG_RETURN_VOID();
}
diff --git a/contrib/ltree/sql/ltree.sql b/contrib/ltree/sql/ltree.sql
index 4623b57f7b5..57f9559b4ac 100644
--- a/contrib/ltree/sql/ltree.sql
+++ b/contrib/ltree/sql/ltree.sql
@@ -331,6 +331,7 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
diff --git a/doc/src/sgml/ltree.sgml b/doc/src/sgml/ltree.sgml
index bb66e33944b..00a6ae70da3 100644
--- a/doc/src/sgml/ltree.sgml
+++ b/doc/src/sgml/ltree.sgml
@@ -637,7 +637,8 @@ Europe & Russia*@ & !Transportation
path labels as a bitmap signature. Its optional integer parameter
<literal>siglen</literal> determines the
signature length in bytes. The default signature length is 8 bytes.
- Valid values of signature length are between 1 and 2024 bytes. Longer
+ The length must be a positive multiple of <type>int</type> alignment
+ (4 bytes on most machines)) up to 2024. Longer
signatures lead to a more precise search (scanning a smaller fraction of the index and
fewer heap pages), at the cost of a larger index.
</para>
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 11:16 Pavel Borisov <pashkin.elfe@gmail.com>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
1 sibling, 0 replies; 20+ messages in thread
From: Pavel Borisov @ 2023-04-18 11:16 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
On Tue, 18 Apr 2023 at 15:11, Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>
> Hi, Alexander!
>
> On Tue, 18 Apr 2023 at 14:57, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > Hi, Pavel!
> >
> > On Tue, Apr 18, 2023 at 1:34 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > > I've looked into the patch v2 and there is a difference in DETAIL text
> > > for the cases:
> > >
> > > (1)
> > > create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
> > > +ERROR: siglen value must be integer-aligned
> > > +DETAIL: Valid values are int-aligned positive integers up to 2024.
> > >
> > > (2)
> > > +create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
> > > +ERROR: value 2028 out of bounds for option "siglen"
> > > +DETAIL: Valid values are between "4" and "2024"
> > >
> > > Could we stick to the DETAIL like in (2) for both cases?
> >
> > Within ltree we don't have control over error messages, which GUC code
> > emits about min/max boundaries violation (for sure, we're not going to
> > patch GUC code in this fix). So the only thing I can do to match
> > these two DETAIL is to make both of them 'Valid values are between "4"
> > and "2024"'. However, this message would be kind of irrelevant for
> > "siglen value must be integer-aligned" error. It's strange for me
> > when an error mentions alignment, but DETAIL does not.
> >
> > Do you think we can just remove the DETAIL for "siglen value must be
> > integer-aligned" error?
>
> I'd just propose something like making DETAIL output in ltree look
> similar to GUC validation (patch v3). But it's minor and could be done
> by removing DETAIL at all or otherwise.
I think my confusion on error output is due to alignment being checked
first, then limits. So in DETAIL I see different error reasons for
2025 and 2028.
Pavel
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 11:43 Alexander Korotkov <aekorotkov@gmail.com>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Alexander Korotkov @ 2023-04-18 11:43 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
Hi, Pavel!
On Tue, Apr 18, 2023 at 2:11 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> On Tue, 18 Apr 2023 at 14:57, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Tue, Apr 18, 2023 at 1:34 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > > I've looked into the patch v2 and there is a difference in DETAIL text
> > > for the cases:
> > >
> > > (1)
> > > create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
> > > +ERROR: siglen value must be integer-aligned
> > > +DETAIL: Valid values are int-aligned positive integers up to 2024.
> > >
> > > (2)
> > > +create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
> > > +ERROR: value 2028 out of bounds for option "siglen"
> > > +DETAIL: Valid values are between "4" and "2024"
> > >
> > > Could we stick to the DETAIL like in (2) for both cases?
> >
> > Within ltree we don't have control over error messages, which GUC code
> > emits about min/max boundaries violation (for sure, we're not going to
> > patch GUC code in this fix). So the only thing I can do to match
> > these two DETAIL is to make both of them 'Valid values are between "4"
> > and "2024"'. However, this message would be kind of irrelevant for
> > "siglen value must be integer-aligned" error. It's strange for me
> > when an error mentions alignment, but DETAIL does not.
> >
> > Do you think we can just remove the DETAIL for "siglen value must be
> > integer-aligned" error?
>
> I'd just propose something like making DETAIL output in ltree look
> similar to GUC validation (patch v3). But it's minor and could be done
> by removing DETAIL at all or otherwise.
LGMT. I'm going to push v3 unless there are more comments.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 15:06 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2023-04-18 15:06 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Pavel Borisov <pashkin.elfe@gmail.com>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Alexander Korotkov <aekorotkov@gmail.com> writes:
> LGMT. I'm going to push v3 unless there are more comments.
I think this reads pretty awkwardly:
+ERROR: siglen value must be integer-aligned
+DETAIL: Valid are int-aligned values between "4" and "2024".
The DETAIL message's grammar seems a bit off. Also, this is confusing the
range limitation with the alignment requirement. How about just saying
ERROR: siglen value must be a multiple of 4
and leaving out-of-range cases to be handled by the existing check?
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 15:19 Pavel Borisov <pashkin.elfe@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 20+ messages in thread
From: Pavel Borisov @ 2023-04-18 15:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Korotkov <aekorotkov@gmail.com>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Hi, Tom!
On Tue, 18 Apr 2023 at 19:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > LGMT. I'm going to push v3 unless there are more comments.
>
> I think this reads pretty awkwardly:
>
> +ERROR: siglen value must be integer-aligned
> +DETAIL: Valid are int-aligned values between "4" and "2024".
>
> The DETAIL message's grammar seems a bit off. Also, this is confusing the
> range limitation with the alignment requirement. How about just saying
> ERROR: siglen value must be a multiple of 4
I definitely like this wording.
> and leaving out-of-range cases to be handled by the existing check?
But that means that if we try 2025 then we just get it is not multiple
of 4 (and no clue of the range). Then we try 2028 and get another
error that it's outside of range. I suppose giving clues one by one
makes this look like a step-by-step quest. But in principle it's
possible.
Regards,
Pavel Borisov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-18 15:25 Tom Lane <tgl@sss.pgh.pa.us>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2023-04-18 15:25 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Alexander Korotkov <aekorotkov@gmail.com>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Pavel Borisov <pashkin.elfe@gmail.com> writes:
> On Tue, 18 Apr 2023 at 19:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The DETAIL message's grammar seems a bit off. Also, this is confusing the
>> range limitation with the alignment requirement. How about just saying
>> ERROR: siglen value must be a multiple of 4
> I definitely like this wording.
>> and leaving out-of-range cases to be handled by the existing check?
> But that means that if we try 2025 then we just get it is not multiple
> of 4 (and no clue of the range). Then we try 2028 and get another
> error that it's outside of range. I suppose giving clues one by one
> makes this look like a step-by-step quest. But in principle it's
> possible.
Well, that's the case just about everywhere else that there are multiple
constraints on an input. It'd be impossibly unwieldy, and confusing,
for every error message to include details on all the other errors
you might have hit if you didn't hit that one.
FWIW, I find it odd that the multiple-of-4 error comes out in advance
of the range check; that seems backwards somehow. Maybe we should
alter the order of applying the range check and the custom validator?
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-19 16:26 Alexander Korotkov <aekorotkov@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Korotkov @ 2023-04-19 16:26 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Pavel Borisov <pashkin.elfe@gmail.com>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Hi!
On Tue, Apr 18, 2023 at 6:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pavel Borisov <pashkin.elfe@gmail.com> writes:
> > On Tue, 18 Apr 2023 at 19:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> The DETAIL message's grammar seems a bit off. Also, this is confusing the
> >> range limitation with the alignment requirement. How about just saying
> >> ERROR: siglen value must be a multiple of 4
This message is changed as you proposed.
> > I definitely like this wording.
>
> >> and leaving out-of-range cases to be handled by the existing check?
>
> > But that means that if we try 2025 then we just get it is not multiple
> > of 4 (and no clue of the range). Then we try 2028 and get another
> > error that it's outside of range. I suppose giving clues one by one
> > makes this look like a step-by-step quest. But in principle it's
> > possible.
>
> Well, that's the case just about everywhere else that there are multiple
> constraints on an input. It'd be impossibly unwieldy, and confusing,
> for every error message to include details on all the other errors
> you might have hit if you didn't hit that one.
>
> FWIW, I find it odd that the multiple-of-4 error comes out in advance
> of the range check; that seems backwards somehow. Maybe we should
> alter the order of applying the range check and the custom validator?
I think it was intended that custom validators work after builtin
validation, but the current order of validation is caused by bug. I
don't see why build_local_reloptions() should call custom validators
when validate == false. Patch 0001 fixes this. I propose to
backpatch this.
------
Regards,
Alexander Korotkov
Attachments:
[application/octet-stream] 0002-Validate-ltree-siglen-GiST-option-v4.patch (4.3K, ../../CAPpHfdtaZ_sb8Nar-1Kjpc4P-MS5kx5NCL+--L+2Bu47WnJ53w@mail.gmail.com/2-0002-Validate-ltree-siglen-GiST-option-v4.patch)
download | inline diff:
From ccbead48bcc2f92f73e4f148f19af823e8e06155 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 19 Mar 2023 19:58:27 +0300
Subject: [PATCH 2/2] Validate ltree siglen GiST option
Unaligned siglen could lead to an unaligned access to subsequent key fields.
---
contrib/ltree/expected/ltree.out | 9 ++++++---
contrib/ltree/ltree_gist.c | 17 ++++++++++++++++-
contrib/ltree/sql/ltree.sql | 2 ++
doc/src/sgml/ltree.sgml | 3 ++-
4 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/contrib/ltree/expected/ltree.out b/contrib/ltree/expected/ltree.out
index 27122153c71..f0dcc4019f8 100644
--- a/contrib/ltree/expected/ltree.out
+++ b/contrib/ltree/expected/ltree.out
@@ -7835,10 +7835,13 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
ERROR: value 0 out of bounds for option "siglen"
-DETAIL: Valid values are between "1" and "2024".
+DETAIL: Valid values are between "4" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
-ERROR: value 2025 out of bounds for option "siglen"
-DETAIL: Valid values are between "1" and "2024".
+ERROR: siglen value must be integer-aligned
+DETAIL: Valid values are int-aligned positive integers up to 2024.
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+ERROR: value 2028 out of bounds for option "siglen"
+DETAIL: Valid values are between "4" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
count
diff --git a/contrib/ltree/ltree_gist.c b/contrib/ltree/ltree_gist.c
index 3cba2269d86..21b7d020286 100644
--- a/contrib/ltree/ltree_gist.c
+++ b/contrib/ltree/ltree_gist.c
@@ -719,6 +719,18 @@ ltree_consistent(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(res);
}
+static void
+ltree_gist_relopts_validator(void *parsed_options, relopt_value *vals,
+ int nvals)
+{
+ LtreeGistOptions *options = (LtreeGistOptions *) parsed_options;
+
+ if (options->siglen != INTALIGN(options->siglen))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("siglen value must be a multiple of %d", ALIGNOF_INT)));
+}
+
Datum
ltree_gist_options(PG_FUNCTION_ARGS)
{
@@ -727,8 +739,11 @@ ltree_gist_options(PG_FUNCTION_ARGS)
init_local_reloptions(relopts, sizeof(LtreeGistOptions));
add_local_int_reloption(relopts, "siglen",
"signature length in bytes",
- LTREE_SIGLEN_DEFAULT, 1, LTREE_SIGLEN_MAX,
+ LTREE_SIGLEN_DEFAULT,
+ INTALIGN(1),
+ LTREE_SIGLEN_MAX,
offsetof(LtreeGistOptions, siglen));
+ register_reloptions_validator(relopts, ltree_gist_relopts_validator);
PG_RETURN_VOID();
}
diff --git a/contrib/ltree/sql/ltree.sql b/contrib/ltree/sql/ltree.sql
index 4623b57f7b5..402096f6c46 100644
--- a/contrib/ltree/sql/ltree.sql
+++ b/contrib/ltree/sql/ltree.sql
@@ -331,6 +331,8 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2019));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
diff --git a/doc/src/sgml/ltree.sgml b/doc/src/sgml/ltree.sgml
index bb66e33944b..00a6ae70da3 100644
--- a/doc/src/sgml/ltree.sgml
+++ b/doc/src/sgml/ltree.sgml
@@ -637,7 +637,8 @@ Europe & Russia*@ & !Transportation
path labels as a bitmap signature. Its optional integer parameter
<literal>siglen</literal> determines the
signature length in bytes. The default signature length is 8 bytes.
- Valid values of signature length are between 1 and 2024 bytes. Longer
+ The length must be a positive multiple of <type>int</type> alignment
+ (4 bytes on most machines)) up to 2024. Longer
signatures lead to a more precise search (scanning a smaller fraction of the index and
fewer heap pages), at the cost of a larger index.
</para>
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] 0001-Fix-custom-validators-call-in-build_local_relopti-v4.patch (1.1K, ../../CAPpHfdtaZ_sb8Nar-1Kjpc4P-MS5kx5NCL+--L+2Bu47WnJ53w@mail.gmail.com/3-0001-Fix-custom-validators-call-in-build_local_relopti-v4.patch)
download | inline diff:
From 147b765134a7eb96f8082766a04828aff5d6d15d Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 19 Apr 2023 19:11:13 +0300
Subject: [PATCH 1/2] Fix custom validators call in build_local_reloptions()
We need to call them only when validate == true.
---
src/backend/access/common/reloptions.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 14c23101adf..90cb3951fca 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1969,8 +1969,9 @@ build_local_reloptions(local_relopts *relopts, Datum options, bool validate)
fillRelOptions(opts, relopts->relopt_struct_size, vals, noptions, validate,
elems, noptions);
- foreach(lc, relopts->validators)
- ((relopts_validator) lfirst(lc)) (opts, vals, noptions);
+ if (validate)
+ foreach(lc, relopts->validators)
+ ((relopts_validator) lfirst(lc)) (opts, vals, noptions);
if (elems)
pfree(elems);
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-19 16:47 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2023-04-19 16:47 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Pavel Borisov <pashkin.elfe@gmail.com>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Alexander Korotkov <aekorotkov@gmail.com> writes:
> [ v4 patches ]
I think you forgot to adjust the expected output in ltree.out.
LGTM otherwise. I agree with back-patching.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-19 16:59 Alexander Korotkov <aekorotkov@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Korotkov @ 2023-04-19 16:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Pavel Borisov <pashkin.elfe@gmail.com>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Wed, Apr 19, 2023 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > [ v4 patches ]
>
> I think you forgot to adjust the expected output in ltree.out.
> LGTM otherwise. I agree with back-patching.
Thank you, Tom. The corrected version is attached.
I've registered it on commitfest to make it pass
commitfest.cputube.org. I'm going to push it once it passes.
------
Regards,
Alexander Korotkov
Attachments:
[application/octet-stream] 0001-Fix-custom-validators-call-in-build_local_relopti-v5.patch (1.1K, ../../CAPpHfduV1B5UORjzCpyYuFn6uFAm1583sjkgQs3NuexJBwo77w@mail.gmail.com/2-0001-Fix-custom-validators-call-in-build_local_relopti-v5.patch)
download | inline diff:
From 147b765134a7eb96f8082766a04828aff5d6d15d Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 19 Apr 2023 19:11:13 +0300
Subject: [PATCH 1/2] Fix custom validators call in build_local_reloptions()
We need to call them only when validate == true.
---
src/backend/access/common/reloptions.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 14c23101adf..90cb3951fca 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1969,8 +1969,9 @@ build_local_reloptions(local_relopts *relopts, Datum options, bool validate)
fillRelOptions(opts, relopts->relopt_struct_size, vals, noptions, validate,
elems, noptions);
- foreach(lc, relopts->validators)
- ((relopts_validator) lfirst(lc)) (opts, vals, noptions);
+ if (validate)
+ foreach(lc, relopts->validators)
+ ((relopts_validator) lfirst(lc)) (opts, vals, noptions);
if (elems)
pfree(elems);
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] 0002-Validate-ltree-siglen-GiST-option-v5.patch (4.3K, ../../CAPpHfduV1B5UORjzCpyYuFn6uFAm1583sjkgQs3NuexJBwo77w@mail.gmail.com/3-0002-Validate-ltree-siglen-GiST-option-v5.patch)
download | inline diff:
From 5b90bbaff7e3ba6f312b64dc510c9eed01f3461c Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 19 Mar 2023 19:58:27 +0300
Subject: [PATCH 2/2] Validate ltree siglen GiST option
Unaligned siglen could lead to an unaligned access to subsequent key fields.
---
contrib/ltree/expected/ltree.out | 9 +++++++--
contrib/ltree/ltree_gist.c | 17 ++++++++++++++++-
contrib/ltree/sql/ltree.sql | 2 ++
doc/src/sgml/ltree.sgml | 3 ++-
4 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/contrib/ltree/expected/ltree.out b/contrib/ltree/expected/ltree.out
index 27122153c71..984cd030cfa 100644
--- a/contrib/ltree/expected/ltree.out
+++ b/contrib/ltree/expected/ltree.out
@@ -7835,10 +7835,15 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
ERROR: value 0 out of bounds for option "siglen"
-DETAIL: Valid values are between "1" and "2024".
+DETAIL: Valid values are between "4" and "2024".
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
ERROR: value 2025 out of bounds for option "siglen"
-DETAIL: Valid values are between "1" and "2024".
+DETAIL: Valid values are between "4" and "2024".
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+ERROR: value 2028 out of bounds for option "siglen"
+DETAIL: Valid values are between "4" and "2024".
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2019));
+ERROR: siglen value must be a multiple of 4
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
count
diff --git a/contrib/ltree/ltree_gist.c b/contrib/ltree/ltree_gist.c
index 3cba2269d86..21b7d020286 100644
--- a/contrib/ltree/ltree_gist.c
+++ b/contrib/ltree/ltree_gist.c
@@ -719,6 +719,18 @@ ltree_consistent(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(res);
}
+static void
+ltree_gist_relopts_validator(void *parsed_options, relopt_value *vals,
+ int nvals)
+{
+ LtreeGistOptions *options = (LtreeGistOptions *) parsed_options;
+
+ if (options->siglen != INTALIGN(options->siglen))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("siglen value must be a multiple of %d", ALIGNOF_INT)));
+}
+
Datum
ltree_gist_options(PG_FUNCTION_ARGS)
{
@@ -727,8 +739,11 @@ ltree_gist_options(PG_FUNCTION_ARGS)
init_local_reloptions(relopts, sizeof(LtreeGistOptions));
add_local_int_reloption(relopts, "siglen",
"signature length in bytes",
- LTREE_SIGLEN_DEFAULT, 1, LTREE_SIGLEN_MAX,
+ LTREE_SIGLEN_DEFAULT,
+ INTALIGN(1),
+ LTREE_SIGLEN_MAX,
offsetof(LtreeGistOptions, siglen));
+ register_reloptions_validator(relopts, ltree_gist_relopts_validator);
PG_RETURN_VOID();
}
diff --git a/contrib/ltree/sql/ltree.sql b/contrib/ltree/sql/ltree.sql
index 4623b57f7b5..402096f6c46 100644
--- a/contrib/ltree/sql/ltree.sql
+++ b/contrib/ltree/sql/ltree.sql
@@ -331,6 +331,8 @@ SELECT * FROM ltreetest WHERE t ? '{23.*.1,23.*.2}' order by t asc;
drop index tstidx;
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=0));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2019));
create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2024));
SELECT count(*) FROM ltreetest WHERE t < '12.3';
diff --git a/doc/src/sgml/ltree.sgml b/doc/src/sgml/ltree.sgml
index bb66e33944b..00a6ae70da3 100644
--- a/doc/src/sgml/ltree.sgml
+++ b/doc/src/sgml/ltree.sgml
@@ -637,7 +637,8 @@ Europe & Russia*@ & !Transportation
path labels as a bitmap signature. Its optional integer parameter
<literal>siglen</literal> determines the
signature length in bytes. The default signature length is 8 bytes.
- Valid values of signature length are between 1 and 2024 bytes. Longer
+ The length must be a positive multiple of <type>int</type> alignment
+ (4 bytes on most machines)) up to 2024. Longer
signatures lead to a more precise search (scanning a smaller fraction of the index and
fewer heap pages), at the cost of a larger index.
</para>
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-19 18:56 Pavel Borisov <pashkin.elfe@gmail.com>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Pavel Borisov @ 2023-04-19 18:56 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
Hi!
On Wed, 19 Apr 2023 at 20:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Wed, Apr 19, 2023 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Alexander Korotkov <aekorotkov@gmail.com> writes:
> > > [ v4 patches ]
> >
> > I think you forgot to adjust the expected output in ltree.out.
> > LGTM otherwise. I agree with back-patching.
>
> Thank you, Tom. The corrected version is attached.
> I've registered it on commitfest to make it pass
> commitfest.cputube.org. I'm going to push it once it passes.
I've looked at both patches and they look good. The question about
reporting alignment when the value is out of range went away.
I think they are ready to be pushed.
Regards,
Pavel Borisov
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: BUG #17847: Unaligned memory access in ltree_gist
@ 2023-04-23 11:32 Alexander Korotkov <aekorotkov@gmail.com>
parent: Pavel Borisov <pashkin.elfe@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Korotkov @ 2023-04-23 11:32 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
On Wed, Apr 19, 2023 at 9:57 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> On Wed, 19 Apr 2023 at 20:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > On Wed, Apr 19, 2023 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Alexander Korotkov <aekorotkov@gmail.com> writes:
> > > > [ v4 patches ]
> > >
> > > I think you forgot to adjust the expected output in ltree.out.
> > > LGTM otherwise. I agree with back-patching.
> >
> > Thank you, Tom. The corrected version is attached.
> > I've registered it on commitfest to make it pass
> > commitfest.cputube.org. I'm going to push it once it passes.
>
> I've looked at both patches and they look good. The question about
> reporting alignment when the value is out of range went away.
> I think they are ready to be pushed.
Tom, Pavel, thank you!
Pushed!
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2023-04-23 11:32 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16 18:00 BUG #17847: Unaligned memory access in ltree_gist PG Bug reporting form <noreply@postgresql.org>
2023-03-16 19:35 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-18 07:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-03-19 17:09 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-03-19 17:08 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-03-20 07:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-04-18 10:16 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-04-18 10:34 ` Pavel Borisov <pashkin.elfe@gmail.com>
2023-04-18 10:57 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-04-18 11:11 ` Pavel Borisov <pashkin.elfe@gmail.com>
2023-04-18 11:16 ` Pavel Borisov <pashkin.elfe@gmail.com>
2023-04-18 11:43 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-04-18 15:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-04-18 15:19 ` Pavel Borisov <pashkin.elfe@gmail.com>
2023-04-18 15:25 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-04-19 16:26 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-04-19 16:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-04-19 16:59 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-04-19 18:56 ` Pavel Borisov <pashkin.elfe@gmail.com>
2023-04-23 11:32 ` Alexander Korotkov <aekorotkov@gmail.com>
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