CodeSOD: Unzipped

When you promise to deliver a certain level of service, you need to live up to that promise. When your system is critical to your customers, there are penalties for failing to live up to that standard. For the mission-critical application Rich D supports, that penalty is $10,000 a minute for any outages.

Now, one might think that such a mission critical system has a focus on testing, code quality, and stability. You probably don't think that, but someone might expect that.

This Java application contains a component which needs to take a zip file, extract an executable script from it, and then execute that script. The code that does this is... a lot, so we're going to take it in chunks. Let's start by looking at the core loop.

private void extractAndLaunch(File file, String fileToLaunch) {try {ZipInputStream zipIn = new ZipInputStream(new FileInputStream(file));ZipEntry entry = zipIn.getNextEntry();byte[] buffer = new byte[1024];while (entry != null) {if (!entry.isDirectory() && entry.getName().compareToIgnoreCase(fileToLaunch) == 0) {// SNIP, for now}}zipIn.closeEntry();entry = zipIn.getNextEntry();zipIn.close();} catch (Exception e) {LOG.error("Failed to load staging file {}", file, e);}}

So, we create a ZipInputStream to cycle through the zip file, and then get the first entry from it. While entry != null, we do a test: if the entry isn't a directory, and the name of the entry is the file we want to launch, we'll do all the magic of launching the executable. Otherwise, we go back to the top of the loop, and repeat the same check, on the same entry, forever. If the first file in this zip file isn't the file we want to execute, this falls into an infinite loop, because the code for cycling to the next entry is outside of the loop.

This may be a good time to point out that this code has been in production for four years.

Okay, so how do we extract the file?

// Extract the fileFile newFile = new File(top + File.separator + entry.getName());LOG.debug("Extracting and launching {}", newFile.getPath());// Create any necessary directoriestry { new File(newFile.getParent()).mkdirs();} catch (NullPointerException ignore) { // CS:NO IllegalCatch // No parent directory so dont worry about it}int len;FileOutputStream fos = new FileOutputStream(newFile);while ((len = zipIn.read(buffer)) > 0) { fos.write(buffer, 0, len);}fos.close();zipIn.closeEntry();

So, first, we build a file path with string concatenation, which is ugly and avoidable. At least they use File.separator, instead of hard-coding a "/" or "\". But there's a problem with this: top comes from a configuration file and is loaded by System.getProperty(), which may not be set, or may be an empty string. This means we might jam things into a directory called null, or worse, try and extract to the root of the filesystem.

Which also means that newFile.getParent() may be null. Instead of checking that, we'll just catch any exceptions it throws.

We also call zipIn.closeEntry() here, and we close the same entry again after the loop. I assume the double close doesn't hurt anything, but it's definitely annoying.

Okay, so how do we execute the file?

OsHelper.execute(newFile.getName(), newFile.getParentFile());// Execute the file// TODO Fix this to work under linuxList<String> cmdAndArgs = Arrays.asList("cmd", "/c", fileToLaunch);ProcessBuilder pb = new ProcessBuilder(cmdAndArgs);pb.directory(new File(System.getProperty("top")));Process p = pb.start();InputStream error = p.getErrorStream();byte[] errBuf = new byte[1024];if (error.available() > 0) { error.read(errBuf); LOG.error("Script {} had error {}", fileToLaunch, errBuf);}int exitValue = 0;while (true) { try { exitValue = p.exitValue(); break; } catch (IllegalThreadStateException ignore) {} // Just waiting for the batch to end}LOG.info("Script " + fileToLaunch + " exited with status of " + exitValue);newFile.delete();break;

OsHelper.execute does not, as the name implies, execute the program we want to run. It actually sets the executable bit on Linux systems. It doesn't use any Java APIs to do this, but just calls chmod to mark the file as executable.

Of course, that doesn't matter, because as the comment explains: this doesn't actually work on Linux. They actually shell out to cmd to run it, the Windows shell.

Then we launch the script, running it in the working directory specified by top, but instead of re-using the variable, we fetch it from the configuration again. We read from standard error on the process, but we don't wait, so most of the time this won't give us anything. We'd have to be very lucky to get any output from this running process.

Then, we wait for the script to complete. Now, it's worth noting that there's a Java built-in for this, Process#waitFor() which will idle until the process completes. Idle, instead of busy wait, which is what this code does. It's also worth noting that Process#exitValue() throws an exception if the process is still running, so in practice this code spams IllegalThreadStateExceptions as fast as it can.

Finally, none of these exception handlers have finally blocks, so if we do get an error that bubbles up, we'll never call newFile.delete(), leaving our intermediately processed work sitting there.

The code, in its entirety:

private void extractAndLaunch(File file, String fileToLaunch) {try {ZipInputStream zipIn = new ZipInputStream(new FileInputStream(file));ZipEntry entry = zipIn.getNextEntry();byte[] buffer = new byte[1024];while (entry != null) {if (!entry.isDirectory() && entry.getName().compareToIgnoreCase(fileToLaunch) == 0) {// Extract the fileFile newFile = new File(top + File.separator + entry.getName());LOG.debug("Extracting and launching {}", newFile.getPath());// Create any necessary directoriestry {new File(newFile.getParent()).mkdirs();} catch (NullPointerException ignore) { // CS:NO IllegalCatch// No parent directory so dont worry about it}int len;FileOutputStream fos = new FileOutputStream(newFile);while ((len = zipIn.read(buffer)) > 0) {fos.write(buffer, 0, len);}fos.close();zipIn.closeEntry();OsHelper.execute(newFile.getName(), newFile.getParentFile());// Execute the file// TODO Fix this to work under linuxList<String> cmdAndArgs = Arrays.asList("cmd", "/c", fileToLaunch);ProcessBuilder pb = new ProcessBuilder(cmdAndArgs);pb.directory(new File(System.getProperty("censored")));Process p = pb.start();InputStream error = p.getErrorStream();byte[] errBuf = new byte[1024];if (error.available() > 0) {error.read(errBuf);LOG.error("Script {} had error {}", fileToLaunch, errBuf);}int exitValue = 0;while (true) {try {exitValue = p.exitValue();break;} catch (IllegalThreadStateException ignore) {} // Just waiting for the batch to end}LOG.info("Script " + fileToLaunch + " exited with status of " + exitValue);newFile.delete();break;}}zipIn.closeEntry();entry = zipIn.getNextEntry();zipIn.close();} catch (Exception e) {LOG.error("Failed to load staging file {}", file, e);}}

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.