Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

postgresql: security updates #27691

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

dgilman
Copy link
Contributor

@dgilman dgilman commented Feb 17, 2025

Description

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 14.6.1 23G93 x86_64
Xcode 16.2 16C5032a

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL in commit message?
  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@jyrkiwahlstedt for port postgresql13-doc, postgresql13-server, postgresql14-doc, postgresql14-server, postgresql15-doc, postgresql15-server.
@barracuda156 for port postgresql13, postgresql14, postgresql15, postgresql16-server, postgresql16, postgresql17-server, postgresql17.

@barracuda156
Copy link
Contributor

barracuda156 commented Feb 17, 2025

@dgilman postgresql16 and postgresql17 need to depend on MacPorts perl5, since they require at least 5.14, while 10.6, for example, has 5.10. I am not sure about 10.7.

P. S. It seems from variant existence that perl is optional, but it is not, or otherwise argument is wrong and has no effect.
Merely activating perl5 port works, no special arguments or env variables needed. I.e. depends_build should suffice.

@dgilman
Copy link
Contributor Author

dgilman commented Feb 17, 2025

I see that postgresql17 gained a requirement on perl 5.14 but don't see the same comment in postgresql16's docs https://www.postgresql.org/docs/16/install-requirements.html

@barracuda156
Copy link
Contributor

I see that postgresql17 gained a requirement on perl 5.14 but don't see the same comment in postgresql16's docs https://www.postgresql.org/docs/16/install-requirements.html

@dgilman As a matter of fact, it failed with the system perl 5.10. Possibly requirement is lower than 5.14, but if it is not higher than 5.10, then it is a bug, and it should be.
(Of course, there is no need to add a dependency on perl5 across the board, only old systems need it.)

@dgilman
Copy link
Contributor Author

dgilman commented Feb 17, 2025

postgresql16 has been in MacPorts for ~1.5 years, are you saying you see a regression with the 16.6 -> 16.7 change, or it has never worked on old macs?

@barracuda156
Copy link
Contributor

@dgilman I have no idea when this appeared, but configure script of 16.7 has this:

