Subject: Re: pgsql: Don't trust unvalidated xl_tot_len.

From: Christoph Berg <[email-masked]>
Date: Sat, 11 Nov 2023 12:53:37 +0100

Re: Thomas Munro
> In the 13 branch we see that's in the new scan_server_header()
> subroutine where it tries to open the header, after asking pg_config
> --includedir-server where it lives.  Hmm...

It's no ok to use pg_config at test time since `make install` might
not have been called yet:

https://www.postgresql.org/message-id/[email-masked]
https://www.postgresql.org/message-id/YqkV/[email-masked]

Christoph

---

From: Thomas Munro <[email-masked]>
Date: Sun, 12 Nov 2023 07:17:02 +1300

> https://www.postgresql.org/message-id/YqkV/[email-masked]

[CC'ing Michael who was involved in that analysis and who also wrote
those bits of this commit]

We don't have an installation into the final --prefix, but we have
tmp_install, surely?  And the tests are run with PATH set to point to
tmp_install's bin directory.  It looks like it did actually find a
pg_config executable because otherwise we'd have hit die "could not
execute pg_config" and failed sooner.  So now I'm wondering if the
pg_config it found gives the wrong answer for --includedir-server,
because of Debian's local patches that insert a major version into
some paths.  For example, maybe it's trying to look for
access/xlog_internal.h under tmp_install/usr/include/postgresql/server
when it's really under tmp_install/usr/include/postgresql/13/server,
or vice versa.  But then why does that only happen on the salsa build,
not on the apt.postgresql.org one?

---

From: Christoph Berg <[email-masked]>
Date: Sat, 11 Nov 2023 19:26:59 +0100

Re: Thomas Munro
> But then why does that only happen on the salsa build,

The build chroots there have postgresql-NN already installed so
extension builds don't have to download 7 PG versions over and over.
My guess would be that that's the difference and it's using some
pg_config from /usr/bin or /usr/lib/postgresql/*/bin.

I can confirm that it's also failing in my local chroots if none of
the postgresql-* packages are preinstalled.

Christoph

---

From: Andres Freund <[email-masked]>
Date: Sat, 11 Nov 2023 11:43:25 -0800

Hi,

On 2023-11-12 07:17:02 +1300, Thomas Munro wrote:
> tmp_install, surely?

Yea, that should work and does work locally.  I guess it'd fail though, if you
somehow ran the tests with NO_TEMP_INSTALL=1 or such.

> And the tests are run with PATH set to point to
> some paths.

>From the second link above, the problem might more be that debian pg_config is
patched to not be relocatable (huh?) - so it'd return an absolute path into
the final non-DESTDIR path. Which would fail, because the file isn't installed
yet.

If that's the case, does that also mean that all the tests that are
selectively enabled using check_pg_config() don't work in the debian context?

I assume the reason to not use the source-tree access/xlog_internal.h here was
just that it's nontrivial to find the top of the source tree form tap tests?
It's not hard to make it available... And as we already pass in top_builddir,
it doesn't actually measurably further weaken usability of the tap framework
in the pgxs context.

> For example, maybe it's trying to look for access/xlog_internal.h under
> tmp_install/usr/include/postgresql/server when it's really under
> tmp_install/usr/include/postgresql/13/server, or vice versa.  But then why
> does that only happen on the salsa build, not on the apt.postgresql.org one?

Christoph, can you apply a patch to emit a bit more information in that
environment?

diff --git i/src/test/perl/PostgreSQL/Test/Utils.pm w/src/test/perl/PostgreSQL/Test/Utils.pm
index cd86897580c..3c588a41755 100644
--- i/src/test/perl/PostgreSQL/Test/Utils.pm
+++ w/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -722,7 +722,8 @@ sub scan_server_header
     chomp($stdout);
     $stdout =~ s/\r$//;
 
-    open my $header_h, '<', "$stdout/$header_path" or die "$!";
+    my $fname = "$stdout/$header_path";
+    open my $header_h, '<', "$fname" or die "could not open $fname: $!";
 
     my @match = undef;
     while (<$header_h>)

would be helpful.

I guess we should also apply something like that to our tree - printing a
stringified errno without even a path/filename isn't very useful.

Greetings,

Andres Freund