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.

Sign up for GitHub

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

Jump to bottom

nginx: upgrade pcre to pcre2 #355989

Merged
merged 2 commits into from
Nov 19, 2024
Merged

nginx: upgrade pcre to pcre2 #355989

merged 2 commits into from
Nov 19, 2024

Conversation

de11n
Copy link

@de11n de11n commented Nov 14, 2024

Nginx builds with pcre2 by default as of version 1.21.5.

I've tested with both nginx and nginxMainline.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

nginx: upgrade pcre to pcre2
06b9024 
Nginx builds with pcre2 by default as of version 1.21.5.
@nix-owners nix-owners bot requested a review from RaitoBezarius November 14, 2024 20:10
@emilazy
Copy link
Member

emilazy commented Nov 14, 2024 •
edited
Loading

Do we have a tracking issue for this? I learned the other day that Debian is phasing out PCRE because of security concerns, so we should probably follow suit.

@de11n
Copy link
Author

de11n commented Nov 14, 2024

If there is, I'm not aware of it.

@Izorkin
Copy link
Contributor

Izorkin commented Nov 15, 2024

Do the lua modules work?

@emilazy emilazy mentioned this pull request Nov 16, 2024
Open
@emilazy
Copy link
Member

emilazy commented Nov 16, 2024

I’ve opened #356387 to track this.

@emilazy
Copy link
Member

emilazy commented Nov 16, 2024

@Izorkin Debian already started working on removing PCRE in 2021 and the library is EOL. The latest Debian stable release doesn’t ship it at all. I doubt anything significant is broken by this (and anything that is broken by it is probably in a dire maintenance state).

@emilazy
Copy link
Member

emilazy commented Nov 16, 2024

@ofborg build nginx.passthru.tests

@ulrikstrid
Copy link
Member

Should we wait for after the stable release?

@emilazy
Copy link
Member

emilazy commented Nov 16, 2024

I think the opposite – we should backport this to 24.11 because we’re exposing a library that went EOL in 2021 and hasn’t been updated since to untrusted user input. My previous comment was wrong on the timeline: Debian considered it too risky for that purpose at the time, which is why they’ve already removed it. It’s concerning that we’ve fallen so far behind, especially for something as security‐critical as Nginx.

@ulrikstrid
Copy link
Member

Then I think we should go for it

@emilazy emilazy added the backport release-24.11 Backport PR automatically label Nov 17, 2024
@emilazy
Copy link
Member

emilazy commented Nov 17, 2024 •
edited
Loading

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 355989

x86_64-linux

❌ 2 packages failed to build:
  • openresty
  • openresty.doc
✅ 17 packages built:
  • angie
  • angie.doc
  • angieQuic
  • angieQuic.doc
  • devpi-client
  • devpi-client.dist
  • devpi-server
  • devpi-server.dist
  • nginx (nginxStable)
  • nginx.doc (nginxStable.doc)
  • nginxMainline
  • nginxMainline.doc
  • nginxQuic
  • nginxQuic.doc
  • nginxShibboleth
  • nginxShibboleth.doc
  • tests.testers.lycheeLinkCheck.network

Looks like this breaks OpenResty:

./configure: error: the HTTP rewrite module requires the PCRE library.

We can probably backport the upstream fix. (Or just bump to 1.27?)

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Nov 17, 2024
@emilazy
Copy link
Member

emilazy commented Nov 18, 2024

cc @thoughtpolice @lblasc @kalekseev for OpenResty

@emilazy
Copy link
Member

emilazy commented Nov 18, 2024

(Pinging because this would break OpenResty unless we presumably backport a patch from upstream Nginx or perhaps bump the version 😅)

@de11n
Copy link
Author

de11n commented Nov 18, 2024

I'll look today.

@de11n
Copy link
Author

de11n commented Nov 18, 2024

Upgrading openresty to 1.27.1.1 fixed the build. The NixOS tests also pass.

emilazy
emilazy approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! Looks like it should be backwards‐compatible enough for the backport. I’ll let @lblasc take another look before merging.

@lblasc
Copy link
Contributor

lblasc commented Nov 19, 2024

@emilazy sorry, I've reacted too fast on your comment, best way to handle openresty is to bump the version, they don't brake compatibility, should be safe for backporting.

@de11n thx for doing the bump.

@wegank wegank added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 19, 2024
@ofborg ofborg bot requested review from ulrikstrid and lblasc November 19, 2024 15:16
@emilazy emilazy merged commit fc1a0e3 into NixOS:master Nov 19, 2024
22 of 24 checks passed
@github-actions GitHub Actions
Copy link
Contributor

Successfully created backport PR for release-24.11:

@github-actions github-actions bot mentioned this pull request Nov 19, 2024
Merged
1 task
@de11n de11n deleted the upstream/nginx-pcre2 branch November 19, 2024 15:38
@SuperSandro2000
Copy link
Member

With this change my nginx fails to start with bad system call. I confirmed this by deploying it with an override on pcre2 to pcre.