if test "$PERL"; then
  pgac_perl_version=`$PERL -v 2>/dev/null | sed -n 's/This is perl.*v[a-z ]*\([0-9]\.[0-9][0-9.]*\).*$/\1/p'`
  { $as_echo "$as_me:${as_lineno-$LINENO}: using perl $pgac_perl_version" >&5
$as_echo "$as_me: using perl $pgac_perl_version" >&6;}
  if echo "$pgac_perl_version" | sed 's/[.a-z_]/ /g' | \
    $AWK '{ if ($1 == 5 && ($2 >= 14)) exit 1; else exit 0;}'
  then
    { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING:
*** The installed version of Perl, $PERL, is too old to use with PostgreSQL.
*** Perl version 5.14 or later is required, but this is $pgac_perl_version." >&5
$as_echo "$as_me: WARNING:
*** The installed version of Perl, $PERL, is too old to use with PostgreSQL.
*** Perl version 5.14 or later is required, but this is $pgac_perl_version." >&2;}
    PERL=""
  fi
fi

@dgilman
Copy link
Contributor Author

dgilman commented Feb 17, 2025

I see that, and I opened the bug for the docs in upstream.

Honestly, if they're going to start requiring perl for even tarball builds, I'm not opposed to putting the dependency on perl for all versions of macOS. Perl 5.14 came out in 2011 so 10.7, possibly 10.8 needs it. But for everyone else, might as well use a still supported perl?

@barracuda156
Copy link
Contributor

Pre-16 versions build normally with system perl 5.10, perhaps on newer systems 16+ will also build with system perl (provided it is at least 5.14). At the same time, it is hard to imagine MacPorts installation without default perl, so perhaps overhead is negligible even if a dependency is added across the board.

@dgilman
Copy link
Contributor Author

dgilman commented Feb 17, 2025

Looking at the configure script you quoted, it only prints a warning and nulls out the PERL variable. The part below it (not quoted) prints out a further warning if PERL is empty. The build does fail if $with_perl is set and PERL is empty, but MacPorts would only hit that if you're building the +perl variant. But if you were doing a +perl build it would have brought in MacPorts perl as a dependency already. So where exactly is postgresql16 failing with old perl?

@barracuda156
Copy link
Contributor

@dgilman I am away from the hardware, so can’t check. This is supposed to be reproducible on x86, or if it is not by some miracle, no need to bother fixing it. Maybe perl is enabled by default?

@dgilman
Copy link
Contributor Author

dgilman commented Feb 17, 2025

Can you confirm exactly versions of postgresql you test built on what versions of macos?

Maybe it is reproducible on x86 but that doesn't matter, I don't have any old hardware. I am reliant on you and your feedback to make fixes for old systems. You said that postgresql16 doesn't build with old perl, can you provide more details? I had opened a ticket to upstream to address this issue based on your original feedback and now we need to sort out what's going on.

@barracuda156
Copy link
Contributor

@dgilman I was building your updates in a row starting from 13 up, doing a clean build (in a sense of deactivating all ports first), and 13–15 built normally, 16–17 failed with ”no perl found”. The failure looked similar or identical, and since it was straightforward, I did not dig into logs further. It is possible that only one failure explicitly mentioned 5.14 requirement, but both failed because of not finding a satisfactory perl. Once I activated perl5 port, both versions built normally. It was on 10.6.8 (ppc irrelevant, since perl version is identical with x86).

@dgilman
Copy link
Contributor Author

dgilman commented Mar 2, 2025

When you have access to the build machines again can you provide the error from the logs? The current situation here is:

  • I opened a ticket for this issue upstream, upstream says what you are seeing here is impossible
  • If you can provide error logs I can push back on upstream

If upstream is right this PR is good as-is, if barracuda156 is right it needs the fix in MacPorts and possibly upstream

@barracuda156
Copy link
Contributor

upstream says what you are seeing here is impossible

Well, I do not really have any reason to fabricate a failure where there is none, and can’t see any alternative reason for it to happen with specific versions, but if somebody tests the build from scratch (with all ports deactivated) on 10.5 or 10.6 on Intel, that should presumably reproduce the failure (10.5 has 5.8.8 and 10.6 has 5.10, I think).

Architecture cannot matter here as such; perhaps the only meaningful difference between my setup and what MacPorts has on 10.6 is gcc vs clang, which, hopefully, has no impact on perl detection. However, if on Intel perl5 pops up in dependency tree for postgresql17, that would obscure the bug, since MacPorts gonna use its own perl instead of the system one.

P. S. To be clear, does upstream think that both postgresql16 and postgresql17 should build fine with perl < 5.14 or only postgresql16 should but not postgresql17?

@barracuda156
Copy link
Contributor

and I opened the bug for the docs in upstream.

@dgilman Could you tag me there, please, or share a link?

@barracuda156
Copy link
Contributor

By the way, there is no need to delay a security update for the reason discussed. I would suggest adding a dependency on perl5 only for old systems, like it is done for openssl* ports (but including 10.6 in this case), which can be later dropped, if there is a better solution; but if there is an opposition to that, a fix can wait until someone complains about the breakage.

@dgilman
Copy link
Contributor Author

dgilman commented Mar 2, 2025

https://www.postgresql.org/message-id/5a141995-b3b0-45d5-9296-67239eb98e46%40dunslane.net

@barracuda156
Copy link
Contributor

Perl 5.14 or later is needed to build from a Git checkout, or if you changed the input files for any of the build steps that use Perl scripts.

@dgilman If I read this correctly, this may mean that once sources are patched, perl becomes required, and there are patches applied:

See:

platform darwin powerpc {
patchfiles-append patch-icu.diff \
patch-fix-ppc-asm.diff
}

@barracuda156
Copy link
Contributor

@dgilman On a side note, do you think the upstream will consider fixing back assembler code which was broken earlier? I.e. https://github.com/macports/macports-ports/blob/40bf6c7a44debbc0fea47c8cda5c44fc06be924d/databases/postgresql16/files/patch-fix-ppc-asm.diff

@dgilman
Copy link
Contributor Author

dgilman commented Mar 2, 2025

And that goes back to one of my earliest comments. pg16 has been in macports for 1.5 years, this latest release didn't change the rebuild behavior, did it never work on these old systems?

Per the one email discussion above, this postgresql commit was the one that added the requirement on new perl for postgresql 17+. The change is to not include certain files in the tarball and instead rely on Perl to generate them. The MacPorts patches aren't modifying any of those computed files so I don't know why they would be triggering the Perl stuff at compile time.

My issue here is that none of this is adding up. What upstream is telling me, what the code is telling me, and what the history in MacPorts is telling me is that MacPorts didn't need new perl on these old systems for pg16 and before. Maybe my understanding of the build process is wrong here, but I'd appreciate logs or an explanation to point out what I'm missing.

@barracuda156
Copy link
Contributor

barracuda156 commented Mar 2, 2025

pg16 has been in macports for 1.5 years, this latest release didn't change the rebuild behavior, did it never work on these old systems?

@dgilman The explanation why I never got the failure is trivial: I never deactivated all ports to build something until recent, when I started building ports which can be installable on other systems than my own. So I always had perl5 active.

P. S. I will share logs once I have access to my hardware, which is expected to be in about a week. I only have Apple Silicon laptop now, so even 10.6.8 Rosetta won’t work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants