glibc: 2.37-39 -> 2.38-23

[Ma27]

Description of changes

Even though I managed to get quite far with it already, a Hydra jobset would be helpful to find out if there are notable breakages in any subsystem to be fixed (cc @vcunat).

---

Announcement: https://sourceware.org/pipermail/libc-alpha/2023-July/150524.html

So far this looks surprisingly good, I managed to build the stdenv on aarch64-linux and got up to building zfs and nix on x86_64-linux.

The patchset is still empty because the latest commit on the release branch is the one the 2.38 tag points to. I added an empty file though to keep things consistent.

Also applied the new version of the DT_HASH fix from ArchLinux[1]. This one's a way easier version than before because it doesn't contain the autoconf changes, but only hardcodes the desired ld flags. It was already confirmed that this patch is sufficient to fix the underlying problem[2].

[1] https://gitlab.archlinux.org/archlinux/packaging/packages/glibc/-/commit/e54d98e2d1aae4930ecad9404ef12234922d9dfd#7b1bfda0391ff4c2662e04a5e193c37e233a0738
[2] ValveSoftware/Proton#6051 (comment)

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 23.11 Release Notes (or backporting 23.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.

[vcunat]

Fortification: I stumbled on this in the NEWS

• A new configure option, "--enable-fortify-source", can be used to build the
  GNU C Library with _FORTIFY_SOURCE. The level of fortification can either be
  provided, or is set to the highest value supported by the compiler. If not
  explicitly enabled, then fortify source is forcibly disabled so to keep
  original behavior unchanged.

So until this PR we mistakenly built glibc without fortification, if I look right?

    # The pie, stackprotector and fortify hardening flags are autodetected by
    # glibc and enabled by default if supported. Setting it for every gcc
    # invocation does not work.
    hardeningDisable = [ "fortify" "pie" "stackprotector" ];

/cc @risicle as fortification proponent :-)

[vcunat]

CVE-2023-25139: I re-checked that this had been fixed in glibc-2.37-4 and we had -8 already on 23.05 and later.

[vcunat]

• I'll create a single-platform jobset, once Hydra frees up a bit. (I expect that will happen within a few days.)

[Ma27]

CVE-2023-25139: I re-checked that this had been fixed in glibc-2.37-4 and we had -8 already on 23.05 and later.

Yep, also checked yesterday that this is already applied. Got a bit of a shock before because I thought I've missed a critical rated CVE in glibc for half a year first ;-)

So until this PR we mistakenly built glibc without fortification, if I look right?

So upstream commit 64d9580cdf7e417170abbef0327e04b29712e949 basically adds required compiler flags into fortify-source (from config.make.in) and then it's decided whether or not to add these to the cflags when building a given subdir (because certain early components cannot be fortified). Then, there are a few follow-up commits that fix certain subsystems to work with fortify_source (e.g. f8f9a272573a4074c5b13ec69522945695d5d3f2).

E.g. nscd was already built with FORTIFY_SOURCE, but only because it used to be hard-coded (upstream commit 5b61880ba3a0367f8969e028cb2cfe80d6eda8ab).

So unless I missed something, you're right @vcunat.

Because of the fixups following the introduction of --enable-fortify-source, I'm somewhat hesitant to actually add this to the existing 2.37 on 23.05.

cc @fpletz who originally added this note 3ba99f8. I know that you did this is quite a few years back, but any chance you still remember the details? AFAIU the autodetection in this commit only affects stackprotector, but I may be wrong.

cc @amjoseph-nixpkgs @trofi just in case you have time, would you mind looking at this? Perhaps I missed somethign and you seem to be more experienced with libc %)

[vcunat]

I wouldn't change anything in 23.05, except possibly update within the 2.37 branch (but I haven't noticed anything important really in there).

[vcunat]

I just rebased the commit to staging-next, so that we have a more certain comparison base and created https://hydra.nixos.org/jobset/nixpkgs/pr-247401-glibc-2.38 (x86_64-linux only, for now)

[Ma27]

