Repository navigation
Replace JNA with java.lang.foreign in equinox.security.linux - #1348
Conversation
|
@jjohnstn I would appreciate your review and testing |
There was a problem hiding this comment.
🟡 Changes recommended
Native lists use the wrong destructor, risking crashes, while several owned native allocations are leaked.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces JNA-based Linux secure storage integration with Java FFM bindings for libsecret and GLib/GIO.
Changes:
- Adds FFM layouts, native bindings, and helper utilities.
- Removes obsolete JNA structures and dependency.
- Raises the fragment requirement to Java 25 and version to 1.2.0.
File summaries
| File | Description |
|---|---|
SecretSchemaAttributeType.java |
Removes obsolete JNA constants. |
SecretSchemaAttribute.java |
Removes obsolete JNA structure. |
SecretSchema.java |
Implements the native schema with FFM layouts. |
LinuxPasswordProvider.java |
Migrates secure-storage operations to FFM. |
LibSecret.java |
Adds libsecret downcall bindings. |
LibGio.java |
Adds GLib/GIO downcall bindings. |
GList.java |
Removes the JNA list structure. |
GError.java |
Reads native errors through FFM. |
Foreign.java |
Adds shared FFM loading and invocation utilities. |
pom.xml |
Bumps the bundle version. |
META-INF/MANIFEST.MF |
Removes JNA and requires Java 25. |
.settings/org.eclipse.jdt.core.prefs |
Configures Java 25 compilation. |
.classpath |
Selects the Java 25 runtime. |
Review details
Suppressed comments (2)
bundles/org.eclipse.equinox.security.linux/src/org/eclipse/equinox/internal/security/linux/LinuxPasswordProvider.java:94
secret_password_lookup_synctransfers ownership of the returned native string to the caller. Converting it to a Java string without then callingsecret_password_free(org_free) leaks the password allocation on every secure-storage read; add the corresponding binding and release the pointer in afinallyblock after copying it.
return Foreign.readString(password);
bundles/org.eclipse.equinox.security.linux/src/org/eclipse/equinox/internal/security/linux/LinuxPasswordProvider.java:75
unlockedandlistareGList*values, notGError*values. Passing either tog_error_freeinvokes the wrong native destructor and can corrupt the GLib allocator or crash whenever an unlock is attempted. Bindg_list_freeand use it for both list containers instead.
fLibGio.errorFree(unlocked.get(ValueLayout.ADDRESS, 0));
fLibGio.errorFree(list);
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results 231 files 231 suites 45m 23s ⏱️ Results for commit b8c7017. ♻️ This comment has been updated with latest results. |
0632612 to
e32a322
Compare
There was a problem hiding this comment.
🟡 Changes recommended
CI lacks the required Java 25 toolchain, and the migrated native path has no active automated coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
7f2fe79 to
4b9f6b8
Compare
4b9f6b8 to
71f74ed
Compare
It's a prereq to allow using Java 25 in eclipse-equinox/equinox#1348 . It will require that all verification builds are migrated to use Java 25 to keep them working.
It's a prereq to allow using Java 25 in eclipse-equinox/equinox#1348 . It will require that all verification builds are migrated to use Java 25 to keep them working.
71f74ed to
a23f49b
Compare
a23f49b to
a452d1f
Compare
It's a prereq to allow using Java 25 in eclipse-equinox/equinox#1348 . It will require that all verification builds are migrated to use Java 25 to keep them working.
There was a problem hiding this comment.
🔵 Needs a closer look
Critical native ABI and error-handling defects remain, while the FFM bindings lack effective test coverage.
Review details
Suppressed comments (1)
bundles/org.eclipse.equinox.security.linux/src/org/eclipse/equinox/internal/security/linux/LibSecret.java:59
- No automated test currently exercises these new downcall descriptors:
LinuxPreferencesTestis absent fromAllSecurityTests, andObsoletesTestskips wheneverisValid()detects a broken binding. A wrong SONAME, descriptor, variadic boundary, or schema layout can therefore make CI green by skipping. Add a binding-level test using a controlled native fixture, or otherwise ensure failures in the FFM path fail the suite.
private LibSecret() {
SymbolLookup library = Foreign.load(SONAME);
serviceGetSyncHandle = Foreign.downcall(library, "secret_service_get_sync", //$NON-NLS-1$
FunctionDescriptor.of(ValueLayout.ADDRESS, ValueLayout.JAVA_INT, ValueLayout.ADDRESS,
ValueLayout.ADDRESS));
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
|
Thanks for working on this. |
|
Honestly no, I haven't looked into jextract at all. I left it for exercise for later as this one is noisy already to get Java 25 ready. |
a452d1f to
08148d7
Compare
|
It is in good shape according to my testing so unless there are concerns I plan to push this one early next week. |
|
I think everyone has been supporting moving in this direction so it looks like a good plan and now is the best time to make such changes. |
08148d7 to
0087cce
Compare
Use FFM downcalls to libsecret/GIO instead of JNA mappings and drop the com.sun.jna requirement. Needs BREE JavaSE-25, so the fragment no longer resolves on older JREs. Assisted-by: Anthropic Claude Code (claude-opus-5[1m])
0087cce to
b8c7017
Compare
Use FFM downcalls to libsecret/GIO instead of JNA mappings and drop the com.sun.jna requirement.
Needs BREE JavaSE-25, so the fragment no longer resolves on older JREs.
Assisted-by: Anthropic Claude Code (claude-opus-5[1m])