diff --git a/src/main/java/emissary/place/MultiFileUnixCommandPlace.java b/src/main/java/emissary/place/MultiFileUnixCommandPlace.java index 1ac648a038..a419656a3f 100755 --- a/src/main/java/emissary/place/MultiFileUnixCommandPlace.java +++ b/src/main/java/emissary/place/MultiFileUnixCommandPlace.java @@ -8,6 +8,7 @@ import emissary.directory.KeyManipulator; import emissary.kff.KffDataObjectHandler; import emissary.util.shell.Executrix; +import emissary.util.shell.TempFileNames; import java.io.File; import java.io.IOException; @@ -630,13 +631,12 @@ protected List processData(@Nullable IBaseDataObject tData, int return sprouts; } - // make the directory and write the input file. - String[] names; File f = null; int result = -1; try { - names = executrix.writeDataToNewTempDir(tData.data(), start, len); - f = new File(names[Executrix.INPATH]); + // make the directory and write the input file. + TempFileNames names = executrix.writeInputDataToNewTempDir(tData.data(), start, len); + f = new File(names.getInputFilename()); logger.debug("Wrote file out to {}", f.getPath()); // Create the command string and run it @@ -677,7 +677,7 @@ protected List processData(@Nullable IBaseDataObject tData, int } } - // If there was not result, then report it in 2 places. + // If there was no result, then report it in 2 places. if (sprouts.isEmpty()) { logger.debug("Command failed. nothing to sprout for file: result={}", result); tData.addProcessingError("ERROR in " + placeName + ". Exec returned errno " + result); diff --git a/src/main/java/emissary/place/UnixCommandPlace.java b/src/main/java/emissary/place/UnixCommandPlace.java index aa28dc5016..aa95cc136a 100755 --- a/src/main/java/emissary/place/UnixCommandPlace.java +++ b/src/main/java/emissary/place/UnixCommandPlace.java @@ -5,6 +5,7 @@ import emissary.core.ResourceException; import emissary.directory.KeyManipulator; import emissary.util.shell.Executrix; +import emissary.util.shell.TempFileNames; import java.io.File; import java.io.IOException; @@ -278,10 +279,10 @@ protected void unSynchronizedProcess(IBaseDataObject theDataObject) throws Resou */ @SuppressWarnings("CatchingUnchecked") protected byte[] runCommandOn(byte[] data) throws ResourceException { - String[] names = executrix.makeTempFilenames(); - String tempDirName = names[Executrix.DIR]; - String inputFileName = names[Executrix.INPATH]; - String outputFileName = names[Executrix.OUTPATH]; + TempFileNames names = executrix.createTempFilenames(); + String tempDirName = names.getTempDir(); + String inputFileName = names.getInputFilename(); + String outputFileName = names.getOutputFilename(); File tempDir = new File(tempDirName); byte[] outputData = null; diff --git a/src/main/java/emissary/util/shell/Executrix.java b/src/main/java/emissary/util/shell/Executrix.java index 6bfabfa5ee..ba85b76f5b 100755 --- a/src/main/java/emissary/util/shell/Executrix.java +++ b/src/main/java/emissary/util/shell/Executrix.java @@ -135,21 +135,35 @@ protected void configure(@Nullable final Configurator configGArg) { this.processMaxMillis = configG.findLongEntry("PROCESS_MAX_MILLIS", DEFAULT_PROCESS_MAX_MILLIS); } + /** + * Creates a set of temp file names (does not do any disk activity) + * + * @return new {@link TempFileNames} instance + */ + public TempFileNames createTempFilenames() { + return new TempFileNames(this.tmpDir, this.placeName, this.inFileEnding, this.outFileEnding); + } + /** * Make a set of temp file names (does not do any disk activity) + * + * @deprecated see {@link #createTempFilenames} */ + @Deprecated public String[] makeTempFilenames() { + final TempFileNames tfn = createTempFilenames(); + final String[] names = new String[7]; - final String dir = FileManipulator.mkTempFile(this.tmpDir, this.placeName); - final String base = Long.toString(System.nanoTime()); - names[DIR] = dir; - names[BASE] = base; - names[BASE_PATH] = dir + File.separator + base; - names[IN] = base + this.inFileEnding; - names[OUT] = base + this.outFileEnding; - names[INPATH] = dir + File.separator + base + this.inFileEnding; - names[OUTPATH] = dir + File.separator + base + this.outFileEnding; + names[DIR] = tfn.getTempDir(); + names[BASE] = tfn.getBase(); + names[BASE_PATH] = tfn.getBasePath(); + names[IN] = tfn.getIn(); + names[OUT] = tfn.getOut(); + names[INPATH] = tfn.getInputFilename(); + names[OUTPATH] = tfn.getOutputFilename(); + return names; + } /** @@ -890,7 +904,9 @@ private static void streamData(Process p, byte[] data) throws IOException { * * @param data the bytes to write * @return the tempNames structure that was created + * @deprecated see {@link #writeInputDataToNewTempDir(byte[])} */ + @Deprecated public String[] writeDataToNewTempDir(final byte[] data) { return writeDataToNewTempDir(data, 0, data.length); } @@ -902,7 +918,9 @@ public String[] writeDataToNewTempDir(final byte[] data) { * @param start offset in array to start writing * @param len length of data to write * @return the tempNames structure that was created + * @deprecated see {@link #writeInputDataToNewTempDir(byte[], int, int)} */ + @Deprecated public String[] writeDataToNewTempDir(final byte[] data, final int start, final int len) { final String[] tnames = makeTempFilenames(); writeDataToFile(data, start, len, tnames[INPATH], false); @@ -948,6 +966,31 @@ public File writeDataToNewTempDir(final String dirn, final byte[] data) { return writeDataToNewTempDir(data, dirn); } + + /** + * Write data out for processing into a new subdir under our configured temp area + * + * @param data the bytes to write + * @return the tempNames structure that was created + */ + public TempFileNames writeInputDataToNewTempDir(final byte[] data) { + return writeInputDataToNewTempDir(data, 0, data.length); + } + + /** + * Write data out for processing into a new subdir under our configured temp area + * + * @param data the bytes to write + * @param start offset in array to start writing + * @param len length of data to write + * @return the tempNames structure that was created + */ + public TempFileNames writeInputDataToNewTempDir(final byte[] data, final int start, final int len) { + final TempFileNames tnames = createTempFilenames(); + writeDataToFile(data, start, len, tnames.getInputFilename(), false); + return tnames; + } + /** * Gets the value of command that this instance will execute adding configured limits and configured paths to the * configuration value @@ -1008,6 +1051,57 @@ public String[] getCommand(final String[] tmpNames, final String commandArg, fin return new String[] {"/bin/sh", "-c", "ulimit -c 0; " + ulimitv + "cd " + tmpNames[DIR] + "; " + c}; } + /** + * Gets the value of command that this instance will execute adding configured limits and supplied paths to the + * configuration value + * + * @param tmpNames set of input/output directory names + * @return the value of command + */ + public String[] getCommand(TempFileNames tmpNames) { + return getCommand(tmpNames, getCommand(), this.cpuTimeLimit, this.vmSizeLimit); + } + + /** + * Gets the value of a command that can be executed adding configured limits and supplied paths to the configuration + * value + * + * @param tmpNames set of input/output directory names + * @param commandArg a command string to work with + * @return the value of command + */ + public String[] getCommand(final TempFileNames tmpNames, final String commandArg) { + return getCommand(tmpNames, commandArg, this.cpuTimeLimit, this.vmSizeLimit); + } + + /** + * Gets the value of a command that can be executed adding supplied limits and supplied paths to the configuration value + * The values in the command string that can be replaced are <INPUT_PATH>, <OUTPUT_PATH>, + * <INPUT_NAME>, and <OUTPUT_NAME>. On unix systems it is wrapped like + * /bin/sh -c ulimit -c 0; ulimit -v val; your command + * + * @param tmpNames set of input/output directory names + * @param commandArg a command string to work with + * @param cpuLimit the cpu limit for the ulimit command + * @param vmSzLimit for the ulimit command + * @return the value of command + */ + public String[] getCommand(final TempFileNames tmpNames, final String commandArg, final int cpuLimit, final int vmSzLimit) { + String c = commandArg; + c = c.replaceAll("", tmpNames.getInputFilename()); + c = c.replaceAll("", tmpNames.getOutputFilename()); + c = c.replaceAll("", tmpNames.getIn()); + c = c.replaceAll("", tmpNames.getOut()); + + // Run the command in shell limiting the core file size to 0 and the specified vm size + String ulimitv = ""; + if (!SystemUtils.IS_OS_MAC) { + ulimitv = "ulimit -v " + vmSzLimit + "; "; + } + return new String[] {"/bin/sh", "-c", "ulimit -c 0; " + ulimitv + "cd " + tmpNames.getTempDir() + "; " + c}; + } + + /** * Gets the value of a command that can be executed adding configured limits and supplied paths to the configuration * value @@ -1406,4 +1500,5 @@ public ProcessReader getStdOutProcessReader(Process p) { } + } diff --git a/src/main/java/emissary/util/shell/TempFileNames.java b/src/main/java/emissary/util/shell/TempFileNames.java new file mode 100644 index 0000000000..01e406d184 --- /dev/null +++ b/src/main/java/emissary/util/shell/TempFileNames.java @@ -0,0 +1,139 @@ +package emissary.util.shell; + +import emissary.util.io.FileManipulator; + +import java.io.File; + +/** + * A related set file and directory path names suitable for operations that require temporary disk-backed files. The + * names do not directly imply that corresponding files and directories actually exist; I/O operations including + * creating or deletions of files or directories is the responsibility of TempFileNames instance consumers. + */ +public class TempFileNames { + private final String tempDir; + private final String base; + private final String basePath; + private final String in; + private final String out; + private final String inputFilename; + private final String outputFilename; + + /** + * Creates a new TempFileNames instance, using the same logic as the legacy{@link Executrix#makeTempFilenames()} API + * + * @param tmpDir configured temp directory for the server process + * @param placeName place for which the names are needed + * @param inFileEnding input file ending + * @param outFileEnding output file ending + */ + TempFileNames(String tmpDir, String placeName, String inFileEnding, String outFileEnding) { + base = Long.toString(System.nanoTime()); + tempDir = FileManipulator.mkTempFile(tmpDir, placeName); + in = base + inFileEnding; + out = base + outFileEnding; + basePath = tempDir + File.separator + base; + inputFilename = basePath + inFileEnding; + outputFilename = basePath + outFileEnding; + } + + /** + * Temporary directory name for commands that use file i/o + *

