On Wed, 20 Mar 2024 18:55:38 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> This is a patch that: >> a) upgrades the JLine inside the JDK to 3.25.1 >> b) since the new version of JLine has a FFM backend, our custom native >> backends are removed, and replaced with the FFM backend >> >> Some changes had to be made to the original JLine in order to fit into the >> JDK. Most of them were already done for the previous version (repackaging, >> avoiding non-ASCII characters, commenting out logging, adding ability to >> modify to wrap the InputStream used by the terminal), and have only been >> transferred to the new one. The main two new changes are: >> - fixes to the FFM backend, so that it works on Linux and JDK 22. These have >> been proposed to JLine itself: https://github.com/jline/jline3/pull/945 >> - disabling the `NativeFileDescriptorCreator`, as I believe we don't need >> it, and cannot make it work easily >> >> There's a full patch between the >> `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content >> of the corresponding sources of these original JLine sub-projects: >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal >> the patch is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff >> >> I've also cleaned the patch a little removing most of the changes for the >> rename. The result is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 26 commits: > > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 > - Merge remote-tracking branch 'origin/native-access-modules1' into > native-access-modules1 > - Apply suggestions from code review > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> > - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 > - Reflecting review feedback. > - Updating copyright headers. > - Re-enabling the exec provider. > - Cleanup. > - ... and 16 more: https://git.openjdk.org/jdk/compare/e5e7cd20...c8097592 src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java line 497: > 495: sb.append("Unable to find openpty native method in static > libraries and unable to load the util library."); > 496: List<Throwable> suppressed = new ArrayList<>(); > 497: try { This entire section trying to load libutil is quite complex. For a start, I'd recommend you break it out into a separate method `loadLibUtil` or something like that, to make this clearer. src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java line 504: > 502: } > 503: if (openPtyAddr.isEmpty()) { > 504: String libUtilPath = > System.getProperty("jdk.internal.org.jline.ffm.libutil"); Wait a minute. If I get this right, then you can pass `-Djdk.internal.org.jline.ffm.libutil=mydodgylib.so` as command line, and have the JDK load any arbitrary library? That sounds super bad from a security perspective. I recommend you remove this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535461403 PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535462748
