BUG #18135: Incorrect memory access occurs when attaching a partition with an index

On Wed, Sep 27, 2023 at 10:00:01AM +0000, PG Bug reporting form wrote:

executed under Valgrind, it leads to an incorrect memory access:
==00:00:00:03.947 396156== Invalid read of size 2
==00:00:00:03.947 396156== at 0x2E323D: CompareIndexInfo (index.c:2572)
==00:00:00:03.947 396156== by 0x3D009B: AttachPartitionEnsureIndexes
(tablecmds.c:18797)
==00:00:00:03.947 396156== by 0x3D8B4F: ATExecAttachPartition
(tablecmds.c:18578)
==00:00:00:03.947 396156== by 0x3D9A88: ATExecCmd (tablecmds.c:5379)
==00:00:00:03.947 396156== by 0x3D9BC7: ATRewriteCatalogs
(tablecmds.c:5063)

I have just tested that on HEAD and REL_16_STABLE, but fail to see
this report, which is weird (3.19.0 here). Are you using any specific
option of valgrind I should be aware of? Here is what I used, for
reference:
valgrind \
--suppressions=$PG_SOURCE/src/tools/valgrind.supp \
--trace-children=yes --track-origins=yes --read-var-info=yes \
postgres -D REST_OF_ARGS

The function CompareIndexInfo() contains the code:
/* ignore expressions at this stage */
if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
(attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
info1->ii_IndexAttrNumbers[i]))
return false;

