Repository navigation
Conversation
- zstd: use the C90 qsort workaround on OpenHarmony. - System.Net.Security.Native: skip gssapi on OpenHarmony and use the OpenSSL crypto shim. - System.Native: read the full ethtool speed, and avoid close_range (the sandbox traps it with SIGSYS) by setting FD_CLOEXEC from /proc/self/fd. - System.Security.Cryptography.Native: resolve libssl/libcrypto by absolute path next to the shim and keep the dlopen shim in the NativeAOT static archive.
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
dotnet#136194 carries N3/N4/N7/N8/D with device validation. Closed dotnet#135839, dotnet#135840, dotnet#135866 and dotnet#136187 in its favor and replied to am11 on dotnet#136132. Runtime in flight: dotnet#136185, dotnet#136186, dotnet#136194; SDK: dotnet#56732, dotnet#56733.
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
Rebased pr/ohos-native-robustness onto d267e98 (b3ddf1c, 11 files 123+/9-) and opened the PR. Runtime in flight: dotnet#136185, dotnet#136186, dotnet#136194, dotnet#136198; SDK: dotnet#56732, dotnet#56733.
Reverting the OpenHarmony TFM special casing leaves an empty diff; the standard TargetOS TFM path works like FreeBSD. Replied to jkotas and closed the PR. Runtime in flight: dotnet#136185, dotnet#136194, dotnet#136198.
|
The native fixes were consolidated at @am11's request. @jkotas noted on #135839 that a consolidated multi-area PR needs sign-offs from owners of multiple areas and will be slow. #136194 currently touches process, network interfaces, crypto/gssapi and zstd, but has no area-owner reviews yet (and only carries the (a) keep it consolidated and request sign-off from each area owner, or We're happy to do either. If (b), we'll reopen the per-area PRs (or open fresh focused ones) and link the same device validation (hello, child process, NIC, RSA/TLS). Note This comment was drafted with AI assistance (agent tooling) under the author's direction. |
The "port" PRs in the past had native code clubbed together (src/native, src/coreclr etc.). Those PRs are miscellaneous in nature. Once the initial work is done, we switch to regular per-area work routine. e.g. the code in this PR has less to do with "domain knowledge" and more to do with coding conventions and looking out for unnecessary code bloat. |
am11
left a comment
There was a problem hiding this comment.
Change this line to:
<NetCoreAppNativeLibrary Include="System.Net.Security.Native" Condition="!$(_targetOS.StartsWith('tvos')) and '$(_linuxLibcFlavor)' != 'bionic' and '$(_targetOS)' != 'openharmony'" />Drafted with AI assistance.
|
Addressed in
Note This reply was drafted with AI assistance (agent tooling) under the author's direction. |
|
You still need am11@7ef728f. |
|
Applied your Note This reply was drafted with AI assistance (agent tooling) under the author's direction. |
|
Gentle ping when you have a moment: CI is green (100/100) and Note This comment was drafted with AI assistance (agent tooling) under the author's direction. |
| DlOpen(MAKELIB("4")); | ||
| } | ||
|
|
||
| #if defined(TARGET_OPENHARMONY) |
There was a problem hiding this comment.
@dotnet/area-system-security, it wasn't put next to android etc. above because the "published app" mode takes linux route and only dotnet exec myapp.dll takes this fallback.
|
|
||
| #if HAVE_FDWALK | ||
| // Callback used with fdwalk() on Illumos/Solaris to set FD_CLOEXEC on all file descriptors >= 3. | ||
| #if defined(TARGET_OPENHARMONY) |
There was a problem hiding this comment.
This should be reviewed and signed off by @adamsitnik
| { | ||
| #ifdef TARGET_ANDROID | ||
| nii->Speed = (int64_t)ecmd.speed; | ||
| #if defined(TARGET_ANDROID) || defined(TARGET_OPENHARMONY) |
There was a problem hiding this comment.
This should be reviewed by @dotnet/ncl .
Also, do we need a regression test for this bug on Android?
There was a problem hiding this comment.
This looks android specific, @simonrozsival could you or someone from your team take a look?
There was a problem hiding this comment.
re regression test, I already commented on #136354 that a regression test should be part of the same PR that introduces the fix to make it less chaotic
There was a problem hiding this comment.
The regression test is now part of this PR in 6364648. Could you take a look?
|
Thanks for the review and sign-off, @am11. Thanks for the pointers, @jkotas.
Note This comment was drafted with AI assistance (agent tooling) under the author's direction. |
|
Local deterministic repro of the Android/OpenHarmony speed truncation (no high-speed NIC needed). Harness and output: It compiles the real
So the truncation is real and the fix is correct. A managed test cannot reproduce this deterministically (it needs a NIC reporting >65535 Mbps); I can add a conditional Note This comment was drafted with AI assistance (agent tooling) under the author's direction. |
The Android/OpenHarmony ethtool path can truncate speeds above 65535 Mbps. Compare NetworkInterface.Speed with /sys/class/net/<iface>/speed when the kernel reports a high-speed link; skip interfaces where sysfs is unavailable.
|
Moved the regression test from #136354 into this PR ( Note This comment was drafted with AI assistance (agent tooling) under the author's direction. |
|
Thanks again for the earlier review and pointers. The regression test from #136354 is now included in this PR in Following the review comments, would it be possible for the relevant area owners to take a look when convenient?
No rush at all — happy to adjust anything that looks off.
|
adamsitnik
left a comment
There was a problem hiding this comment.
@springmin Big thanks for your contribution. PTAL at my comments.
|
Thanks for the review. Updated in Could you take another look when convenient?
|
OpenHarmony seccomp uses SCMP_ACT_TRAP for close_range, so the syscall receives SIGSYS and never returns; errno == SIGSYS cannot be observed. Keep the /proc/self/fd walker on OpenHarmony.
|
I verified this on-device. OpenHarmony uses
|
|
The current diff is now five files. Most earlier review threads were against pre-rebase revisions and are marked outdated, so I resolved those stale threads. The remaining threads refer to the current code. On The speed regression test is included in
|
| #if defined(TARGET_OPENHARMONY) | ||
| // OpenHarmony has no system libssl.so.3, and a shell-launched process only searches system | ||
| // directories, so fall back to the libssl deployed next to this library. | ||
| static void DlOpenNextToShim(const char* libraryName) |
There was a problem hiding this comment.
I certainly hope that the OSSL assets are being pulled in at the last minute via something like a Docker build script, and not being redisted with the concept of the runtime.
There was a problem hiding this comment.
This PR does not add or redistribute any OpenSSL binaries. It only adds the fallback loader in opensslshim.c. The OSSL assets are produced at image-build time in the #1786 image; the runtime repository contains no libssl/libcrypto binaries. The sibling lookup does not package an asset — it only tries an already-deployed libssl after the normal system lookups fail.
NOTE: AI-assisted.
| DlOpen(MAKELIB("4")); | ||
| } | ||
|
|
||
| #if defined(TARGET_OPENHARMONY) |
There was a problem hiding this comment.
If this block is supposed to match probe order with Linux, then I wonder if we'd better having a conditional path expansion at the beginning of DlOpen; or something like the TARGET_OPENHARMONY build causes current DlOpen to be named DlOpenCore and the current proposed DlOpenNextToShim takes over the name DlOpen.
That'd be similar to how, when we loaded OpenSSL on OSX/macOS, we handled the .so vs .dylib inside DlOpen.
If they're only coincidentally the same, and it's expected to deviate at some point, then this is fine.
There was a problem hiding this comment.
I described it above #136194 (comment), it's not needed for published app (which use linux route) that's why it has lower precedence.
There was a problem hiding this comment.
The sibling lookup is intentionally after the normal probe order: libssl.so.3, 1.1, and 4 are tried first. DlOpenNextToShim only runs when all of those fail, which is the shell-launched dotnet exec case. Published apps take the Linux route and are unaffected.
I can factor the path expansion into DlOpen if you prefer, but it would add a second path-selection mechanism for a fallback that is intentionally lower precedence.
NOTE: AI-assisted.
There was a problem hiding this comment.
So on OpenHarmony it's going to try all versions from system paths, then all versions from a local path? Why would the library be findable for a published library but not a local-exec one?
There was a problem hiding this comment.
A published app (HAP) is launched by the system with its libs/<abi> directory on the loader search path, so libssl.so.3 resolves by soname. A shell-launched dotnet runs in the default loader namespace, which only searches system directories (/system/lib64, /vendor/lib64, /lib64, ... per ld-musl-namespace-*.ini). The platform's own OpenSSL is libssl_openssl.z.so, so no libssl.so.3 is reachable there, and our native libs have no $ORIGIN runpath to find one next to the shim.
Published apps are the production-facing case, and dotnet app.dll from a shell is the secondary one, hence the precedence: the normal probes run first (so the HAP's bundled copy or LD_LIBRARY_PATH wins), and the sibling lookup is only a last-resort fallback.
There was a problem hiding this comment.
FWIW, static linking OpenSSL is bad for two reasons. 1) no security updates unless you rebuild, and 2) France (et al).
It's the same as redisting, really (except that you make it harder for people to use you as a passive dependency to the crypto libraries).
But, not really relevant for this PR. Just a thing to share for "if some official OpenHarmony .NET SDK is building with static linking for OpenSSL, that's a problem"
There was a problem hiding this comment.
- France (et al).
Pretty sure OpenHarmony is really only intended for China today.
There was a problem hiding this comment.
Static linking doesn't use this loading mechanism. I'm not sure how @springmin came to the conclusion that reverting this file is the ask here? @bartonjs only wanted to understand the loading order choice. Yes, OHOS is ... quite unique than what we are used to (in a fragmented way..): #132953 (comment).
There was a problem hiding this comment.
You're right — I over-interpreted the earlier questions as a request to remove the fallback. I've restored opensslshim.c in 6f6d01e4ad1; the PR is back to the original five-file scope.
The fallback remains the lower-priority path for shell-launched dotnet exec. A HAP resolves the app-provided library through the normal libs/<abi> soname probes; the sibling lookup only runs after those probes fail. I'm not splitting this into a loader PR.
On the static-linking point: this PR does not add or change static OpenSSL linking. The static-linked SDK artifact is pre-existing; I'm not using it as justification for the loader, and static-linking policy remains out of scope for this PR.
The mid/long-term items — static-linking policy, an OH_Crypto-backed System.Security.Cryptography, and general CA-store packaging — will be separate issues/PRs after this lands.
Note
This comment was drafted with AI assistance (agent tooling) under the author's direction.
There was a problem hiding this comment.
Added an isolated fallback check, so the validation now covers both the normal soname path and the lower-priority sibling path:
- HAP (JIT, app-provided
libs/arm64-v8a):OpenSSL_versionreports 3.5.9, the loadedlibcrypto.so.3is under the HAP-privatelibs/arm64directory, RSA 2048 and TLS (577 bytes) pass. This is the normal soname lookup. - Shell
dotnet exec:LD_LIBRARY_PATHpointing at 3.5.9 reports 3.5.9; the default environment reports 3.6.5. RSA/TLS pass in both cases. - Independent fallback branch: I forced the normal
libssl.so.3probe to fail with a non-ELF placeholder onLD_LIBRARY_PATHand suppliedlibcrypto.so.3separately. Withlibssl.so.3next to the shim, RSA 2048 and TLS pass. Removing that sibling reproducesNo usable version of libssl was foundand an RSA failure. The shim itself has onlyNEEDED libc.so. - Version consistency: the reported
OpenSSL_versionmatched the selected library in all runs.
These are validation-only results; no files or code changed, and the five-file scope is unchanged. The HAP and LD_LIBRARY_PATH runs exercise the normal soname probes; the third run exercises DlOpenNextToShim.
Note
This comment was drafted with AI assistance (agent tooling) under the author's direction.
The sibling libssl probe is a separate loader-policy change; keep this PR scoped to the process, interface-address and build-selection fixes.
This reverts commit 17dbc17.
Summary
Make the OpenHarmony native libraries work on device without platform-specific
build plumbing: the
close_rangefallback for the seccomp sandbox, the fullethtool speed read, the crypto-library selection via the existing static
OpenSSL link, and the lower-priority shell fallback for a sibling libssl.
Changes
src/native/libs/CMakeLists.txt: selectSystem.Security.Cryptography.Nativefor OpenHarmony (the same branch as
FORCE_ANDROID_OPENSSL) and skipSystem.Net.Security.Native, which has no gssapi in the OHOS sysroot.System.Native/pal_interfaceaddresses.c: read the full ethtool speed onAndroid/OpenHarmony, whose UAPI headers lack
ethtool_cmd_speed.System.Native/pal_process.c: on OpenHarmony, walk/proc/self/fdwithgetdents()and setFD_CLOEXECin the forked child;close_rangeis trappedby the sandbox with SIGSYS. Also fixes the
HAVE_CLOSE_RANGEcmakedefine01guard so the
__NR_close_rangefallback stays available on Linux.System.Security.Cryptography.Native/opensslshim.c: after the normal sonameprobes fail, try
libsslnext to the shim for shell-launcheddotnet execonOpenHarmony (the device has no system
libssl.so.3).Validation (HarmonyOS device, aarch64, SDK 11.0.100-rc.2.26451.112)
childline exercises the handle-inheritance fallback; thenicline thespeed read. The
rsa/tlslines exercise the crypto library selected via theexisting static-link path.
libs/<abi>through thenormal soname probes, and shell
dotnet execfalls back to a sibling only afterthose probes fail.
-1) on this device'swlan0(thedriver does not expose an ethtool speed); enumeration itself succeeds.
Test
dll with the existing host (
dotnet app.dll; the apphost cannot be executeddirectly on this device).
Out of scope
System.Security.CryptographyimplementationThe
System.Net.Security.Nativeexclusion inMicrosoft.NETCore.Native.Unix.targetsis in #136185.Supersedes #135866 and #136187.
Note
This PR was authored with AI assistance (agent tooling) under the author's direction.