systemd[1]: nginx.service: Control process exited, code=exited, status=159/n/a
nginx-pre-start[4083150]: /nix/store/pjw413fdjq9fmnr718c6y3mai5v1bgng-unit-script-nginx-pre-start/bin/nginx-pre-start: line 7: 4083151 Bad system call         (core
 dumped) /nix/store/gs6s5r211gbd2banxkfxq80j4nmx6jbh-nginxQuic-1.27.2/bin/nginx -c /etc/nginx/nginx.conf -t
systemd[1]: nginx.service: Failed with result 'exit-code'.

dmesg contains:

audit: type=1326 audit(1732833069.740:14): auid=4294967295 uid=60 gid=60 ses=4294967295 subj=kernel pid=4082877 comm="nginx" exe="/nix/store/gs6s5r211gbd2banxkfxq
80j4nmx6jbh-nginxQuic-1.27.2/bin/nginx" sig=31 arch=c000003e syscall=319 compat=0 ip=0x7fc61bf4ddcb code=0x80000000
audit: type=1326 audit(1732833080.876:15): auid=4294967295 uid=60 gid=60 ses=4294967295 subj=kernel pid=4083151 comm="nginx" exe="/nix/store/gs6s5r211gbd2banxkfxq
80j4nmx6jbh-nginxQuic-1.27.2/bin/nginx" sig=31 arch=c000003e syscall=319 compat=0 ip=0x7f7a0ba68dcb code=0x80000000

syscall 319 maps to memfd_create.

It feels similar to #179444 but neither disabling MemoryDenyWriteExecute nor allowing memfd_create fixed the crash.

SuperSandro2000 added a commit to SuperSandro2000/nixpkgs that referenced this pull request Nov 28, 2024
@SuperSandro2000
nixos/nginx: don't disable IPC
0774a29 
This also disables the memfd_create syscall which is required for
certain regex's when using pcre2.

see NixOS#355989 (comment)
SuperSandro2000 added a commit to SuperSandro2000/nixpkgs that referenced this pull request Nov 28, 2024
@SuperSandro2000
nixos/nginx: don't disable IPC
996f9e4 
This also disables the memfd_create syscall which is required for
certain regex's when using pcre2.

see NixOS#355989 (comment)
@SuperSandro2000 SuperSandro2000 mentioned this pull request Nov 28, 2024
Merged
13 tasks
@SuperSandro2000
Copy link
Member

I did a fix in #360008

fpletz added a commit to fpletz/nixpkgs that referenced this pull request Nov 29, 2024
@fpletz
Revert "nginx: upgrade pcre to pcre2"
6625e52 
This reverts commit 861b05c.

See NixOS#355989 & NixOS#360008. This needs more testing and polish.
@fpletz fpletz mentioned this pull request Nov 29, 2024
Closed
SuperSandro2000 added a commit to SuperSandro2000/nixpkgs that referenced this pull request Nov 30, 2024
@SuperSandro2000
nixos/nginx: don't disable IPC
f48b911 
This also disables the memfd_create syscall which is required for
certain regex's when using pcre2.

see NixOS#355989 (comment)
SuperSandro2000 added a commit to SuperSandro2000/nixpkgs that referenced this pull request Dec 3, 2024
@SuperSandro2000
nixos/nginx: don't disable IPC
dfae458 
This also disables the memfd_create syscall which is required for
certain regex's when using pcre2.

see NixOS#355989 (comment)
SuperSandro2000 added a commit to SuperSandro2000/nixpkgs that referenced this pull request Dec 6, 2024
@SuperSandro2000
nixos/nginx: don't disable IPC
68598bf 
This also disables the memfd_create syscall which is required for
certain regex's when using pcre2.

see NixOS#355989 (comment)
nixpkgs-ci bot pushed a commit that referenced this pull request Dec 9, 2024
@SuperSandro2000 @github-actions
nixos/nginx: don't disable IPC
9e66870 
This also disables the memfd_create syscall which is required for
certain regex's when using pcre2.

see #355989 (comment)

(cherry picked from commit 996f9e4)
SuperSandro2000 added a commit to SuperSandro2000/nixpkgs that referenced this pull request Dec 9, 2024
@SuperSandro2000
nixos/nginx: don't disable IPC
65d5026 
This also disables the memfd_create syscall which is required for
certain regex's when using pcre2.

see NixOS#355989 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilazy emilazy emilazy approved these changes

@RaitoBezarius RaitoBezarius Awaiting requested review from RaitoBezarius

@Conni2461 Conni2461 Awaiting requested review from Conni2461

@fpletz fpletz Awaiting requested review from fpletz

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

@dasJ dasJ Awaiting requested review from dasJ

@Izorkin Izorkin Awaiting requested review from Izorkin

@helsinki-Jo helsinki-Jo Awaiting requested review from helsinki-Jo

@ulrikstrid ulrikstrid Awaiting requested review from ulrikstrid

@lblasc lblasc Awaiting requested review from lblasc

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package backport release-24.11 Backport PR automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@de11n @emilazy @Izorkin @ulrikstrid @lblasc @SuperSandro2000 @wegank