@vcunat thanks! Incidentally I started fixing up a bit of stuff yesterday as well and rebased once again, but got distracted while looking at rapidjson ;-) . Anyways, checked that your commit is still fine with the newly rebased state and force-pushed.

Shall we await another Hydra evaluation with x86_64-linux only or already build for aarch64-linux already? (Haven't started an eval so far).

[vcunat]

Was the rebase causing a mass rebuild?

[Ma27]

Yes.

[vcunat]

I expect that with your fixes x86_64-linux should be mostly covered, so I triggered aarch64-linux only this time. Most regressions will be shared by the two, I think. And it's not like we need to fix them all, especially not before merging to staging.

[Ma27]

Rebased onto latest staging. We may want to await another Hydra run, but the last jobset eval (1527 failing out of 114243 builds) looked pretty promising, so we could consider merging this later on.

[vcunat]

Sounds like OK to just merge and leave the rest to staging-next cycle.

[vcunat]

By that we'd also skip #256887 – or at least I hope we already have those security fixes in here as well (TODO: check before merging)

[vcunat]

Oh, you merged it a while ago :-)

[Ma27]

Oh, you merged it a while ago :-)

Yeah, I didn't expect such a fast decision and if we would've awaited at least another Hydra round we'd probably had to delay this for at least another staging-next cycle, so I figured having these fixes in master/unstable a little quicker is a good thing %)

TODO: check before merging

I checked, they are.

[Ma27]

Added the change to the release notes for 23.11 since I'm rather certain that we'll get this ready on time this round.

@vcunat I'm fine with both awaiting another Hydra round or merging this right away because of how few issues this release seems to have with our packages, so I'd leave this up to you :)

[vcunat]

staging-next is mainly waiting because of insufficient aarch64-linux builders, so I don't think we can afford to do those builds before merging. But x86 were relatively free now so I triggered an eval with that. Maybe this will catch the next staging-next anyway, "thanks" to the delays.

[vcunat]

The pkgsi686Linux.libarchive regression seems reproducible and will be relevant for wine-related packages. (probably not a blocker to merge this PR though; I don't have time right now to look deeper)

[vcunat]

Ah, that issue was unrelated and resolved by PR #257080. Pulling not yet discovered unrelated regressions is the main down-side of basing on staging.

[Ma27]

Ah good to know, was about to test the base of staging because I was surprised that my last push caused the regression %)
In this case it was necessary to avoid the merge-conflict with the glibc 2.37 bump unfortunately.

Anyways, the jobset eval keeps looking good. Do you want me to take a look at another regression we should fix before this lands ins taging?

[vcunat]

The closest comparison I found and used was
https://hydra.nixos.org/eval/1799570?compare=1799561

I believe this is good to include in the upcoming staging-next iteration. Fixes are welcome independently of merging this (ideally before it hits master). From my glancing at the list, I think swift might be good to fix.

[Ma27]

@vcunat please note that swift is not entirely fixed by this. I'm not entirely sure what exactly is the problem, so bear with me for a moment.

[vcunat]

Ah, OK. Anyway, there will most likely be at least a week before this can get into nixpkgs master, so there will be time. We might ping maintainers of regressed packages, too.

[Ma27]

So the problem is that swift-tools-support-core fails like this:

swift-tools-support-core> /build/swift-tools-support-core-ac4871e/Sources/TSCBasic/FileSystem.swift:428:24: error: value of optional type 'UnsafeMutablePointer<FILE>?' (aka 'Optional<UnsafeMutablePointer<_IO_FILE>>') must be unwrapped to a value of type 'UnsafeMutablePointer<FILE>' (aka 'UnsafeMutablePointer<_IO_FILE>')
swift-tools-support-core>         defer { fclose(fp) }
swift-tools-support-core>                        ^

I'm not entirely sure how TSClibc provides access to libc functions like fclose/fopen, but my theory is that 71d9e0fe766a3c22a730995b9d024960970670af (libio: Add __nonnull for FILE * arguments of fclose and freopen) from glibc is the culprit.

