Skip to content

Commit

Permalink
8303392: Runtime.exec and ProcessBuilder.start should use System logger
Browse files Browse the repository at this point in the history
Reviewed-by: stuefe, alanb, mullan
  • Loading branch information
Roger Riggs committed Mar 29, 2023
1 parent be764a7 commit d063b89
Show file tree
Hide file tree
Showing 6 changed files with 272 additions and 0 deletions.
35 changes: 35 additions & 0 deletions src/java.base/share/classes/java/lang/ProcessBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@

public final class ProcessBuilder
{
// Lazily and racy initialize when needed, racy is ok, any logger is ok
private static System.Logger LOGGER;

private List<String> command;
private File directory;
private Map<String,String> environment;
Expand Down Expand Up @@ -1067,6 +1070,19 @@ public ProcessBuilder redirectErrorStream(boolean redirectErrorStream) {
*
* @throws IOException if an I/O error occurs
*
* @implNote
* In the reference implementation, logging of the command, arguments, directory,
* stack trace, and process id can be enabled.
* The logged information may contain sensitive security information and the potential exposure
* of the information should be carefully reviewed.
* Logging of the information is enabled when the logging level of the
* {@linkplain System#getLogger(String) system logger} named {@code java.lang.ProcessBuilder}
* is {@link System.Logger.Level#DEBUG Level.DEBUG} or {@link System.Logger.Level#TRACE Level.TRACE}.
* When enabled for {@code Level.DEBUG} only the process id, directory, command, and stack trace
* are logged.
* When enabled for {@code Level.TRACE} the arguments are included with the process id,
* directory, command, and stack trace.
*
* @see Runtime#exec(String[], String[], java.io.File)
*/
public Process start() throws IOException {
Expand Down Expand Up @@ -1119,6 +1135,21 @@ private Process start(Redirect[] redirects) throws IOException {
event.pid = process.pid();
event.commit();
}
// Racy initialization for logging; errors in configuration may throw exceptions
System.Logger logger = LOGGER;
if (logger == null) {
LOGGER = logger = System.getLogger("java.lang.ProcessBuilder");
}
if (logger.isLoggable(System.Logger.Level.DEBUG)) {
boolean detail = logger.isLoggable(System.Logger.Level.TRACE);
var level = (detail) ? System.Logger.Level.TRACE : System.Logger.Level.DEBUG;
var cmdargs = (detail) ? String.join("\" \"", cmdarray) : cmdarray[0];
RuntimeException stackTraceEx = new RuntimeException("ProcessBuilder.start() debug");
LOGGER.log(level, "ProcessBuilder.start(): pid: " + process.pid() +
", dir: " + dir +
", cmd: \"" + cmdargs + "\"",
stackTraceEx);
}
return process;
} catch (IOException | IllegalArgumentException e) {
String exceptionInfo = ": " + e.getMessage();
Expand Down Expand Up @@ -1263,6 +1294,10 @@ private Process start(Redirect[] redirects) throws IOException {
* @throws UnsupportedOperationException
* If the operating system does not support the creation of processes
*
* @implNote
* In the reference implementation, logging of each process created can be enabled,
* see {@link ProcessBuilder#start()} for details.
*
* @throws IOException if an I/O error occurs
* @since 9
*/
Expand Down
24 changes: 24 additions & 0 deletions src/java.base/share/classes/java/lang/Runtime.java
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,10 @@ public void halt(int status) {
* @throws IllegalArgumentException
* If {@code command} is empty
*
* @implNote
* In the reference implementation, logging of the created process can be enabled,
* see {@link ProcessBuilder#start()} for details.
*
* @see #exec(String[], String[], File)
* @see ProcessBuilder
*/
Expand Down Expand Up @@ -397,6 +401,10 @@ public Process exec(String command) throws IOException {
* @throws IllegalArgumentException
* If {@code command} is empty
*
* @implNote
* In the reference implementation, logging of the created process can be enabled,
* see {@link ProcessBuilder#start()} for details.
*
* @see #exec(String[], String[], File)
* @see ProcessBuilder
*/
Expand Down Expand Up @@ -458,6 +466,10 @@ public Process exec(String command, String[] envp) throws IOException {
* @throws IllegalArgumentException
* If {@code command} is empty
*
* @implNote
* In the reference implementation, logging of the created process can be enabled,
* see {@link ProcessBuilder#start()} for details.
*
* @see ProcessBuilder
* @since 1.3
*/
Expand Down Expand Up @@ -503,6 +515,10 @@ public Process exec(String command, String[] envp, File dir)
* If {@code cmdarray} is an empty array
* (has length {@code 0})
*
* @implNote
* In the reference implementation, logging of the created process can be enabled,
* see {@link ProcessBuilder#start()} for details.
*
* @see ProcessBuilder
*/
public Process exec(String[] cmdarray) throws IOException {
Expand Down Expand Up @@ -546,6 +562,10 @@ public Process exec(String[] cmdarray) throws IOException {
* If {@code cmdarray} is an empty array
* (has length {@code 0})
*
* @implNote
* In the reference implementation, logging of the created process can be enabled,
* see {@link ProcessBuilder#start()} for details.
*
* @see ProcessBuilder
*/
public Process exec(String[] cmdarray, String[] envp) throws IOException {
Expand Down Expand Up @@ -641,6 +661,10 @@ public Process exec(String[] cmdarray, String[] envp) throws IOException {
* If {@code cmdarray} is an empty array
* (has length {@code 0})
*
* @implNote
* In the reference implementation, logging of the created process can be enabled,
* see {@link ProcessBuilder#start()} for details.
*
* @see ProcessBuilder
* @since 1.3
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
############################################################
# Enable logging java.lang.ProcessBuilder to the console
############################################################

handlers= java.util.logging.ConsoleHandler

java.util.logging.ConsoleHandler.level = ALL
java.lang.ProcessBuilder.level = FINE
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
############################################################
# Enable logging java.lang.ProcessBuilder to the console
############################################################

handlers= java.util.logging.ConsoleHandler

java.util.logging.ConsoleHandler.level = ALL
java.lang.ProcessBuilder.level = FINER
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
############################################################
# Enable logging java.lang.ProcessBuilder to the console
############################################################

handlers= java.util.logging.ConsoleHandler

java.util.logging.ConsoleHandler.level = ALL
java.lang.ProcessBuilder.level = INFO
189 changes: 189 additions & 0 deletions test/jdk/java/lang/ProcessBuilder/ProcessStartLoggingTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

import java.io.BufferedReader;
import java.io.File;
import java.io.IOException;
import java.nio.file.Path;
import java.util.List;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import java.util.logging.StreamHandler;
import java.util.stream.Stream;

import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.ParameterizedTest;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;

/*
* @test
* @summary verify logging of ProcessBuilder.start()
* @run junit/othervm ProcessStartLoggingTest
*/
public class ProcessStartLoggingTest {

private final static int NORMAL_STATUS = 0;
private final static int ERROR_STATUS = 1;

private static final String TEST_JDK = System.getProperty("test.jdk");
private static final String TEST_SRC = System.getProperty("test.src");

private static Object HOLD_LOGGER;

/**
* Launch a process with the arguments.
* @param args 1 or strings passed directly to ProcessBuilder as command and arguments.
*/
public static void main(String[] args) throws InterruptedException {
if (System.getProperty("ThrowingHandler") != null) {
HOLD_LOGGER = ProcessStartLoggingTest.ThrowingHandler.installHandler();
}
String directory = System.getProperty("directory");
try {
ProcessBuilder pb = new ProcessBuilder(args);
pb.directory((directory == null) ? null : new File(directory));
Process p = pb.start();
int status = p.waitFor();
if (status != 0) {
System.out.println("exitValue: " + status);
}
} catch (IOException ioe) {
System.out.println("ProcessBuilder.start() threw IOException: " + ioe);
}
}

/**
* Test various log level settings, and none.
* @return a stream of arguments for parameterized test
*/
private static Stream<Arguments> logParamProvider() {

return Stream.of(
// Logging enabled with level TRACE
Arguments.of(List.of("-Djava.util.logging.config.file=" +
Path.of(TEST_SRC, "ProcessLogging-FINER.properties").toString(),
"-Ddirectory=."),
List.of("echo", "echo0"),
NORMAL_STATUS,
"dir: ., cmd: \"echo\" \"echo0\""),
// Logging enabled with level DEBUG
Arguments.of(List.of("-Djava.util.logging.config.file=" +
Path.of(TEST_SRC, "ProcessLogging-FINE.properties").toString(),
"-Ddirectory=."),
List.of("echo", "echo1"),
NORMAL_STATUS,
"dir: ., cmd: \"echo\""),
// Logging disabled due to level INFO
Arguments.of(List.of("-Djava.util.logging.config.file=" +
Path.of(TEST_SRC, "ProcessLogging-INFO.properties").toString()),
List.of("echo", "echo2"),
NORMAL_STATUS,
""),
// Console logger DEBUG
Arguments.of(List.of("--limit-modules", "java.base",
"-Djdk.system.logger.level=DEBUG"),
List.of("echo", "echo3"),
NORMAL_STATUS,
"dir: null, cmd: \"echo\""),
// Console logger TRACE
Arguments.of(List.of("--limit-modules", "java.base",
"-Djdk.system.logger.level=TRACE",
"-Ddirectory=."),
List.of("echo", "echo4"),
NORMAL_STATUS,
"dir: ., cmd: \"echo\" \"echo4\""),
// No Logging configured
Arguments.of(List.of(),
List.of("echo", "echo5"),
NORMAL_STATUS,
""),
// Throwing Handler
Arguments.of(List.of("-DThrowingHandler",
"-Djava.util.logging.config.file=" +
Path.of(TEST_SRC, "ProcessLogging-FINE.properties").toString()),
List.of("echo", "echo6"),
ERROR_STATUS,
"Exception in thread \"main\" java.lang.RuntimeException: Exception in publish")
);
}

/**
* Check that the logger output of a launched process contains the expected message.
*
* @param logArgs Arguments to configure logging in the java test process
* @param childArgs the args passed to the child to be invoked as a Process
* @param expectMessage log should contain the message
*/
@ParameterizedTest
@MethodSource("logParamProvider")
public void checkLogger(List<String> logArgs, List<String> childArgs,
int expectedStatus, String expectMessage) {
ProcessBuilder pb = new ProcessBuilder();
pb.redirectErrorStream(true);

List<String> cmd = pb.command();
cmd.add(Path.of(TEST_JDK,"bin", "java").toString());
cmd.addAll(logArgs);
cmd.add(this.getClass().getName());
cmd.addAll(childArgs);
try {
Process process = pb.start();
try (BufferedReader reader = process.inputReader()) {
List<String> lines = reader.lines().toList();
boolean match = (expectMessage.isEmpty())
? lines.size() == 0
: lines.stream().filter(s -> s.contains(expectMessage)).findFirst().isPresent();
if (!match) {
// Output lines for debug
System.err.println("Expected> \"" + expectMessage + "\"");
lines.forEach(l -> System.err.println("Actual> \"" + l+ "\""));
fail("Unexpected log contents");
}
}
int result = process.waitFor();
assertEquals(expectedStatus, result, "Exit status");
} catch (IOException | InterruptedException ex) {
fail(ex);
}
}

/**
* A LoggingHandler that throws an Exception.
*/
public static class ThrowingHandler extends StreamHandler {

// Install this handler for java.lang.ProcessBuilder
public static Logger installHandler() {
Logger logger = Logger.getLogger("java.lang.ProcessBuilder");
logger.addHandler(new ProcessStartLoggingTest.ThrowingHandler());
return logger;
}

@Override
public synchronized void publish(LogRecord record) {
throw new RuntimeException("Exception in publish");
}
}
}

1 comment on commit d063b89

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.