Skip to content

Fix if that should have been an ifdef#129330

Merged
vcsjones merged 2 commits into
dotnet:mainfrom
vcsjones:fix-ossl-ifdef
Jun 12, 2026
Merged

Fix if that should have been an ifdef#129330
vcsjones merged 2 commits into
dotnet:mainfrom
vcsjones:fix-ossl-ifdef

Conversation

@vcsjones

Copy link
Copy Markdown
Member

Fixes #129329

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts preprocessor guarding in the native crypto OpenSSL interop layer to avoid referencing an undefined FEATURE_DISTRO_AGNOSTIC_SSL macro during compilation (and also removes a stray whitespace-only line).

Changes:

  • Replace #if FEATURE_DISTRO_AGNOSTIC_SSL with #ifdef FEATURE_DISTRO_AGNOSTIC_SSL in one OpenSSL 1.1 fallback gate.
  • Remove trailing whitespace in CryptoNative_EvpPKeyEcHasExplicitEncoding.
Show a summary per file
File Description
src/native/libs/System.Security.Cryptography.Native/pal_ecc_import_export.c Updates preprocessor usage around distro-agnostic OpenSSL feature gating to avoid undefined-macro build failures.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

@vcsjones

Copy link
Copy Markdown
Member Author

@vcsjones-bot test 4043bd8 with OpenSSL_1_1_1w
@vcsjones-bot test 4043bd8 with OpenSSL_1_1_1w, nonportable

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks

@vcsjones vcsjones enabled auto-merge (squash) June 12, 2026 16:01
@am11

am11 commented Jun 12, 2026

Copy link
Copy Markdown
Member

This is not the right fix because this won't be using system libssl on non-portable builds. We need to either move this line:

__CMakeArgs="-DFEATURE_DISTRO_AGNOSTIC_SSL=$__PortableBuild $__CMakeArgs"

(also present in eng/native/build-commons.sh and src/native/corehost/build.sh)

out of the the shell script's if block without touching the C sources, or change the condition to #if defined(FEATURE_DISTRO_AGNOSTIC_SSL) && FEATURE_DISTRO_AGNOSTIC_SSL==1 without changing the shell scripts.

@vcsjones

Copy link
Copy Markdown
Member Author

out of the the shell script's if block without touching the C sources, or change the condition to #if defined(FEATURE_DISTRO_AGNOSTIC_SSL) && FEATURE_DISTRO_AGNOSTIC_SSL==1 without changing the shell scripts.

I am a bit confused by your assessment. We have a lot of other checks that use #ifdef FEATURE_DISTRO_AGNOSTIC_SSL without guarding what is the actual value of FEATURE_DISTRO_AGNOSTIC_SSL.

Every check is broken?

@am11

am11 commented Jun 12, 2026

Copy link
Copy Markdown
Member

See the diff https://godbolt.org/z/qh3hqsrP9.

Every check is broken?

@tmds, @omajid, have you guys verified on distros?

@jkotas

jkotas commented Jun 12, 2026

Copy link
Copy Markdown
Member

__CMakeArgs="-DFEATURE_DISTRO_AGNOSTIC_SSL

This definition applies to cmake.

The definition that applies to C that is relevant for this fix is here:

if (FEATURE_DISTRO_AGNOSTIC_SSL)
list(APPEND NATIVECRYPTO_SOURCES
opensslshim.c
)
add_definitions(-DFEATURE_DISTRO_AGNOSTIC_SSL)

@am11

am11 commented Jun 12, 2026

Copy link
Copy Markdown
Member

Ah, that makes sense. Maybe we should prefix it with CLR_CMAKE_FEATURE instead, would be less confusing. 😅

@vcsjones vcsjones merged commit b9ff865 into dotnet:main Jun 12, 2026
114 of 116 checks passed
@vcsjones vcsjones deleted the fix-ossl-ifdef branch June 12, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build failure: error: 'FEATURE_DISTRO_AGNOSTIC_SSL' is not defined

6 participants