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

bitcoin-core: Enable libc++ "Debug" hardening mode #11725

Merged
merged 1 commit into from
May 22, 2024
Merged

bitcoin-core: Enable libc++ "Debug" hardening mode #11725

merged 1 commit into from
May 22, 2024

Conversation

hebasto
Copy link
Contributor

@hebasto hebasto commented Mar 24, 2024 •
edited
Loading

This PR enables libc++ "Debug" hardening mode.

See #11725 (comment).

@github-actions GitHub Actions
Copy link

hebasto is a new contributor to projects/bitcoin-core. The PR must be approved by known contributors before it can be merged. The past contributors are: fanquake, guidovranken, dergoegge, maflcko, achow101

@hebasto
Copy link
Contributor Author

hebasto commented Mar 24, 2024

cc @maflcko @fanquake

@maflcko
Copy link
Contributor

maflcko commented Mar 27, 2024

Seems fine, looking at the CI output.

Previously: https://github.com/google/oss-fuzz/actions/runs/7656996295/job/20866632302#step:7:9774
This pull: https://github.com/google/oss-fuzz/actions/runs/8411616467/job/23084431048#step:7:9734

@maflcko
Copy link
Contributor

maflcko commented Mar 27, 2024

@fanquake Any objections?

@fanquake
Copy link
Contributor

Seems fine as long as we still have LTO. Although not sure if the flags will make any difference here? given builds are with libc++, and the LLVM debug mode wont work as-is as far as I'm aware (It's also the legacy mode).

@maflcko
Copy link
Contributor

maflcko commented Mar 27, 2024

Right, D_LIBCPP_ENABLE_DEBUG_MODE should be a no-op in libc++-14-trunk. Though, I am working on bumping it to 18-trunk in oss-fuzz, so it will hopefully take effect then.

DavidKorczynski
DavidKorczynski reviewed Apr 1, 2024
View reviewed changes
Copy link
Collaborator

@DavidKorczynski DavidKorczynski left a comment

Choose a reason for hiding this comment

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

Based on the comments in the PR I'm not entirely sure if the Bitcoin maintainers would like to see this merged? @maflcko

@maflcko
Copy link
Contributor

maflcko commented Apr 1, 2024

I think it is fine, but let's wait for clang-18-trunk, so that it can be tested :)

@fanquake fanquake mentioned this pull request Apr 1, 2024
Merged
@fanquake
Copy link
Contributor

fanquake commented Apr 1, 2024

I think it is fine, but let's wait for clang-18-trunk, so that it can be tested :)

If Clang is bumped to 18 then bitcoin/bitcoin#29781 will also be needed, as the current macro will still be a no-op.

@maflcko
Copy link
Contributor

maflcko commented Apr 2, 2024

Cross-ref to https://github.com/google/oss-fuzz/pull/10166/files

fanquake added a commit to bitcoin/bitcoin that referenced this pull request Apr 9, 2024
@fanquake
Merge #29781: depends: add new LLVM debug macro
9dfe435 
5efebc0 depends: add the new LLVM debug macro (fanquake)

Pull request description:

  `LIBCXX_HARDENING_MODE` is the new macro, the previous one was removed in LLVM 18.

  See https://libcxx.llvm.org/Hardening.html.

  Required before google/oss-fuzz#11725 will do anything (with the bump to 18.x).

  Seems reasonable to do now that almost all our test infra is using LLVM 18.

ACKs for top commit:
  theuni:
    ACK 5efebc0

Tree-SHA512: 43078eeb5940c55ef4f95c72682f8a372dcd3eb97956b3114149c16d9f59b067a999b2aab7f34ffb57eab191524514408e2bba154ff4a6ea0cd6ec4d119c5d18
@fanquake fanquake mentioned this pull request Apr 10, 2024
Closed
@DonggeLiu
Copy link
Contributor

Thanks @hebasto and @maflcko,
I will convert this to a draft for now until it is verified : )

@DonggeLiu DonggeLiu marked this pull request as draft April 18, 2024 02:10
@maflcko
Copy link
Contributor

