Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Fri, 1 Mar 2024 01:50:46 GMT, Vladimir Petko wrote:
> This MR fixes segsegv in jspawnhelper when it is called without args.
> This scenario happens when a long running Java process is not restarted
> during upgrade.
>
> It updates test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java to
> check that jspawnhelper exits with code 1:
>
> After test update:
>
> $ make CONF=linux-x86_64-server-fastdebug test
> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
> ...
> Running jspawnhelper without args
> STDERR:
> java.lang.Exception: Parent process exited with 12
> at
> JspawnhelperProtocol.simulateJspawnhelperWithoutArgs(JspawnhelperProtocol.java:126)
> at JspawnhelperProtocol.main(JspawnhelperProtocol.java:267)
> at
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
> at
> com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
> at java.base/java.lang.Thread.run(Thread.java:1575)
> ...
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>>> 1 0 1 0 <<
> ==
> TEST FAILURE
>
> After jspawnhelper change the test passes:
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
> 1 1 0 0
>
> ==
> TEST SUCCESS
>
>
> The user will see the following output in the logs:
>
> An earlier version of Java is trying to call jspawnhelper.
> Please restart Java process.
> Exception in thread "main" java.io.IOException: Cannot run program "ls":
> error=0, Failed to exec spawn helper: pid: 2168121, exit value: 1
> at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143)
> at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073)
> at Test.main(Test.java:3)
> Caused by: java.io.IOException: error=0, Failed to exec spawn helper: pid:
> 2168121, exit value: 1
> at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
> at java.base/java.lang.ProcessImpl.(ProcessImpl.java:314)
> at java.base/java.lang.ProcessIm...
Manual test:
public class Test {
public static void main(String[] args) throws Throwable {
Process p = new ProcessBuilder("ls", "-alrt", "/tmp").start();
p.waitFor();
}
}
Running older java with a new jspawnhelper will result in the following output:
java --version
openjdk 17.0.9-internal 2023-10-17
OpenJDK Runtime Environment (build 17.0.9-internal+0-adhoc.vladimirp.jdk17u)
OpenJDK 64-Bit Server VM (build 17.0.9-internal+0-adhoc.vladimirp.jdk17u, mixed
mode)
$ ./java -cp . Test
An earlier version of Java is trying to call jspawnhelper.
Please restart Java process.
Exception in thread "main" java.io.IOException: Cannot run program "ls":
error=0, Failed to exec spawn helper: pid: 2168121, exit value: 1
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143)
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073)
at Test.main(Test.java:3)
Caused by: java.io.IOException: error=0, Failed to exec spawn helper: pid:
2168121, exit value: 1
at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
at java.base/java.lang.ProcessImpl.(ProcessImpl.java:314)
at java.base/java.lang.ProcessImpl.start(ProcessImpl.java:244)
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1110)
-
PR Comment: https://git.openjdk.org/jdk/pull/18074#issuecomment-1972308222
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Fri, 1 Mar 2024 01:50:46 GMT, Vladimir Petko wrote: > This MR fixes segsegv in jspawnhelper when it is called without args. > This scenario happens when a long running Java process is not restarted > during upgrade. > > It updates test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java to > check that jspawnhelper exits with code 1: > > After test update: > > $ make CONF=linux-x86_64-server-fastdebug test > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > ... > Running jspawnhelper without args > STDERR: > java.lang.Exception: Parent process exited with 12 > at > JspawnhelperProtocol.simulateJspawnhelperWithoutArgs(JspawnhelperProtocol.java:126) > at JspawnhelperProtocol.main(JspawnhelperProtocol.java:267) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) > at java.base/java.lang.reflect.Method.invoke(Method.java:580) > at > com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) > at java.base/java.lang.Thread.run(Thread.java:1575) > ... > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java >>> 1 0 1 0 << > == > TEST FAILURE > > After jspawnhelper change the test passes: > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > > == > TEST SUCCESS > > > The user will see the following output in the logs: > > An earlier version of Java is trying to call jspawnhelper. > Please restart Java process. > Exception in thread "main" java.io.IOException: Cannot run program "ls": > error=0, Failed to exec spawn helper: pid: 2168121, exit value: 1 > at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1143) > at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1073) > at Test.main(Test.java:3) > Caused by: java.io.IOException: error=0, Failed to exec spawn helper: pid: > 2168121, exit value: 1 > at java.base/java.lang.ProcessImpl.forkAndExec(Native Method) > at java.base/java.lang.ProcessImpl.(ProcessImpl.java:314) > at java.base/java.lang.ProcessIm... Would it be possible to expand a bit on what is going on here? Are you saying that in the course of upgrading a JDK that you somehow get into a state where jspawnhelper has been replaced with a version from a newer JDK? Is the real issue here that the upgrade didn't shutdown the application and/or something copied over an existing installation during the upgrade? - PR Comment: https://git.openjdk.org/jdk/pull/18074#issuecomment-1972674199
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Fri, 1 Mar 2024 07:37:22 GMT, Alan Bateman wrote: > Would it be possible to expand a bit on what is going on here? Are you saying > that in the course of upgrading a JDK that you somehow get into a state where > jspawnhelper has been replaced with a version from a newer JDK? Is the real > issue here that the upgrade didn't shutdown the application and/or something > copied over an existing installation during the upgrade? It appears that answer is yes (in Debian and Ubuntu) if the machine is running unattended-upgrades service and openjdk is not blacklisted from upgrades. I believe we should blacklist openjdk packages by default to avoid this situation. - PR Comment: https://git.openjdk.org/jdk/pull/18074#issuecomment-1977331977
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan wrote:
> This change is intended to address the segmentation fault issue that occurs
> when jspawnhelper is called without arguments, and it includes an updated
> test to verify the behavior in such cases.
>
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without
>
>if (argc != 2) {
> shutItDown();
> }
>
> code block
>
>> Test results: failed: 1
>> Report written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >> 1 0 1 0
>> >> <<
>> ==
>> TEST FAILURE
>
>
>
> after added the code block back
>
>if (argc != 2) {
> shutItDown();
> }
>
>
> updated test passes
>
>
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
> \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
> \\
> -ea \\
> -esa \\
>
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
> \\
> com.sun.javatest.regtest.agent.MainWrapper
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...
lgtm
-
Marked as reviewed by eastigeevich (Committer).
PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1915420153
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan wrote:
> This change is intended to address the segmentation fault issue that occurs
> when jspawnhelper is called without arguments, and it includes an updated
> test to verify the behavior in such cases.
>
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without
>
>if (argc != 2) {
> shutItDown();
> }
>
> code block
>
>> Test results: failed: 1
>> Report written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >> 1 0 1 0
>> >> <<
>> ==
>> TEST FAILURE
>
>
>
> after added the code block back
>
>if (argc != 2) {
> shutItDown();
> }
>
>
> updated test passes
>
>
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
> \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
> \\
> -ea \\
> -esa \\
>
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
> \\
> com.sun.javatest.regtest.agent.MainWrapper
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...
Changes requested by rriggs (Reviewer).
Need more time to review.
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 111:
> 109: if (p.exitValue() != 1)
> 110: System.exit(ERROR + 2);
> 111: System.exit(0);
Its bad form to System.exit from not at the top level. Is that necessary.
-
PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1915545846
PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1977643226
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1511921441
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Mon, 4 Mar 2024 23:23:04 GMT, Roger Riggs wrote:
>> This change is intended to address the segmentation fault issue that occurs
>> when jspawnhelper is called without arguments, and it includes an updated
>> test to verify the behavior in such cases.
>>
>> Existing tests passes since it does not check behavior without args.
>> After test update the test fails without
>>
>>if (argc != 2) {
>> shutItDown();
>> }
>>
>> code block
>>
>>> Test results: failed: 1
>>> Report written to
>>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>>> Results written to
>>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>> Error: Some tests failed or other problems occurred.
>>> Finished running test
>>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>>> Test report is stored in
>>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>>
>>> ==
>>> Test summary
>>> ==
>>>TEST TOTAL PASS FAIL
>>> ERROR
>>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>>> >> 1 0 1
>>> >> 0 <<
>>> ==
>>> TEST FAILURE
>>
>>
>>
>> after added the code block back
>>
>>if (argc != 2) {
>> shutItDown();
>> }
>>
>>
>> updated test passes
>>
>>
>> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
>> \\
>> -Xmx768m \\
>> -XX:MaxRAMPercentage=1.04167 \\
>> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>>
>> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
>> \\
>> -ea \\
>> -esa \\
>>
>> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
>> \\
>> com.sun.javatest.regtest.agent.MainWrapper
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang...
>
> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 111:
>
>> 109: if (p.exitValue() != 1)
>> 110: System.exit(ERROR + 2);
>> 111: System.exit(0);
>
> Its bad form to System.exit from not at the top level. Is that necessary.
This is in line with other tests in this file , e.g. see `parentCode()`.
I agree that code would be cleaner if the test is refactored to return error
code from the test methods and system.exit() in the main instead, but this
might be a separate pr done before or after this one.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1511953089
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan wrote:
> This change is intended to address the segmentation fault issue that occurs
> when jspawnhelper is called without arguments, and it includes an updated
> test to verify the behavior in such cases.
>
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without
>
>if (argc != 2) {
> shutItDown();
> }
>
> code block
>
>> Test results: failed: 1
>> Report written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >> 1 0 1 0
>> >> <<
>> ==
>> TEST FAILURE
>
>
>
> after added the code block back
>
>if (argc != 2) {
> shutItDown();
> }
>
>
> updated test passes
>
>
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
> \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
> \\
> -ea \\
> -esa \\
>
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
> \\
> com.sun.javatest.regtest.agent.MainWrapper
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...
The change in jspawnhelper looks good.
I think it would be simpler to have a separate
`jdk/java/lang/ProcessBuilder/JspawnhelperMisuse.java` test that simply invokes
the `jspawnhelper` and verifies the proper message is printed. It would be more
straightforward than trying to fit the _protocol_ test? Plus, we can test 0, 1,
3, 4 args, not only 0 args in that test, and we can also test 2 args with bad
format.
-
PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1916529453
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan wrote:
> This change is intended to address the segmentation fault issue that occurs
> when jspawnhelper is called without arguments, and it includes an updated
> test to verify the behavior in such cases.
>
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without
>
>if (argc != 2) {
> shutItDown();
> }
>
> code block
>
>> Test results: failed: 1
>> Report written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >> 1 0 1 0
>> >> <<
>> ==
>> TEST FAILURE
>
>
>
> after added the code block back
>
>if (argc != 2) {
> shutItDown();
> }
>
>
> updated test passes
>
>
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
> \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
> \\
> -ea \\
> -esa \\
>
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
> \\
> com.sun.javatest.regtest.agent.MainWrapper
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...
I'm curious why this test is requires `vm.debug`? That means it generally
won't get run on release builds.
-
PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1979329508
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Mon, 4 Mar 2024 23:54:38 GMT, Vladimir Petko wrote: >> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 111: >> >>> 109: if (p.exitValue() != 1) >>> 110: System.exit(ERROR + 2); >>> 111: System.exit(0); >> >> Its bad form to System.exit from not at the top level. Is that necessary. > > This is in line with other tests in this file , e.g. see `parentCode()`. > > I agree that code would be cleaner if the test is refactored to return error > code from the test methods and system.exit() in the main instead, but this > might be a separate pr done before or after this one, e.g. something like [1]. > > [1] > https://github.com/vpa1977/jdk/commit/7cbbaf498da8b71476e61e1c0f3fa7882c7aa7b6 ok, the same pattern is used consistently. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1513262005
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Tue, 5 Mar 2024 17:56:21 GMT, Roger Riggs wrote: > I'm curious why this test is requires `vm.debug`? That means it generally > won't get run on release builds. This is due to the tests of the crash scenarios that are guarded with #ifdef DEBUG in jspawnhelper. Moving the test in the separate file is a great suggestion so that misuse case is added to the release test suite. - PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1979466109
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Tue, 5 Mar 2024 10:39:31 GMT, Aleksey Shipilev wrote: > The change in jspawnhelper looks good. > > I think it would be simpler to have a separate > `jdk/java/lang/ProcessBuilder/JspawnhelperMisuse.java` test that simply > invokes the `jspawnhelper` and verifies the proper message is printed. It > would be more straightforward than trying to fit the _protocol_ test? Plus, > we can test 0, 1, 3, 4 args, not only 0 args in that test, and we can also > test 2 args with bad format. Yes, this makes the test cleaner[1]. Also it seems that there is another issue with process streams handling: the test started to pass only after I've flushed stdout in jspawnhelper [2]. [1] https://github.com/vpa1977/jdk/commit/ac7b5c13eaaa177058c3940cf5c817f4c729ef56 [2] https://github.com/vpa1977/jdk/commit/5f398d5344f4f525cee0e92029528a70fd8e2479 - PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1979592133
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Tue, 5 Mar 2024 20:34:47 GMT, Vladimir Petko wrote:
> > The change in jspawnhelper looks good.
> > I think it would be simpler to have a separate
> > `jdk/java/lang/ProcessBuilder/JspawnhelperMisuse.java` test that simply
> > invokes the `jspawnhelper` and verifies the proper message is printed. It
> > would be more straightforward than trying to fit the _protocol_ test? Plus,
> > we can test 0, 1, 3, 4 args, not only 0 args in that test, and we can also
> > test 2 args with bad format.
>
> Yes, this makes the test cleaner[1].
Note that we have `OutputAnalyzer` for these test cases:
https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/process/OutputAnalyzer.java
-- lots of test use it, and it would be something like just:
Process p = ProcessTools.startProcess(...);
OutputAnalyzer oa = new OutputAnalyzer(p);
oa.shouldNotHaveExitValue(0);
oa.shouldContain("This command is not for general use");
-
PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1980434240
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Wed, 6 Mar 2024 09:26:08 GMT, Aleksey Shipilev wrote:
> ```
> Process p = ProcessTools.startProcess(...);
> OutputAnalyzer oa = new OutputAnalyzer(p);
> oa.shouldNotHaveExitValue(0);
> oa.shouldContain("This command is not for general use");
> ```
Thank you! This shortens things quite a bit[1]
[1]
https://github.com/openjdk/jdk/commit/bb65fc8551ae759df88171463445ecdb9676eed3
-
PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1982286476
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Tue, 5 Mar 2024 17:56:21 GMT, Roger Riggs wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > I'm curious why this test is requires `vm.debug`? That means it generally > won't get run on release builds. Let's see if @RogerRiggs and @vpa1977 have any additional feedback. - PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1984137117
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Tue, 5 Mar 2024 17:56:21 GMT, Roger Riggs wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > I'm curious why this test is requires `vm.debug`? That means it generally > won't get run on release builds. > Let's see if @RogerRiggs and @vpa1977 have any additional feedback. Hi, the changes look good, just a minor note regarding waitFor() removal. OutputAnalyzer/LazyOutputBuffer do not have timeout on waitFor() so in a rare case when jspawnhelper is stuck, the test might block the CI. It is hardly possible with the current implementation, but might become a possibility in the future. - PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1984253692
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v2]
> This change is intended to address the segmentation fault issue that occurs
> when jspawnhelper is called without arguments, and it includes an updated
> test to verify the behavior in such cases.
>
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without
>
>if (argc != 2) {
> shutItDown();
> }
>
> code block
>
>> Test results: failed: 1
>> Report written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >> 1 0 1 0
>> >> <<
>> ==
>> TEST FAILURE
>
>
>
> after added the code block back
>
>if (argc != 2) {
> shutItDown();
> }
>
>
> updated test passes
>
>
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
> \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
> \\
> -ea \\
> -esa \\
>
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
> \\
> com.sun.javatest.regtest.agent.MainWrapper
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...
Elif Aslan has updated the pull request incrementally with one additional
commit since the last revision:
Seperate out jspawnhelper tests
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18112/files
- new: https://git.openjdk.org/jdk/pull/18112/files/7a486eda..3f382e74
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=00-01
Stats: 124 lines in 3 files changed: 84 ins; 39 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18112.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112
PR: https://git.openjdk.org/jdk/pull/18112
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v3]
> This change is intended to address the segmentation fault issue that occurs
> when jspawnhelper is called without arguments, and it includes an updated
> test to verify the behavior in such cases.
>
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without
>
>if (argc != 2) {
> shutItDown();
> }
>
> code block
>
>> Test results: failed: 1
>> Report written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >> 1 0 1 0
>> >> <<
>> ==
>> TEST FAILURE
>
>
>
> after added the code block back
>
>if (argc != 2) {
> shutItDown();
> }
>
>
> updated test passes
>
>
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
> \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
> \\
> -ea \\
> -esa \\
>
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
> \\
> com.sun.javatest.regtest.agent.MainWrapper
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...
Elif Aslan has updated the pull request incrementally with one additional
commit since the last revision:
Use outputanalyzer
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18112/files
- new: https://git.openjdk.org/jdk/pull/18112/files/3f382e74..8fbf24e2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=01-02
Stats: 32 lines in 1 file changed: 1 ins; 21 del; 10 mod
Patch: https://git.openjdk.org/jdk/pull/18112.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112
PR: https://git.openjdk.org/jdk/pull/18112
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v4]
> This change is intended to address the segmentation fault issue that occurs
> when jspawnhelper is called without arguments, and it includes an updated
> test to verify the behavior in such cases.
>
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without
>
>if (argc != 2) {
> shutItDown();
> }
>
> code block
>
>> Test results: failed: 1
>> Report written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >> 1 0 1 0
>> >> <<
>> ==
>> TEST FAILURE
>
>
>
> after added the code block back
>
>if (argc != 2) {
> shutItDown();
> }
>
>
> updated test passes
>
>
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
> \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
> \\
> -ea \\
> -esa \\
>
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
> \\
> com.sun.javatest.regtest.agent.MainWrapper
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...
Elif Aslan has updated the pull request incrementally with one additional
commit since the last revision:
Revert JspawnhelperProtocol.java
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18112/files
- new: https://git.openjdk.org/jdk/pull/18112/files/8fbf24e2..ef803b4c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=02-03
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18112.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112
PR: https://git.openjdk.org/jdk/pull/18112
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v4]
On Thu, 7 Mar 2024 16:33:11 GMT, Elif Aslan wrote:
>> This change is intended to address the segmentation fault issue that occurs
>> when jspawnhelper is called without arguments,.
>> There is a new test added to verify the behavior in such cases.
>>
>> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test
>> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java`
>>
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
>> 1 1 0 0
>> ==
>> TEST SUCCESS
>
> Elif Aslan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert JspawnhelperProtocol.java
So, the test is "passing" with `argc == 2` because jspawnhelper shuts down on
illegal `argv[1]`, right? This is probably fine.
More stylistic comments:
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 31:
> 29: * @requires (os.family == "linux") | (os.family == "aix")
> 30: * @library /test/lib
> 31: * @run main/othervm/timeout=300 JspawnhelperWarnings
This test does not have to be `othervm`, it could be `driver`:
@run driver JspawnhelperWarnings
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 61:
> 59: public static void main(String[] args) throws Exception {
> 60: for (int nArgs = 0; nArgs < 10; ++nArgs)
> 61: jspawnhelperWithNArgs(nArgs);
Style: braces for loop body. There is also no point in using `++nArgs`, when
`nArgs++` is sufficient.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 63:
> 61: jspawnhelperWithNArgs(nArgs);
> 62: }
> 63: }
Needs newline at the end of the file.
-
PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1922949264
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516478694
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516481779
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516482020
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v3]
On Thu, 7 Mar 2024 16:29:12 GMT, Elif Aslan wrote:
>> This change is intended to address the segmentation fault issue that occurs
>> when jspawnhelper is called without arguments,.
>> There is a new test added to verify the behavior in such cases.
>>
>> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test
>> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java`
>>
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
>> 1 1 0 0
>> ==
>> TEST SUCCESS
>
> Elif Aslan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use outputanalyzer
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 238:
> 236: }
> 237: }
> 238: }
This looks like a leftover, please revert.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 46:
> 44:
> 45: private static void jspawnhelperWithNArgs(int nArgs) throws Exception
> {
> 46: System.out.println("Running jspawnhelper with "+nArgs+" args");
Style: spaces around `+` operator.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 47:
> 45: private static void jspawnhelperWithNArgs(int nArgs) throws Exception
> {
> 46: System.out.println("Running jspawnhelper with "+nArgs+" args");
> 47: String[] args = new String[nArgs +1];
Style: spaces around `+` operator.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 54:
> 52: if (!p.waitFor(TIMEOUT, TimeUnit.SECONDS)) {
> 53: throw new RuntimeException("jspawnhelper timed out after " +
> TIMEOUT + " seconds");
> 54: }
I don't think we need to wait here, the `shouldHaveExitValue` waits for us.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516472478
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516472833
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516477172
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516476881
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v5]
> This change is intended to address the segmentation fault issue that occurs > when jspawnhelper is called without arguments,. > There is a new test added to verify the behavior in such cases. > > `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` > > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java > 1 1 0 0 > == > TEST SUCCESS Elif Aslan has updated the pull request incrementally with one additional commit since the last revision: Address the comments - Changes: - all: https://git.openjdk.org/jdk/pull/18112/files - new: https://git.openjdk.org/jdk/pull/18112/files/ef803b4c..d34a1403 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=03-04 Stats: 9 lines in 1 file changed: 2 ins; 4 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112 PR: https://git.openjdk.org/jdk/pull/18112
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v6]
> This change is intended to address the segmentation fault issue that occurs > when jspawnhelper is called without arguments,. > There is a new test added to verify the behavior in such cases. > > `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` > > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java > 1 1 0 0 > == > TEST SUCCESS Elif Aslan has updated the pull request incrementally with one additional commit since the last revision: more styling fix - Changes: - all: https://git.openjdk.org/jdk/pull/18112/files - new: https://git.openjdk.org/jdk/pull/18112/files/d34a1403..fb9cb777 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=04-05 Stats: 8 lines in 1 file changed: 0 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112 PR: https://git.openjdk.org/jdk/pull/18112
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
> This change is intended to address the segmentation fault issue that occurs > when jspawnhelper is called without arguments,. > There is a new test added to verify the behavior in such cases. > > `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` > > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java > 1 1 0 0 > == > TEST SUCCESS Elif Aslan has updated the pull request incrementally with one additional commit since the last revision: Add args[0] back - Changes: - all: https://git.openjdk.org/jdk/pull/18112/files - new: https://git.openjdk.org/jdk/pull/18112/files/fb9cb777..c82fb77b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=05-06 Stats: 3 lines in 1 file changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112 PR: https://git.openjdk.org/jdk/pull/18112
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 17:13:12 GMT, Elif Aslan wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > Elif Aslan has updated the pull request incrementally with one additional > commit since the last revision: > > Add args[0] back I am good with this version. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1923081130
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 17:13:12 GMT, Elif Aslan wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > Elif Aslan has updated the pull request incrementally with one additional > commit since the last revision: > > Add args[0] back Marked as reviewed by vpetko (Author). - PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1923293793
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 17:13:12 GMT, Elif Aslan wrote:
>> This change is intended to address the segmentation fault issue that occurs
>> when jspawnhelper is called without arguments,.
>> There is a new test added to verify the behavior in such cases.
>>
>> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test
>> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java`
>>
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
>> 1 1 0 0
>> ==
>> TEST SUCCESS
>
> Elif Aslan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add args[0] back
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 29:
> 27: * @test
> 28: * @bug 8325567
> 29: * @requires (os.family == "linux") | (os.family == "aix")
Unless I'm mistaken, jspawn helper is used on Mac as well.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 36:
> 34: import java.nio.file.Paths;
> 35: import java.util.Arrays;
> 36: import java.util.concurrent.TimeUnit;
Unused import.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 56:
> 54: public static void main(String[] args) throws Exception {
> 55: for (int nArgs = 0; nArgs < 10; nArgs++) {
> 56: tryWithNArgs(nArgs);
Running with more than 3 arguments is unnecessary. Yes, its quick but just
burns cpu.
When running with 2 arguments, the failure mode is not due to the number of
arguments but is because argument 1 is formatted incorrectly; should be
`"%d:%d:%d"`. Though I supposed this falls into the "incorrect use category".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516736692
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516738195
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516733166
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 19:44:11 GMT, Roger Riggs wrote:
>> Elif Aslan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add args[0] back
>
> test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 56:
>
>> 54: public static void main(String[] args) throws Exception {
>> 55: for (int nArgs = 0; nArgs < 10; nArgs++) {
>> 56: tryWithNArgs(nArgs);
>
> Running with more than 3 arguments is unnecessary. Yes, its quick but just
> burns cpu.
>
> When running with 2 arguments, the failure mode is not due to the number of
> arguments but is because argument 1 is formatted incorrectly; should be
> `"%d:%d:%d"`. Though I supposed this falls into the "incorrect use category".
I wonder if it would make sense to add a test with a correct argument format?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516754742
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 19:47:55 GMT, Roger Riggs wrote: >> Elif Aslan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add args[0] back > > test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 29: > >> 27: * @test >> 28: * @bug 8325567 >> 29: * @requires (os.family == "linux") | (os.family == "aix") > > Unless I'm mistaken, jspawn helper is used on Mac as well. Yes indeed, it is used for all Unix OSes (that is, everything but Windows). - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517332878
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 20:04:13 GMT, Vladimir Petko wrote: > I wonder if it would make sense to add a test with a correct argument format? I would say that is what current jspawnhelper tests already test, and it is also tested implicitly with `Process` tests. Let's keep this test for testing that warning messages are printed on most common cases of misuse -- that is why `JspawnhelperWarnings` is a good name. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517458419
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Fri, 8 Mar 2024 07:35:27 GMT, Magnus Ihse Bursie wrote: >> test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 29: >> >>> 27: * @test >>> 28: * @bug 8325567 >>> 29: * @requires (os.family == "linux") | (os.family == "aix") >> >> Unless I'm mistaken, jspawn helper is used on Mac as well. > > Yes indeed, it is used for all Unix OSes (that is, everything but Windows). I think what matters for this test test most is which platforms we build `jspawnhelper` for, and those platforms indeed are: ifeq ($(call isTargetOs, macosx aix linux), true) $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \ So I'd say we just add `(os.family == "mac")` here. I would find it a bit weird to ask for `os.family != "windows"`, which would implicitly rely on exhaustiveness of current os family types. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517455696
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Fri, 8 Mar 2024 09:19:43 GMT, Aleksey Shipilev wrote: >> Yes indeed, it is used for all Unix OSes (that is, everything but Windows). > > I think what matters for this test test most is which platforms we build > `jspawnhelper` for, and those platforms indeed are: > > > ifeq ($(call isTargetOs, macosx aix linux), true) > $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \ > > > So I'd say we just add `(os.family == "mac")` here. I would find it a bit > weird to ask for `os.family != "windows"`, which would implicitly rely on > exhaustiveness of current os family types. Hm, that is not ideal code in the makefile. `jspawnhelper` is called from `src/java.base/unix/classes/java/lang/ProcessImpl.java`, so it is relied upon for all Unix implementation. Granted, this is currently the same as the list "macosx aix linux", but it would be better to express the same logic in the makefile as in the code. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517516188
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v8]
> This change is intended to address the segmentation fault issue that occurs > when jspawnhelper is called without arguments,. > There is a new test added to verify the behavior in such cases. > > `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` > > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java > 1 1 0 0 > == > TEST SUCCESS Elif Aslan has updated the pull request incrementally with one additional commit since the last revision: Address comments - Changes: - all: https://git.openjdk.org/jdk/pull/18112/files - new: https://git.openjdk.org/jdk/pull/18112/files/c82fb77b..ef321d2c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18112&range=06-07 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112 PR: https://git.openjdk.org/jdk/pull/18112
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Fri, 8 Mar 2024 10:10:23 GMT, Magnus Ihse Bursie wrote: >> I think what matters for this test test most is which platforms we build >> `jspawnhelper` for, and those platforms indeed are: >> >> >> ifeq ($(call isTargetOs, macosx aix linux), true) >> $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \ >> >> >> So I'd say we just add `(os.family == "mac")` here. I would find it a bit >> weird to ask for `os.family != "windows"`, which would implicitly rely on >> exhaustiveness of current os family types. > > Hm, that is not ideal code in the makefile. `jspawnhelper` is called from > `src/java.base/unix/classes/java/lang/ProcessImpl.java`, so it is relied upon > for all Unix implementation. Granted, this is currently the same as the list > "macosx aix linux", but it would be better to express the same logic in the > makefile as in the code. JDK-8327675 was just integrated, which replaces the build logic for jspawnhelper to check for "unix" instead of enumerating all our unixes. I suggest the same pattern should be used here. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1518152090
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v8]
On Fri, 8 Mar 2024 17:24:24 GMT, Elif Aslan wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > Elif Aslan has updated the pull request incrementally with one additional > commit since the last revision: > > Address comments LGTM - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1925740150
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v8]
On Fri, 8 Mar 2024 17:24:24 GMT, Elif Aslan wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > Elif Aslan has updated the pull request incrementally with one additional > commit since the last revision: > > Address comments lgtm - Marked as reviewed by eastigeevich (Committer). PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1925932313