where info1->ii_IndexAttrNumbers[i] is checked for InvalidAttrNumber
(i. e. it's not an expression), but info2->ii_IndexAttrNumbers[i] is not.

Anyway, I can see your point here. info2's first attnum is 0 so we
look at an imaginary position in attmap->attnums. So, yes, that's
wrong.

In addition, there is a check whether both indexes are (are not)
expression indexes, but it's placed below...

Sure, but this makes the check a bit cheaper if the indexes to compare
use expr and non-expr attributes at the same attnums, no? Except if I
am missing something, the attached should be sufficient.
--
Michael

Hi Michael,

28.09.2023 06:30, Michael Paquier wrote:

On Wed, Sep 27, 2023 at 10:00:01AM +0000, PG Bug reporting form wrote:

executed under Valgrind, it leads to an incorrect memory access:
==00:00:00:03.947 396156== Invalid read of size 2
==00:00:00:03.947 396156== at 0x2E323D: CompareIndexInfo (index.c:2572)
==00:00:00:03.947 396156== by 0x3D009B: AttachPartitionEnsureIndexes
(tablecmds.c:18797)
==00:00:00:03.947 396156== by 0x3D8B4F: ATExecAttachPartition
(tablecmds.c:18578)
==00:00:00:03.947 396156== by 0x3D9A88: ATExecCmd (tablecmds.c:5379)
==00:00:00:03.947 396156== by 0x3D9BC7: ATRewriteCatalogs
(tablecmds.c:5063)

Thank you for paying attention to it!

I have just tested that on HEAD and REL_16_STABLE, but fail to see
this report, which is weird (3.19.0 here). Are you using any specific
option of valgrind I should be aware of? Here is what I used, for
reference:
valgrind \
--suppressions=$PG_SOURCE/src/tools/valgrind.supp \
--trace-children=yes --track-origins=yes --read-var-info=yes \
postgres -D REST_OF_ARGS

Please try the following procedure (I've simplified my own):
With the attached patch (for HEAD) applied
CPPFLAGS="-DUSE_VALGRIND -Og" ./configure -q --enable-debug --enable-cassert && make -s -j8
sed 's/create index on idxpart1 ((a + b));/create index on idxpart1 ((a));/' -i src/test/regress/sql/indexing.sql
TESTS=indexing make check-tests

I get:
not ok 1     - indexing                                 9844 ms
# (test process exited with exit code 2)

(I use valgrind-3.18.1.)

If you still see no error, please share details of your method for running
valgrind.

In addition, there is a check whether both indexes are (are not)
expression indexes, but it's placed below...

Sure, but this makes the check a bit cheaper if the indexes to compare
use expr and non-expr attributes at the same attnums, no? Except if I
am missing something, the attached should be sufficient.

I thought about placing that check before the loop, but your fix looks
more clear (and my testing confirms that it works).

Best regards,
Alexander

On Thu, Sep 28, 2023 at 08:00:00AM +0300, Alexander Lakhin wrote:

Please try the following procedure (I've simplified my own):
With the attached patch (for HEAD) applied
CPPFLAGS="-DUSE_VALGRIND "

USE_VALGRIND is the difference here. In the past problems with
valgrind I've seen from you, I've never needed that, actually.

In my case, my process is much simpler: I just use installcheck with
an instance I set up locally using a command like the one I mentioned
above. No need to patch the tree with that and self-contained SQL
sequences.

Anyway, mystery solved.
--
Michael

28.09.2023 10:07, Michael Paquier wrote:

In my case, my process is much simpler: I just use installcheck with
an instance I set up locally using a command like the one I mentioned
above. No need to patch the tree with that and self-contained SQL
sequences.

Yes, I know that method, but unfortunately it doesn't cover TAP tests,
so I prefer to use such patch to perform whole check-world (and run also all
postgres binaries, e.g., pg_dump) under valgrind.

As to patching regress/sql/, in this case my intention was to minimize the
self-contained repro. I use separate scripts for running arbitrary SQL,
for sure.

Best regards,
Alexander

PG Bug reporting form <noreply@postgresql.org> writes:

The function CompareIndexInfo() contains the code:
/* ignore expressions at this stage */
if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
(attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
info1->ii_IndexAttrNumbers[i]))
return false;

where info1->ii_IndexAttrNumbers[i] is checked for InvalidAttrNumber
(i. e. it's not an expression), but info2->ii_IndexAttrNumbers[i] is not.

Agreed, that's pretty broken, and it's not just that it risks an
invalid access. I don't think this reliably rejects cases where
one index has an expression and the other doesn't. Even if it does
work, it's way too complicated to convince oneself that that's
rejected. I think for clarity we should code it as attached.

regards, tom lane

On Thu, Sep 28, 2023 at 01:31:39PM -0400, Tom Lane wrote:

Agreed, that's pretty broken, and it's not just that it risks an
invalid access. I don't think this reliably rejects cases where
one index has an expression and the other doesn't. Even if it does
work, it's way too complicated to convince oneself that that's
rejected. I think for clarity we should code it as attached.

-        /* ignore expressions at this stage */
-        if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
-            (attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
-             info1->ii_IndexAttrNumbers[i]))
-            return false;
+        /* ignore expressions for now (but check their collation/opfamily) */
+        if (!(info1->ii_IndexAttrNumbers[i] == InvalidAttrNumber &&
+              info2->ii_IndexAttrNumbers[i] == InvalidAttrNumber))
+        {
+            /* fail if just one index has an expression in this column */
+            if (info1->ii_IndexAttrNumbers[i] == InvalidAttrNumber ||
+                info2->ii_IndexAttrNumbers[i] == InvalidAttrNumber)
+                return false;

I can see that this has already been committed as 9f71e10d65, but are
you sure that this is correct? I didn't take the time to reply back,
because I got the feeling that even what I proposed is not correct.
The previous code was careful enough to look at the information from
info2 only *through* the attribute map, which is not what this new
code is doing. Rather than looking directly at the elements in info2
at each iteration, shouldn't this stuff loop over the elements in
attmap->attnums for each info1->ii_IndexAttrNumbers[i]?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

I can see that this has already been committed as 9f71e10d65, but are
you sure that this is correct? I didn't take the time to reply back,
because I got the feeling that even what I proposed is not correct.
The previous code was careful enough to look at the information from
info2 only *through* the attribute map, which is not what this new
code is doing. Rather than looking directly at the elements in info2
at each iteration, shouldn't this stuff loop over the elements in
attmap->attnums for each info1->ii_IndexAttrNumbers[i]?

I think it's OK as is. What we are comparing in the modified logic
is whether index columns at the same column positions are expressions or
not, and that's not a matter for mapping. Once we've verified that
the current column is a non-expression in both indexes, then it's
appropriate to use the attmap to see whether the columns correspond.

(Or to put it another way: running "column zero" through the
attmap is always the wrong thing.)

regards, tom lane

On Sat, Sep 30, 2023 at 08:39:37PM -0400, Tom Lane wrote:

I think it's OK as is. What we are comparing in the modified logic
is whether index columns at the same column positions are expressions or
not, and that's not a matter for mapping. Once we've verified that
the current column is a non-expression in both indexes, then it's
appropriate to use the attmap to see whether the columns correspond.

FWIW, I was thinking about a case like that with 2 index attributes:
info1->attr[0] = 0, info1->attr[1] = 1
info2->attr[0] = 1, info2->attr[1] = 0
With a mapping from info2 to info1 like that:
attmap[0] = 1, attmap[1] = 0.

The code before 9f71e10d6 would have accessed an incorrect pointer.
The new code would return false, which would not be correct if the
expressions stored in ii_Expressions for info1/attr0 and info2/attr1
match?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

FWIW, I was thinking about a case like that with 2 index attributes:
info1->attr[0] = 0, info1->attr[1] = 1
info2->attr[0] = 1, info2->attr[1] = 0
With a mapping from info2 to info1 like that:
attmap[0] = 1, attmap[1] = 0.

The code before 9f71e10d6 would have accessed an incorrect pointer.
The new code would return false, which would not be correct if the
expressions stored in ii_Expressions for info1/attr0 and info2/attr1
match?

I think "false" is correct. Otherwise you're claiming that an
index on (expr, col1) is equivalent to an index on (col1, expr).
Maybe it is equivalent for some purposes, but I don't think this
function has any business assuming that it is so for all purposes.

Also, the pre-existing comment clearly intends that false is
the proper result here:

* might differ!) Note that this implies that index columns that are
* expressions appear in the same positions. We will next compare the
* expressions themselves.

The code just failed to enforce that fully.

regards, tom lane