FELIX-6759 - Remove sun.misc.Unsafe - use plurl for URL handler factory multiplexing - #552
FELIX-6759 - Remove sun.misc.Unsafe - use plurl for URL handler factory multiplexing#552paulrutter wants to merge 3 commits into
Conversation
The pull_request trigger filters on the BASE branch, and only accepted master. A stacked pull request - one opened against another feature branch rather than master - therefore matched neither trigger and got no CI at all, which is what happened to PR #552. Accept feature/** and maintenance/** as a base as well, so stacked work is built before it is merged down. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI is red on purpose — Apache RAT rejects the vendored sourcesNow that stacked PRs get CI (the All 12 flagged files are the vendored plurl sources — the 11 This is the licensing question answering itself. The failure is being left in place deliberately. It could be silenced with a RAT exclusion for So the technical side is done and the remaining blocker is purely licensing:
@tjwatson — if the EPL headers are carried over from the original Equinox code rather than intended, correcting them upstream to match plurl's own Apache-2.0 LICENSE and pom would turn this red build green and unblock the approach for Felix and anyone else wanting to adopt the library. If they are intentionally EPL-2.0, then Felix needs one of the alternatives listed in the PR description instead. 🤖 Generated with Claude Code |
|
The intention when I moved the source to Somehow that was overlooked when setting up the source in https://github.com/eclipse-osgi-technology/plurl It should be a non-issue to get the headers updated. But what I ultimately wanted to do was have a release out of the osgi-technology project for plurl that would be used by both Equinox and Felix unchanged, in the original package there. |
Thanks for your quick reply! Let's first see if we can get consensus on the JDK 25 PR, then we can revisit this one. Would it be possible to get plurl released as a separate artifact, with the right headers, so we can use it without copying the code into Felix? Apart from that, does this PR make sense to you? |
I've updated the source headers to use Apache-2.0 with: eclipse-osgi-technology/plurl#45 Note that the source also needed an upstream fix I had put in Equinox with PR: eclipse-osgi-technology/plurl#55 I am not sure how long it will take to get a release out of the osgi-technology project because I have not done a release out of this particular project yet. I'll ask existing members of the project about what steps are needed to get a release done. But I don't want that to block you on progress. With the current state of the source I think it should be fine for you to proceed with copying in the source now. |
|
Thanks, that's great! I will pull in the latest version and use it verbatim except the package names. Once the JDK 25 PR lands, this one is next on the list. Once a proper release is out, we can then switch to that artifact instead and remove the vendored sources. |
|
I don't see where you are calling |
| return name.startsWith("org.apache.felix.framework.BundleWiringImpl$BundleClassLoader") | ||
| || name.startsWith("org.apache.felix.framework.ModuleImpl$ModuleClassLoader") | ||
| || name.equals("org.apache.felix.framework.searchpolicy.ContentClassLoader"); |
There was a problem hiding this comment.
For Equinox I do an additional check to make sure the class loader is for a bundle that is actually installed in this framework instance. I assume you would want the same thing here. Otherwise one felix framework may handle lookups for a bundle resolved in another felix framework instance.
Looking into this, i stopped working on the draft when i noticed the license. Will continue with it now that's resolved 👍🏻 |
I should mention that the intention was to use a |
Addresses the review on PR #552 and makes plurl the mechanism rather than an illustration alongside the existing one. Vendored sources refreshed from upstream. Every file is now Apache-2.0 (eclipse-osgi-technology/plurl#45 corrected headers that were EPL-2.0 by oversight, which apache-rat rightly rejected) and includes the fix from eclipse-osgi-technology/plurl#55. Only the package rename is applied. The prototype could not have worked as written: - Plurl.install(..) was never called. The static Plurl.add(..) helpers operate through a plurl: URL, so without installing the router first they fail with "unknown protocol: plurl". Nothing exercised the adapter, so it compiled and the tests passed while being dead code. - shouldHandle only checked that a class came from some Felix bundle class loader. Two frameworks in one JVM both load classes that way, so one framework could answer lookups for a bundle resolved in another. It now also requires the owning framework to match, as Equinox does. Plurl is now the only mechanism, matching Equinox rather than keeping a fallback: - URLHandlers no longer swaps the java.net.URL and URLConnection static fields, and its singleton is no longer constructed. Keeping that as a fallback would have kept the sun.misc.Unsafe path alive, which is what adopting plurl is meant to remove. Equinox does not fall back either: if plurl registration fails it logs and that framework contributes no URL handlers. - Removing the swap turned out to be necessary rather than merely tidy. With URLHandlers taking the singletons first, plurl found them occupied and required --add-opens java.base/java.net to replace them. Installing into a clean JVM uses the supported API and needs no flag. - URLHandlersBundleStreamHandler and URLHandlersStreamHandlerProxy now extend PlurlStreamHandlerBase, exactly as Equinox changed BundleResourceHandler and URLStreamHandlerProxy. Otherwise plurl has to reflect into java.net to drive them. - The router is installed once per JVM and reference counted. Uninstalling it per framework would tear it down while another framework was still using it. - The bundle: handler is deliberately not bound to one framework. The JVM caches one handler per protocol, so a pinned handler is also used for another framework's bundle: URLs; the framework is resolved per call from the UUID in the URL host. - URLHandlers.m_sm is initialised eagerly, since the constructor that used to set it up is no longer invoked. Adds PlurlURLHandlersTest, which starts a framework and asserts that registration actually happened, that the plurl: protocol resolves, and that shouldHandle does not claim classes outside this framework. On JDK 25 the framework suite is at 121 tests with 9 failures, 8 of which are pre-existing platform specific ones unrelated to this change. The remaining failure is URLHandlersTest.urlHandlersWithClassLoaderIsolation, discussed on the pull request. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for sorting the headers — I refreshed the vendored sources from upstream, so they include the Apache-2.0 headers from eclipse-osgi-technology/plurl#45 and the fix from eclipse-osgi-technology/plurl#55. Only the package rename is applied. And Both of your review points were real defects
Plurl is now the mechanism, not an addition alongside it
On Linux the framework suite is 121 tests with a single error, which is the question below. Routing a URL re-parsed outside any bundle
URL url = bundle.getEntry("...");
new URL(url.toExternalForm()).openStream(); // called from a plain test classThat second Note I deliberately do not bind the I initially assumed Equinox must solve this and Felix was missing something. Reading String host = url.getHost(); // "<bundleId>.<container.hashCode()>"
bundleID = parseBundleIDFromURLHost(host); // parses ONLY the bundle id
Module module = getModule(bundleID); // -> container.getModule(id), the BOUND containerThe container is encoded in the host by Felix's UUID lookup is a stronger guarantee, which is why there's a test for it here and no equivalent upstream. So the question isn't how Equinox does it, but: Would plurl consider routing a A Without that, the Felix-side options are:
I'd rather not do 1 silently. Happy to raise the API idea as a plurl issue with a concrete proposal if you think it's reasonable. |
I am a bit confused by this because there is a unit test in plurl that mimics a copy scenario and a separate class loader (proxy) scenario. The intent of Plurl is not only can the users have their own copy loaded by a difference class loader, but their own copy could live in a completely different package name. For example. see the test that has a copy of Plurl APIs at In this case it reflects on agreed upon method names from the Plurl APIs. This way, as long as the copies mirror the upstream Plurl APIs then they can intermix in the same JVM. This way Felix and Equinox can both run in the same JVM and still multiplex on the protocols even if they copied the plurl API/impl into their own felix and equinox packages. Can you go into more details on what plurl changes you need to get the |
This commit touches only the vendored plurl sources and is meant to be read as a proposal against the upstream plurl project, discussed on #552. It is separated from the Felix side so it can be reviewed, or lifted upstream, on its own. Problem: a factory can currently only be selected from a class on the call stack. If no class is claimed, "the first factory added is selected". That is arbitrary when several instances of the same framework are registered, because they all share a protocol and are told apart only by the URL. It happens in practice when a URL is re-parsed by a caller that is in no bundle, for example new URL(bundleEntryURL.toExternalForm()).openStream(); where nothing on the call stack identifies a framework. Whichever framework registered first is then asked to resolve another framework's URL. Felix detects that and fails; a factory that trusts the selection would answer with the wrong resource. Changes: - PlurlFactory gains a default shouldHandle(URL) returning false, so existing factories are unaffected. A factory that can recognise its own URLs overrides it. - PlurlImpl.findFactory consults shouldHandle(URL) before walking the call stack, reflectively where the factory was compiled against a different copy of the plurl package, and tolerating factories that predate the method. - PlurlFactoryHolder delegates shouldHandle(URL) to the wrapped factory, as it already does for shouldHandle(Class). Without this the default would answer for every holder and the hook would never reach a factory. - PlurlRootURLStreamHandler only records a handler in urlToHandler once the URL is usable for selection. During parseURL the URL is still being populated and its host is not set yet, so a factory cannot claim it and only the call stack is available. Caching that choice would pin the URL to a factory chosen before the URL was known; not recording it lets the first use of the parsed URL select again. The last point is the subtle one: without it a shouldHandle(URL) hook is never reached for a freshly parsed URL, since parseURL runs first and its result is cached for the life of the URL. Note shouldHandle(URL) overloads shouldHandle(Class), which makes shouldHandle(null) ambiguous for callers. A distinct name would avoid that if this is taken upstream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You're right, my earlier statement was wrong — To answer your question: protocol-level delegation wouldn't help, because every Felix framework instance uses the same
Three things worth knowing, all found by implementing it:
Separately: when no factory claims a URL, declining may be safer than selecting the first factory added, since framework A can be handed a URL naming framework B. Felix's UUID check makes that a clean failure; Equinox resolves only the bundle id against its bound container, so it would answer with its own resource. Happy to leave the fallback as-is if you'd rather. Glad to raise this as a plurl issue/PR, squashed however you prefer. Ready-to-apply patch against
|
b5bbd43 to
c3094d7
Compare
|
Open an issue against eclipse-osgi-technology/plurl and you can attach your PR there also. That way we can discuss possible API additions there. Thanks for looking at this. |
Thanks! Will do and continue the discussion there. |
|
Because I've already released a version of Equinox with the current Plurl signatures I will need to attempt to make any new implementation with the new APIs still be able to handle when copies of the old APIs are still in use in the same JVM instance. That may become impossible over time, but I want to at least try to provide some compatibility between versions of plurl. Otherwise it will become increasingly difficult to be able to update a core framework implementation which hosts other applications which may embed their own version of a framework implementation (Equinox or Felix). Think of a WAR that uses osgi connect to compose their application components using felix or equinox in their WEB-INF/lib folder. And then that WAR being deployed to a server which is implemented with OSGi, such as glassfish(using felix) or liberty(using equinox). |
|
One correction before I open the plurl issue: the
Selection has to be decidable at the moment plurl chooses, i.e. mid-parse, when the URL has no host yet. The spec does exist then: default boolean shouldHandle(String protocol, String spec) { return false; }consulted in On your compatibility point: the signature uses only
51/51 in the plurl repo, including a new |
|
Added the silent-degradation warning, and the upstream half that makes it possible — plurl#63 now has a second commit. The problem: the plurl copy that wins the install in a JVM is the one that routes, and it may be older than the copy this framework brought. We register successfully and are then silently misrouted, with no way to tell — Upstream now answers a
Also Framework suite at the pre-existing baseline. @tjwatson no action needed on the Felix commits, but the capability commit upstream is take-it-or-leave-it — the selection commit stands on its own without it. |
|
Plur snapshot is available. Ci build is fully working |
I'm proposing a few upstream tweaks so it also covers all Felix needs, see attached PR for plurl. |
plurl multiplexes the JVM URL factory singletons through the supported URL.setURLStreamHandlerFactory API, so the framework no longer has to claim them by reflectively clearing private static fields of java.net.URL. Equinox uses the same mechanism, so a JVM hosting both can share one router. The sources are vendored under org.apache.felix.framework.plurl, byte for byte as upstream except the package rename, because plurl has not been released to Maven Central yet. Once it is, this becomes a provided dependency embedded with Private-Package, the way org.apache.felix.resolver already is, and these files go away. Includes the URL selection support the framework needs, which is merged upstream as eclipse-osgi-technology/plurl#63: every framework instance uses the same bundle: protocol, so the owner can only be identified by the UUID in the URL, which the call stack cannot supply when a bundle: URL is parsed by a caller that is in no bundle. Apache-2.0, credited in the framework NOTICE alongside the OSGi Alliance entry. See org/apache/felix/framework/plurl/README.md for provenance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each framework instance registers its own factory with the plurl router instead of this class installing itself as the JVM stream and content handler factory. Registering uses the supported URL.setURLStreamHandlerFactory API on a clean JVM, so no reflective write to a java.net static field is left anywhere in the framework. Failing to register does not prevent the framework from starting: as in Equinox, the consequence is that this instance contributes no URL handlers. There is deliberately no fallback to swapping the singleton fields, since removing that is the point. bundle: URLs are claimed by the framework UUID in the URL, read from the spec because plurl has to select a factory before the URL is parsed. Every framework instance uses the same bundle: protocol, so neither the protocol nor the call stack identifies an owner when a bundle: URL is parsed by a caller that is in no bundle. A null spec is the protocol level question plurl asks before any URL exists, and is answered by claiming the protocol. The framework warns at startup when the installed plurl does not support selection by URL, because bundle: URLs are then routed to whichever factory registered first and cannot be resolved. Nothing on the factory side can fix that, so it is reported rather than left to surface later as a failed resource lookup. With plurl owning the JVM factories, URLHandlers is never instantiated, so the dead instance side is removed with it: the constructor that performed the takeover, the cross-classloader framework list rendezvous that plurl now does, the URLStreamHandlerFactory and ContentHandlerFactory implementations, and the built-in handler lookup and caches. What remains is the framework registry that URLHandlersBundleStreamHandler uses to find the owner of a caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…efused Fixes the warning reported in this issue: starting Felix on JDK 24 or later printed that sun.misc.Unsafe::staticFieldOffset is terminally deprecated and will be removed. SecureAction loses swapStaticFieldIfNotClass and flush, which had no callers left once URLHandlers stopped taking over the JVM factories. What remains of Unsafe is getAccessor, which is not a memory access shim that can be ported to VarHandle: it defines an accessor class inside java.base so that setAccessible has an in-module caller, and closing that off is the intent of the JDK's integrity work. It is now reached only when ordinary reflection has already been refused, so where the package is open -- the Add-opens of the org.apache.felix.main launcher, or any --add-opens on the command line -- the deprecated method is never called. ClassPathExtenderFactory resolved URLClassLoader.addURL and ClassLoaders$AppClassLoader.appendToClassPathForInstrumentation in its static initialiser and made both accessible there. That runs from ExtensionManager.<clinit>, so every framework instance paid for it whether or not it ever installs an extension bundle. Looking a method up needs no access, only setAccessible does, so the lookup stays and the access is requested in add(File), where the method is invoked. A framework that installs no extension bundle now never calls setAccessible on a java.base member. Verified on JDK 25 with --sun-misc-unsafe-memory-access=deny: the framework starts and only framework extension bundles fail, cleanly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
34e7dbb to
26ac5e6
Compare
Fixes the
sun.misc.Unsafewarning reported in FELIX-6759 by using plurl to multiplex the JVM URL factory singletons, instead of the framework claiming them by reflectively clearing private static fields ofjava.net.URL.Stacked on #433 (FELIX-6807, Java 25 LTS support) and targets that branch, so it merges after it.
Why
URLHandlersinstalled itself as the JVM stream and content handler factory by clearingjava.net.URL's private static fields. Those are not accessible under JPMS, soSecureActionobtains a trustedMethodHandles.Lookupby readingMethodHandles.Lookup.IMPL_LOOKUPthroughUnsafe.staticFieldOffset, and uses it to define an accessor class insidejava.base. ThatstaticFieldOffsetcall is the one in the report. Two consequences beyond the warning: the last framework to install itself wins the singleton, and the takeover leaves the JVM factories occupied before any cooperative multiplexer can take them.The mechanism has already been patched twice, in FELIX-6430 and FELIX-6432. This stops needing it rather than adapting it again.
What plurl changes
plurl installs one cooperative router through the supported
URL.setURLStreamHandlerFactoryAPI and routes between registered factories, asking each whether a calling class, or the URL being parsed, belongs to it. Each framework instance registers its own factory. Equinox uses the same mechanism, so a JVM hosting both can share one router.Deliberately no fallback to the field-swapping path: if registration fails the framework contributes no URL handlers, matching Equinox. Keeping the fallback would defeat the purpose.
Upstream
plurl needed a change before it could route
bundle:URLs: every framework instance uses the samebundle:protocol, so the protocol does not identify an owner, and a URL parsed by a caller in no bundle leaves nothing on the call stack either — plurl selected the first factory added, which then cannot resolve another instance's UUID.That is merged upstream as eclipse-osgi-technology/plurl#63, with the issue at eclipse-osgi-technology/plurl#62. Thanks to @tjwatson for the review, which found a gap in the first cut: the JVM asks for a handler once per protocol with no URL, so a protocol served only by a factory that claims it by URL was declined before per-URL selection ever ran.
The sources are still vendored under
org.apache.felix.framework.plurl, byte for byte as upstream except the package rename, because plurl has not been released to Maven Central yet. Once it is, this becomes aprovideddependency embedded withPrivate-Package, the wayorg.apache.felix.resolveralready is, and the vendored files go away. The package is private, so that swap is invisible to anyone outside the bundle. Apache-2.0, credited in the frameworkNOTICEalongside the OSGi Alliance entry.Effect on the reported warning
Starting a framework on JDK 25 no longer produces it: measured across JDK 17/21/23/25, no terminal deprecation warning is attributed to Felix for a plain framework start, and the OSGi Core R8 TCK run is clean.
This is not a complete elimination of
Unsafe. One call site remains, reached only when a bundle registers aURLStreamHandlerService:URLHandlersStreamHandlerProxyreflects the protected methods ofjava.net.URLStreamHandlerto delegate to a JDK built-in handler. Protected access does not extend to invoking those methods on a different handler instance, so subclassing does not help; delegating to a built-in handler requires the package to be open. plurl has the same requirement and its own build passes--add-opens java.base/java.net.SecureAction.getAccessoris therefore kept, but is now reached only when ordinary reflection has already been refused. The supported way to satisfy it is for the launcher to grant access:--add-opens java.base/java.netand, for extension bundles,--add-opens java.base/jdk.internal.loader.org.apache.felix.mainalready declares both in itsAdd-opensmanifest header, honoured because it is launched withjava -jar. Embedders that put the framework on a classpath must pass the flags themselves — a manifest header does nothing there.Measured under deny
On JDK 25 with
--sun-misc-unsafe-memory-access=deny, which is what a future JDK will default to before the methods are removed, the framework starts and 119 of 122 framework tests pass. The three failures are all framework extension bundles, failing cleanly withBundleException: Could not create bundle object.Nothing corrupts or hangs.Verification
URLHandlersTest.urlHandlersWithClassLoaderIsolationpasses: a caller outside any bundle still resolves abundle:URL to the framework that owns it.PlurlURLHandlersTestcovers the plurl registration, that only this framework's classes andbundle:URLs are claimed, another framework's UUID is not, and that the installed plurl reports selection by URL — so re-vendoring an older plurl fails there rather than quietly turning the startup warning on for everyone.🤖 Generated with Claude Code