RFR: 8334433: jshell.exe runs an executable test.exe on startup [v2]

[Jaikiran Pai]

On Tue, 25 Jun 2024 12:38:26 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> JLine has multiple providers that can setup and work with the native terminals. As part of an upgrade to JLine 3.26.1 (https://github.com/openjdk/jdk/pull/18142), a first phase of checks in the terminal providers providers runs fairly soon, when the providers are gathered. There is also a provider which setups the terminals using command line utilities - the "exec" provider. And this provider currently runs the "test" command line utility during the gathering phase, which (I believe) was not the case in previous version. I.e. even if some other provider is used to work with the terminal, the "exec" terminal might run the "test" utility anyway.
>> 
>> We currently largely don't need the "exec" provider at all - we currently primarily use a semi-native provider based on FFM. The "exec" provider is, I believe, used on platforms for which the FFM provider is not implemented, which is probably primarily AIX.
>> 
>> The proposal herein is twofold:
>> - disable the "exec" terminal on platforms for the FFM provider should work (Windows, Linux, Mac)
>> - make the "exec" provider ignore the `PATH` environment variable, and use hardcoded value.
>> 
>> I tested this on Windows with `cmd`, Cygwin and MSYS, and on Linux and seemed to work fine.
>> 
>> @MBaesken, please let me know what you think of this change.
>> 
>> I would like to backport this to JDK 23.
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reflecting review feedback.

Hello Jan,
The changes look OK to me. This now prevents launching (potentially) arbitrary processes when launching jshell on platforms where we have FFM support. 

For AIX systems, like you note, we would need inputs from someone from AIX if this change is OK. Especially, the part where we have hardcoded the paths to `/usr/bin` and `/bin` - I don't know if those are applicable for AIX or something else would be needed.

Matthias has noted in this PR that he has included this change in their build queue, but the newly introduced test is not configured to run on AIX, so I'm not sure if it would catch a regression. Do you think any manual testing would need to be done by Matthias to verify those paths work fine?

-------------

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19828#pullrequestreview-2138809842