Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
May 19, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71173c7
[WFCORE-5869] Sanitize '|' symbol in standalone.bat
lvydra Apr 13, 2022
f4501b7
[WFCORE-5869] '|' symbol escaping tests
lvydra May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion core-feature-pack/common/src/main/resources/content/bin/standalone.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,22 @@ if exist "%STANDALONE_CONF%" (
echo Config file not found "%STANDALONE_CONF%"
)

rem Sanitize JAVA_OPTS
rem Currently escaping only | characters any other might be added if needed
setlocal EnableDelayedExpansion

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@yersan yersan May 9, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
if not "!IS_SANITIZED!" == "true" (
for %%C in (^|) do (
set "JAVA_OPTS=!JAVA_OPTS:%%C=^%%C!"
)
)

setlocal DisableDelayedExpansion

if NOT "x%DEBUG_PORT%" == "x" (
set DEBUG_PORT_VAR=%DEBUG_PORT%
)
Expand Down Expand Up @@ -228,7 +244,7 @@ for %%a in (!CONSOLIDATED_OPTS!) do (
)

rem If the -Djava.security.manager is found, enable the -secmgr and include a bogus security manager for JBoss Modules to replace
echo(!JAVA_OPTS! | findstr /r /c:"-Djava.security.manager" > nul && (
echo("!JAVA_OPTS!" | findstr /r /c:"-Djava.security.manager" > nul && (
echo ERROR: The use of -Djava.security.manager has been removed. Please use the -secmgr command line argument or SECMGR=true environment variable.
GOTO :EOF
)
Expand Down
49 changes: 49 additions & 0 deletions testsuite/scripts/src/test/java/org/wildfly/common/test/ServerConfigurator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.wildfly.common.test;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;
import java.io.UncheckedIOException;
Expand Down Expand Up @@ -251,6 +252,54 @@ private static String readStdout(final Path stdout) {
return error.toString();
}

public static void appendJavaOpts(final Path containerHome, final String baseName, final String variable) throws IOException {
final Path binDir = containerHome.resolve("bin");
if (TestSuiteEnvironment.isWindows()) {
Path conf = binDir.resolve(baseName + ".conf.bat");
OpenOption[] options;
if (Files.notExists(conf)) {
options = new OpenOption[]{StandardOpenOption.CREATE_NEW};
} else {
options = new OpenOption[]{StandardOpenOption.APPEND};
}
try (BufferedWriter writer = Files.newBufferedWriter(conf, options)) {
if ("standalone".equals(baseName)) {
writer.newLine();
writer.write("set \"JAVA_OPTS=%JAVA_OPTS% " + variable +"\"");
writer.newLine();
}
}
}
}

public static void removeJavaOpts(final Path containerHome, final String baseName, final String variable) throws IOException {
final Path binDir = containerHome.resolve("bin");
if (TestSuiteEnvironment.isWindows()) {
Path conf = binDir.resolve(baseName + ".conf.bat");
Path confTemp = binDir.resolve(baseName + "Temp" + ".conf.bat");
OpenOption[] options = new OpenOption[]{StandardOpenOption.CREATE_NEW};
BufferedReader reader = Files.newBufferedReader(conf);
BufferedWriter writer = Files.newBufferedWriter(confTemp, options);

String lineToRemove = "set \"JAVA_OPTS=%JAVA_OPTS% " + variable +"\"";
String currentLine;
while ((currentLine = reader.readLine()) != null) {
String trimmedLine = currentLine.trim();

if (trimmedLine.isEmpty() || trimmedLine.equals(lineToRemove)) continue;

writer.newLine();
writer.write(currentLine);
writer.newLine();
}
writer.close();
reader.close();
Files.delete(conf);
Files.copy(confTemp, conf);
Files.delete(confTemp);
}
}

private static void appendConf(final Path containerHome, final String baseName, final String envName) throws IOException {
// Add the local maven repository to the conf file
final String localRepo = System.getProperty("maven.repo.local");
Expand Down
27 changes: 26 additions & 1 deletion testsuite/scripts/src/test/java/org/wildfly/scripts/test/StandaloneScriptTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@
import org.jboss.dmr.ModelNode;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameter;
import org.junit.runners.Parameterized.Parameters;
import org.wildfly.common.test.ServerConfigurator;
import org.wildfly.common.test.ServerHelper;
import org.wildfly.common.test.LoggingAgent;

Expand All @@ -54,6 +56,7 @@
@RunWith(Parameterized.class)
public class StandaloneScriptTestCase extends ScriptTestCase {
private static final boolean MODULAR_JVM;
private static final String STANDALONE_BASE_NAME = "standalone";

static {
final String javaSpecVersion = System.getProperty("java.specification.version");
Expand All @@ -74,7 +77,7 @@ public class StandaloneScriptTestCase extends ScriptTestCase {
private static final Function<ModelControllerClient, Boolean> STANDALONE_CHECK = ServerHelper::isStandaloneRunning;

public StandaloneScriptTestCase() {
super("standalone");
super(STANDALONE_BASE_NAME);
}

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

@Test
public void testBatchScriptJavaOptsToEscape() throws Exception {
Assume.assumeTrue(TestSuiteEnvironment.isWindows());
final String variableToEscape = "-Dhttp.nonProxyHosts=localhost|127.0.0.1|10.10.10.*";
ServerConfigurator.appendJavaOpts(ServerHelper.JBOSS_HOME, "standalone", variableToEscape);
try (ScriptProcess script = new ScriptProcess(ServerHelper.JBOSS_HOME, STANDALONE_BASE_NAME, Shell.BATCH, ServerHelper.TIMEOUT)) {
testScript(script);
}
ServerConfigurator.removeJavaOpts(ServerHelper.JBOSS_HOME, "standalone", variableToEscape);
}

@Test
public void testBatchScriptJavaOptsEscaped() throws Exception {
Assume.assumeTrue(TestSuiteEnvironment.isWindows());
final String escapedVariable = "-Dhttp.nonProxyHosts=localhost^|127.0.0.1^|10.10.10.*";
ServerConfigurator.appendJavaOpts(ServerHelper.JBOSS_HOME, STANDALONE_BASE_NAME, escapedVariable);
try (ScriptProcess script = new ScriptProcess(ServerHelper.JBOSS_HOME, STANDALONE_BASE_NAME, Shell.BATCH, ServerHelper.TIMEOUT)) {
testScript(script);
}
ServerConfigurator.removeJavaOpts(ServerHelper.JBOSS_HOME, STANDALONE_BASE_NAME, escapedVariable);
}

@Override
void testScript(final ScriptProcess script) throws InterruptedException, TimeoutException, IOException {
// This is an odd case for Windows where with the -Xlog:gc* where the file argument does not seem to work with
Expand Down