+ * Corresponds to the {@link Executrix#DIR} usage in the legacy Executrix API + *

+ * + * @return temp directory name + */ + public String getTempDir() { + return tempDir; + } + + /** + * Pseudorandom value intended to help ensure unique temporary filenames + *

+ * Corresponds to the {@link Executrix#BASE} usage in the legacy Executrix API + *

+ * + * @return Pseudorandom key for this instance + * @deprecated this property should be considered internal-use only and will likely be removed + */ + @Deprecated + public String getBase() { + return base; + } + + /** + * Pseudorandom sub-path intended to help ensure unique temporary filenames + *

+ * Corresponds to the {@link Executrix#BASE_PATH} usage in the legacy Executrix API + *

+ * + * @return Pseudorandom sub-path + * @deprecated this property should be considered internal-use only and will likely be removed + */ + @Deprecated + public String getBasePath() { + return basePath; + } + + /** + * Pseudo-random value intended to help ensure unique input filenames + *

+ * Corresponds to the {@link Executrix#IN} usage in the legacy Executrix API + *

+ * + * @return Input filename path, relative to the {@link #getTempDir()} value + */ + public String getIn() { + return in; + } + + /** + * Pseudo-random value intended to help ensure unique output filenames + *

+ * Corresponds to the {@link Executrix#OUT} usage in the legacy Executrix API + *

+ * + * @return Output filename path, relative to the {@link #getTempDir()} value + */ + public String getOut() { + return out; + } + + /** + * Input filename for commands that use file input + *

+ * Corresponds to the {@link Executrix#INPATH} usage in the legacy Executrix API + *

+ * + * @return input filename + */ + public String getInputFilename() { + return inputFilename; + } + + /** + * Output filename for commands that generate file output + *

+ * Corresponds to the {@link Executrix#OUTPATH} usage in the legacy Executrix API + *

+ * + * @return Output filename + */ + public String getOutputFilename() { + return outputFilename; + } + + @Override + public String toString() { + return "TempFileNames{" + + "tempDir='" + tempDir + '\'' + + ", base='" + base + '\'' + + ", basePath='" + basePath + '\'' + + ", in='" + in + '\'' + + ", out='" + out + '\'' + + ", inputFilename='" + inputFilename + '\'' + + ", outputFilename='" + outputFilename + '\'' + + '}'; + } +} diff --git a/src/test/java/emissary/util/shell/ExecutrixTest.java b/src/test/java/emissary/util/shell/ExecutrixTest.java index c4cc09f56b..4d1d61af34 100644 --- a/src/test/java/emissary/util/shell/ExecutrixTest.java +++ b/src/test/java/emissary/util/shell/ExecutrixTest.java @@ -56,6 +56,7 @@ public void tearDown() throws Exception { } @Test + @SuppressWarnings("deprecation") // confirming functionality of legacy API void testExecutrixParams() { e.setInFileEnding(".in"); e.setOutFileEnding(".out"); @@ -75,7 +76,30 @@ void testExecutrixParams() { assertTrue(names[Executrix.IN].endsWith(".in"), "names must use in file ending"); } + /** + * Identical to {@link #testExecutrixParams}, but with the new TempFileNames API for constructing temp filenames + */ @Test + void testExecutrixParamsWithNewTempFileNamesApi() { + assertNotNull(e, "Executrix instance should not be null"); + e.setInFileEnding(".in"); + e.setOutFileEnding(".out"); + e.setOrder("NORMAL"); + + final TempFileNames names = e.createTempFilenames(); + assertNotNull(names.getTempDir(), "getDir() returned null"); + assertNotNull(names.getBase(), "getBase() returned null"); + assertNotNull(names.getBasePath(), "getBasePath() returned null"); + assertNotNull(names.getIn(), "getIn() returned null"); + assertNotNull(names.getOut(), "getOut() returned null"); + assertNotNull(names.getInputFilename(), "getInPath() returned null"); + assertNotNull(names.getOutputFilename(), "getOutPath() returned null"); + assertTrue(names.getOut().endsWith(".out"), "output names must use out file ending"); + assertTrue(names.getIn().endsWith(".in"), "input names must use in file ending"); + } + + @Test + @SuppressWarnings("deprecation") // confirming functionality of legacy API void testExecutrixUniqueBase() { e.setInFileEnding(".in"); e.setOutFileEnding(".out"); @@ -95,6 +119,30 @@ void testExecutrixUniqueBase() { assertEquals(COUNT, basePathSet.size(), "Some BASE_PATH entries mismatch"); } + + /** + * Identical to {@link #testExecutrixUniqueBase}, but with the new TempFileNames API for constructing temp filenames + */ + @Test + void testExecutrixUniqueBaseWithTempFileNamesApi() { + e.setInFileEnding(".in"); + e.setOutFileEnding(".out"); + e.setOrder("NORMAL"); + + final int COUNT = 1000; + final Set basePathSet = Collections.synchronizedSet(new HashSet<>(COUNT)); + + // Generate COUNT sets of names + IntStream.range(0, COUNT).parallel().forEach(number -> { + final TempFileNames names = e.createTempFilenames(); + assertNotNull(names.getTempDir(), "getDir() returned null"); + assertNotNull(names.getBase(), "getBase() returned null"); + assertNotNull(names.getBasePath(), "getBasePath() returned null"); + basePathSet.add(names.getBasePath()); + }); + assertEquals(COUNT, basePathSet.size(), "Some BASE_PATH entries mismatch"); + } + @Test void testReadWrite() throws Exception { final String TMPDIR = e.getTmpDir(); @@ -260,6 +308,7 @@ void testWriteWithCleanup() throws Exception { } @Test + @SuppressWarnings("deprecation") // confirming functionality of legacy API void testExecute() throws IOException { final String[] names = e.makeTempFilenames(); @@ -342,6 +391,93 @@ void testExecute() throws IOException { assertFalse(tdir.exists(), "Temp area should be gone"); } + /** + * Identical to {@link #testExecute}, but with the new TempFileNames API for constructing temp filenames + */ + @Test + void testExecuteWithTempFileNamesApi() throws IOException { + assertNotNull(e, "Executrix instance should not be null"); + + final TempFileNames names = e.createTempFilenames(); + logger.debug("Names for testExecute is {}", names); + + final File tdir = new File(names.getTempDir()); + Files.createDirectories(tdir.toPath()); + assertTrue(tdir.exists() && tdir.isDirectory(), "Temp dir exists"); + + assertTrue(Executrix.writeDataToFile("aaa".getBytes(), names.getInputFilename()), "File written"); + final byte[] data = Executrix.readDataFromFile(names.getInputFilename()); + assertNotNull(data, "Data must be read from " + names.getInputFilename()); + + final String cmd = "cp "; + String[] c = e.getCommand(names, cmd); + assertNotNull(c, "Command returned"); + assertEquals("/bin/sh", c[0], "Command runner"); + + e.setCommand(cmd); + c = e.getCommand(names); + assertNotNull(c, "Command returned"); + assertEquals("/bin/sh", c[0], "Command runner"); + + logger.debug("Command to exec is {}", Arrays.asList(c)); + + int pstat; + final StringBuilder out = new StringBuilder(); + final StringBuilder err = new StringBuilder(); + + pstat = e.execute(c, out, err); + logger.debug("Stdout: {}", out); + logger.debug("Stderr: {}", err); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + pstat = e.execute(c, out); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + pstat = e.execute(c, out, new StringBuilder("UTF-8")); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + pstat = e.execute(c, out, err, "UTF-8"); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + final StringBuilder sout = new StringBuilder(); + final StringBuilder serr = new StringBuilder(); + + pstat = e.execute(c, sout); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + pstat = e.execute(c, sout, serr); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + pstat = e.execute(c, sout, serr, "UTF-8"); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + final Map env = new HashMap<>(); + env.put("FOO", "BAR"); + + pstat = e.execute(c, sout, serr, "UTF-8", env); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + pstat = e.execute(c); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + e.setProcessMaxMillis(0); // wait forever + pstat = e.execute(c, sout, serr, "UTF-8", env); + assertTrue(pstat >= 0, "Process return value"); + readAndNuke(names.getOutputFilename()); + + assertTrue(Executrix.cleanupDirectory(tdir), "Temp area clean up removes all"); + assertFalse(tdir.exists(), "Temp area should be gone"); + } + @Test void testExecuteStream() {