[WFCORE-5869] standalone.bat script does not parse JAVA_OPTS containing '|' symbol properly #5056

lvydra · 2022-04-19T07:29:28Z

https://issues.redhat.com/browse/WFCORE-5869

wildfly-ci · 2022-04-19T11:14:21Z

Core - Full Integration Build 11490 outcome was FAILURE using a merge of 71173c7
Summary: Tests failed: 1 (1 new), passed: 7224, ignored: 140 Build time: 03:42:53

Failed tests

org.jboss.as.test.integration.messaging.jms.external.prefix.ExternalJMSDestinationDefinitionMessagingDeploymentTestCase.testSendMessage: java.io.IOException: java.util.concurrent.ExecutionException: java.io.IOException: HTTP Status 500 Response: <html><head><title>ERROR</title><style>
body {
    font-family: "Lucida Grande", "Lucida Sans Unicode", "Trebuchet MS", Helvetica, Arial, Verdana, sans-serif;
    margin: 5px;
}

.header {
    background-image: linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    background-image: -o-linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    background-image: -moz-linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    background-image: -webkit-linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    background-image: -ms-linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    
    background-image: -webkit-gradient(
        linear,
        left bottom,
        left top,
        color-stop(0.08, rgb(153,151,153)),
        color-stop(0.54, rgb(199,199,199))
    );
    color: black;
    padding: 2px;
    font-weight: normal;
    border: solid 1px;
    font-size: 170%;
    text-align: left;
    vertical-align: middle; 
    height: 32px; 
    margin-bottom: 10px;
}
.error-div {
    display: inline-block;
    width: 32px;
    height: 32px;
    background: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAABmJLR0QAAAAAAAD5Q7t/AAAACXBIWXMAAABIAAAASABGyWs+AAAACXZwQWcAAAAgAAAAIACH+pydAAAGGElEQVRYw8WXW2wcVxnHf+fM7NXrdXbdtZ3ipGqCEzvQKmlFoUggwkvvGBoaqVVV5Y0nkCoQPPCAhMQbQlzUh/IWHoh6QUVKZVLxUCGogKZRW9uJkyg0iVzZ2dhre9c7u7M7c87hYWc2M7t2bBASI306O/85+v7/7zLn24H/8yV2u/EcFNvwVNK2X0DKw1qpYaX1gCWlY0m5arRe8JQ6I2DmaVj/nwk4CxO2Zf1aw9dKo6N67+eOZHN7x0gViiSHBmlXa7hrazjLt1i+eKmxUi5bwpg/e8a8PA3X/msBr0MyJ+WvgFOffehY8v7HH5OW78HqGjgOtNrge2AnIJWAgQEYLqBsm3/NvKM/mZ1rYczpRa2/9x3w/iMBM1BCiHfuGRmZevClF9NJ5cP1m9B0+/aa3t/ZDNw3TtuSfPy737trK6sXtTGPPwOruxLwNhSEEHMHpqZGDz93wubKFdio3ZXUGNOHs2cIDh/g6pt/9G8uXLnlGPP5k1Dt5bN60m6lhXj34OTkxKET37SZm4d6oz/KgFRH7yOrBozbwlTWGf7ql6RZrWTcytrxaTj9Ro9OGb3JS/mz0nDxgYnnvmWbuUsdJ6FTY2JmImShqZA83N90UbOXOXDiG4nCcPFoTsqfbluCP8DenJQLx1/+7pBcXILaZl9qo2svFmalb48xkM9hxkf5+29+u9HSenIayn0ZyEr5i0NHpnKy7aGrtW6UehsLozRBRlRQEh3BwzLpWh3h++yfPJRLSvnzvhK8DkPamOl7n3nCUjcXtyXuK0X0WViaAFc9QtSNJe594uu2MubZGcjHBGTgqeFiQQml0K12nDgSpd4iyi4uRMci6Q8xJSWq7SGEZCg/qD14LCYgIcTzoxMHc6qygYk40aETKTFSdh2aANOWhbEsjJSYCK6C/V0LcH+9xvCB/TlbiBdCAXbQMJPZ/fvQjUbXEULAa6/1deyuh0dwNU+e7DSzEOhmi8xn9iI+ujgVE6ChlBwpYcqVjuIgA3IXBMYYtNbd30IIpJQI0ZEa+gPQnk9ieA8GSrESaGNyiWIBrVQntUHqdrq01iiluo0WilBK4fs+WutOr4S9YAyJoRzamMGYAClEve046ETyTr13EKCU6kZ+N4GNCxfwKpVOryQSuOUVhBCbsRIIuN1cWRkaSCfRrdaOTncijgltNPDn5xHZLIn7x3GNh4CVmADgcv3a9YmBY0cxm06nAQH31KlOao3BL5fxbtzAOE73hAt7JXb6RU7PGO44WK5Do1ZHw6WYAN+YM+X5S8dLXziWizVNvY5aWcFfWuq8Idsctb1Yr8DwmRwrUr1yfVMbcyYmwIWZ2mbdai9+gvfBAsaAbrUwrrtllLuJPibIGGQ2hUlIHLdlA+diTXgSqkKIt5b/OevL+8bwqlWU63anW3jyhaa4c9T2Hs0hrsL5EOCJQ/sov7/gS8Mb07AZEwDgav2D5aVy3eSziEL+Dllk0PSRcmcMqx6BUeGykINsksrttboPP4w2aVfACVhGiFevvft+I/XwJCad3DF6tc2MiD4zmRSZhyZY/OusI4V4JTqKYwIA6lr/2Gm6s5/+7UMv/egDmEyqL7VbRR/LViR6sikGvniYpffm2m6r/VFd65/0vqZ9R/tbsMeG+WKpMDL+lWMJ58Jl/NVqvOG26PAoDmCXhhg4epDl9+a9zUp1uQkPbvWfcMvZchbuEXAuk0kf2X/84YxpuDQv3kA5zW1fw9CswSyZyXFEJsmnf/m46bntuTY8+SxUtuLadri9CokxeEUK8WJhtJgaeeSIxG3RurWGv+Gg3RbaU5CwkOkU1lCWxFgBmbS4ff6qX7297rXh9Gn4/llobMcjtsFywACQ+zZMTsOP8vBIOpNS+bFiJj2cxx7MYg+k8ZwmrWqD9lqNjVvrTc9tWRtw/k345dtwFagDTmTdUUA2EDAYXffB2JPw6FH4ch7GUpCzIemD50J9A1Y+hH/8Cc4vdTq9Tud9D9dNOt+MajclyNL53xZmIhusmcBSQJLOd4UJnHqACzTppDy0kHizl/yuPRB5nggIM0A6IE4GuA34EQFtoBUQuwGm7kbwb+eaEEXmuV5dAAAAJXRFWHRjcmVhdGUtZGF0ZQAyMDA5LTExLTEwVDE5OjM4OjI0LTA3OjAwdDKp4gAAACV0RVh0ZGF0ZTpjcmVhdGUAMjAxMC0wMi0yMFQyMzoyNjoyNC0wNzowMC7DUNYAAAAldEVYdGRhdGU6bW9kaWZ5ADIwMTAtMDEtMTFUMDg6NTc6MzUtMDc6MDCruapPAAAAMnRFWHRMaWNlbnNlAGh0dHA6Ly9lbi53aWtpcGVkaWEub3JnL3dpa2kvUHVibGljX2RvbWFpbj/96s8AAAAldEVYdG1vZGlmeS1kYXRlADIwMDktMTEtMTBUMTk6Mzg6MjQtMDc6MDArg9/WAAAAGXRFWHRTb3VyY2UAVGFuZ28gSWNvbiBMaWJyYXJ5VM/tggAAADp0RVh0U291cmNlX1VSTABodHRwOi8vdGFuZ28uZnJlZWRlc2t0b3Aub3JnL1RhbmdvX0ljb25fTGlicmFyebzIrdYAAAAASUVORK5CYII=') left center no-repeat;
}
.error-text-div {
    display: inline-block;
    vertical-align: top;
    height: 32px;
}
.label {
    font-weight:bold;
    display: inline-block;
}
.value {
    display: inline-block;
    margin-left: 5px;
}
pre {
    font-size: 110%;

yersan · 2022-05-09T12:46:34Z

core-feature-pack/common/src/main/resources/content/bin/standalone.bat

+
+rem If characters are already escaped then IS_SANITIZED is set to true and JAVA__OPTS are left as they are
+for %%C in (^^^|) do (
+  if not "!JAVA_OPTS:%%C=!"=="!JAVA_OPTS!" set IS_SANITIZED=true


This is done character by character, so only the last occurrence will count to know whether it is sanitized. @lvydra Is this intentional?

Well, as soon as one is scaped it counts as completely sanitized. That should be fine, @lvydra, we are not going to treat scaped and non-scaped characters. If only one is scaped and not the others, the script will fail. Maybe it can be controlled better. I'm approving from my side

Hi @yersan, thanks for the review, yes it is intentional as the main purpose is to avoid escaping already escaped characters and breaking that way script execution, even for the cost of leaving characters unescaped.

However, I can try to improve it to handle those cases more complexly.

Thanks, @lvydra , let's wait for James' input here. I think we are fine with what we have; now it can accept '|' or '^|' but not both at the same time, that could be a corner case and it is unlikely to happen.

I've got no real strong opinion here as I cannot find the documentation as to how this works in batch scripts :) I trust whatever you guys feel is correct.

Thanks, @jamezp , I had no idea we had those kinds of tests. For this one, I found this useful: https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/for

@lvydra Could you take a look at whether we can add a test case for this one?. Code works, but a test case will ensure we will not get a regression. Let us know your findings, thanks!

Thanks @jamezp, sure @yersan I will look at the tests.

yersan

@jamezp Could you also take a look?

jamezp

The only thing I can think of that would be helpful is to add a test for this in the testsuite/scripts tests. Something could probably be added here

wildfly-core/testsuite/scripts/src/test/java/org/wildfly/common/test/ServerConfigurator.java

Line 254 in 71173c7

    
           private static void appendConf(final Path containerHome, final String baseName, final String envName) throws IOException {

.

jamezp · 2022-05-10T20:41:01Z

core-feature-pack/common/src/main/resources/content/bin/standalone.bat

+
+rem If characters are already escaped then IS_SANITIZED is set to true and JAVA__OPTS are left as they are
+for %%C in (^^^|) do (
+  if not "!JAVA_OPTS:%%C=!"=="!JAVA_OPTS!" set IS_SANITIZED=true


I've got no real strong opinion here as I cannot find the documentation as to how this works in batch scripts :) I trust whatever you guys feel is correct.

lvydra · 2022-05-13T13:27:30Z

Hi @jamezp @yersan, tests added.

yersan · 2022-05-18T17:30:02Z

testsuite/scripts/src/test/java/org/wildfly/scripts/test/StandaloneScriptTestCase.java

@@ -86,6 +89,28 @@ public static Collection<Object> data() {
        return result;
    }

+    @Test
+    public void testBatchScriptJavaOptsToEscape() throws Exception {
+        Assume.assumeTrue(!TestSuiteEnvironment.isWindows() && Shell.BASH.isSupported());


The assume condition needs to be fixed; it should be: Assume.assumeTrue(TestSuiteEnvironment.isWindows())

Although the test should pass on Linux, we don't need to run it there, indeed, looking at the method name, that was not your original intention.

Oh sure, thanks @yersan, I don't know how did it get there :-)

:) thanks, @lvydra

yersan · 2022-05-18T17:30:56Z

testsuite/scripts/src/test/java/org/wildfly/scripts/test/StandaloneScriptTestCase.java

+
+    @Test
+    public void testBatchScriptJavaOptsEscaped() throws Exception {
+        Assume.assumeTrue(!TestSuiteEnvironment.isWindows() && Shell.BASH.isSupported());


The assume condition needs to be fixed: Assume.assumeTrue(TestSuiteEnvironment.isWindows())

lvydra · 2022-05-18T18:39:48Z

@yersan fixed

wildfly-ci · 2022-05-18T22:20:06Z

Core - Full Integration Build 11594 outcome was FAILURE using a merge of f4501b7
Summary: Tests failed: 2 (2 new), passed: 7222, ignored: 150 Build time: 03:41:51

Failed tests

org.wildfly.extension.undertow.ServerServiceTestCase.testDefinedDefaultAttributes1: java.lang.AssertionError: expected:<1> but was:<0>
	at org.wildfly.extension.undertow.ServerServiceTestCase.testDefinedDefaultAttributes1(ServerServiceTestCase.java:111)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)


org.wildfly.extension.undertow.ServerServiceTestCase.testDefinedDefaultAttributes2: java.lang.AssertionError: expected:<1> but was:<0>
	at org.wildfly.extension.undertow.ServerServiceTestCase.testDefinedDefaultAttributes2(ServerServiceTestCase.java:126)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

[WFCORE-5869] Sanitize '|' symbol in standalone.bat

71173c7

github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Apr 19, 2022

yersan reviewed May 9, 2022

View reviewed changes

yersan approved these changes May 9, 2022

View reviewed changes

jamezp reviewed May 10, 2022

View reviewed changes

lvydra force-pushed the WFCORE-5869 branch from 1d7169e to 1220689 Compare May 13, 2022 13:24

yersan requested changes May 18, 2022

View reviewed changes

[WFCORE-5869] '|' symbol escaping tests

f4501b7

lvydra force-pushed the WFCORE-5869 branch from b6544ae to f4501b7 Compare May 18, 2022 18:39

yersan approved these changes May 19, 2022

View reviewed changes

yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label May 19, 2022

yersan merged commit b4053f1 into wildfly:main May 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFCORE-5869] standalone.bat script does not parse JAVA_OPTS containing '|' symbol properly #5056

[WFCORE-5869] standalone.bat script does not parse JAVA_OPTS containing '|' symbol properly #5056

lvydra commented Apr 19, 2022

wildfly-ci commented Apr 19, 2022

yersan May 9, 2022

yersan May 9, 2022 •

edited

lvydra May 9, 2022

lvydra May 9, 2022

yersan May 9, 2022

jamezp May 10, 2022

yersan May 11, 2022

lvydra May 11, 2022

yersan left a comment

jamezp left a comment

jamezp May 10, 2022

lvydra commented May 13, 2022

yersan May 18, 2022

lvydra May 18, 2022 •

edited

yersan May 19, 2022

yersan May 18, 2022

lvydra commented May 18, 2022

wildfly-ci commented May 18, 2022

[WFCORE-5869] standalone.bat script does not parse JAVA_OPTS containing '|' symbol properly #5056

[WFCORE-5869] standalone.bat script does not parse JAVA_OPTS containing '|' symbol properly #5056

Conversation

lvydra commented Apr 19, 2022

wildfly-ci commented Apr 19, 2022

Failed tests

Choose a reason for hiding this comment

yersan May 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yersan left a comment

Choose a reason for hiding this comment

jamezp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvydra commented May 13, 2022

Choose a reason for hiding this comment

lvydra May 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvydra commented May 18, 2022

wildfly-ci commented May 18, 2022

Failed tests

yersan May 9, 2022 •

edited

lvydra May 18, 2022 •

edited