maflcko commented Apr 30, 2024

This should be good to go now. Please merge or rebase with master.

@hebasto
Copy link
Contributor Author

hebasto commented Apr 30, 2024

This should be good to go now. Please merge or rebase with master.

Rebased.

@hebasto hebasto marked this pull request as ready for review April 30, 2024 09:11
maflcko
maflcko approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM as a temporary workaround, if CI passes.

Future cleanup can be done later, if needed.

@fanquake
Copy link
Contributor

The added comment says we aren't using CPPFLAGS, but then a few lines down we do use it.

Given we established the other flags mentioned in your PR description don't actually matter (could also be re-written now that it's outdated), why not just put -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG into CPPFLAGS, and leave everything else untouched?

@hebasto hebasto force-pushed the 240324-cppflags branch from 09f783a to 89d732b Compare May 4, 2024 09:56
@hebasto hebasto changed the title bitcoin-core: Re-enable linux_debug_CPPFLAGS in depends bitcoin-core: Enable libc++ "Debug" hardening mode May 4, 2024
@hebasto
Copy link
Contributor Author

hebasto commented May 4, 2024

The added comment says we aren't using CPPFLAGS, but then a few lines down we do use it.

Given we established the other flags mentioned in your PR description don't actually matter (could also be re-written now that it's outdated), why not just put -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG into CPPFLAGS, and leave everything else untouched?

Thanks! Reworked.

maflcko
maflcko approved these changes May 4, 2024
View reviewed changes
Copy link
Contributor

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm

@maflcko
Copy link
Contributor

maflcko commented May 6, 2024

@DavidKorczynski this is ready for merge

@maflcko
Copy link
Contributor

maflcko commented May 22, 2024

cc @DavidKorczynski . Anything left to do here for this project patch?

@DavidKorczynski
Copy link
Collaborator

Apologies for the delay! Will land this once the CI is green!

@DavidKorczynski DavidKorczynski enabled auto-merge (squash) May 22, 2024 09:55
@DavidKorczynski DavidKorczynski merged commit 8e0628e into google:master May 22, 2024
15 checks passed
@hebasto hebasto deleted the 240324-cppflags branch May 22, 2024 10:59
@hebasto hebasto mentioned this pull request May 22, 2024
Closed
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 25, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#29781: depends: add new LLVM debug macro
2e44a8e 
5efebc0 depends: add the new LLVM debug macro (fanquake)

Pull request description:

  `LIBCXX_HARDENING_MODE` is the new macro, the previous one was removed in LLVM 18.

  See https://libcxx.llvm.org/Hardening.html.

  Required before google/oss-fuzz#11725 will do anything (with the bump to 18.x).

  Seems reasonable to do now that almost all our test infra is using LLVM 18.

ACKs for top commit:
  theuni:
    ACK 5efebc0

Tree-SHA512: 43078eeb5940c55ef4f95c72682f8a372dcd3eb97956b3114149c16d9f59b067a999b2aab7f34ffb57eab191524514408e2bba154ff4a6ea0cd6ec4d119c5d18
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 26, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#29781: depends: add new LLVM debug macro
81ca71c 
5efebc0 depends: add the new LLVM debug macro (fanquake)

Pull request description:

  `LIBCXX_HARDENING_MODE` is the new macro, the previous one was removed in LLVM 18.

  See https://libcxx.llvm.org/Hardening.html.

  Required before google/oss-fuzz#11725 will do anything (with the bump to 18.x).

  Seems reasonable to do now that almost all our test infra is using LLVM 18.

ACKs for top commit:
  theuni:
    ACK 5efebc0

Tree-SHA512: 43078eeb5940c55ef4f95c72682f8a372dcd3eb97956b3114149c16d9f59b067a999b2aab7f34ffb57eab191524514408e2bba154ff4a6ea0cd6ec4d119c5d18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidKorczynski DavidKorczynski DavidKorczynski approved these changes

@fanquake fanquake fanquake approved these changes

@maflcko maflcko maflcko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @maflcko @fanquake @DonggeLiu @DavidKorczynski