When starting a swift repl from master, I also get UnsafeMutablePointer<FILE>? back from fopen (when importing Glibc) and fclose works because it accepts this type. However, according to the error this is no longer the case anymore. So it seems as if the __nonnull is related (this would be an explanation why this regression could be observed after the glibc bump only). However as I said, I don't know enough about Swift internals to know how access to libc is implemented in there.

Also, I didn't even know that Swift is that much of a thing on Linux.
Anyways cc @stephank @toonn @figsoda @reckenrode who most recently worked on swift.

[reckenrode]

I'm not entirely sure how TSClibc provides access to libc functions like fclose/fopen, but my theory is that 71d9e0fe766a3c22a730995b9d024960970670af (libio: Add __nonnull for FILE * arguments of fclose and freopen) from glibc is the culprit.

That’s probably it. When Swift imports fclose with the new glibc, the nullability annotation is causing it to be mapped over to UnsafeMutablePointer<FILE> instead of UnsafeMutablePointer<FILE>?.

When starting a swift repl from master, I also get UnsafeMutablePointer<FILE>? back from fopen (when importing Glibc) and fclose works because it accepts this type. However, according to the error this is no longer the case anymore. So it seems as if the __nonnull is related (this would be an explanation why this regression could be observed after the glibc bump only). However as I said, I don't know enough about Swift internals to know how access to libc is implemented in there.

Swift can import C headers. Types will be mapped over to Swift types, and functions will have a C calling convention.

https://developer.apple.com/documentation/swift/imported-c-and-objective-c-apis

Also, I didn't even know that Swift is that much of a thing on Linux. Anyways cc @stephank @toonn @figsoda @reckenrode who most recently worked on swift.

It technically is, but the situation isn’t great. There are portability issues between Linux and Darwin due to different Foundation implementations. (There’s a rewrite that should help, but it’s not ready yet.)

[Ma27]

Swift can import C headers.

That's the part I was missing. If that happens, then I'm pretty sure we've found the culprit already. That said, given that it's UB to do fclose(NULL) apparently, that's actually a fairly sensible decision IMHO.

I skimmed over the bugreports for Swift already and I haven't found a bugreport so far. The problem is that I've seen quite a bunch of codepaths doing essentially let fp = fopen(...); defer { fclose(fp); } and all of that requires fixing now.

Given no bugreports known to me, would you mind filing one against swift describing said problem? If you'd need assistance, let me know! If you want to test things first, you'll probably want to use e016224 as a base since Hydra has built quite a lot for x86_64-linux at least.

It technically is, but the situation isn’t great. There are portability issues between Linux and Darwin due to different Foundation implementations. (There’s a rewrite that should help, but it’s not ready yet.)

To me this sounds like this alone doesn't qualify for a full revert on staging then, though.

[reckenrode]

Given no bugreports known to me, would you mind filing one against swift describing said problem? If you'd need assistance, let me know! If you want to test things first, you'll probably want to use e016224 as a base since Hydra has built quite a lot for x86_64-linux at least.

I think I have a patch, but I won’t be able to test it until this evening. They check the return value of fopen for nil, which can be trivially replaced by guard let to conditionally unwrap the optional.

It technically is, but the situation isn’t great. There are portability issues between Linux and Darwin due to different Foundation implementations. (There’s a rewrite that should help, but it’s not ready yet.)

To me this sounds like this alone doesn't qualify for a full revert on staging then, though.

I wouldn’t revert the glibc update just on account of Swift.

[Ma27]

I think I have a patch, but I won’t be able to test it until this evening.

No worries, it's not super time-pressing, the glibc update landed yesterday in staging, so until it'll hit master, it'll take a week at least I think.

They check the return value of fopen for nil, which can be trivially replaced by guard let to conditionally unwrap the optional.

Yeah I wasn't sure what the easiest fix would be (I've heard that Swift seems to be a decent language, but I've never tried it out so far). Would be cool if you could submit this to upstream as well then :)

[vcunat]

staging-next is the branch right now: #257792

[trofi]

blobwars will need a fix for strlcpy/strlcat collision. Proposed as: #260238