Skip to content

Commit

Permalink
[FIXED JENKINS-37332] - Prevent File descriptor leaks when reading ma…
Browse files Browse the repository at this point in the history 
…nifests from JARs (#2516)

* [JENKINS-37332] - Improve diagnostics of non-closed streams during reading of the manifests in PluginManager

* [JENKINS-37332] - Leakless processing of JarUrlConnection during Manifest parsing

* [JENKINS-37332] - Also implement leak-safe method for retrieving file modification date

* [JENKINS-37332] - Add spotcheck methods for manifest file access + Javadoc

* [JENKINS-37332] - Also test multi-line and empty attributes in the test

(cherry picked from commit 96c9786)
  • Loading branch information
@oleg-nenashev @olivergondza
oleg-nenashev authored and olivergondza committed Sep 6, 2016
1 parent 832dc98 commit 37edc1a
Show file tree
Hide file tree
Showing 3 changed files with 205 additions and 6 deletions.
89 changes: 84 additions & 5 deletions core/src/main/java/hudson/PluginManager.java
View file
Expand Up @@ -139,6 +139,10 @@
import static hudson.init.InitMilestone.*;
import hudson.model.DownloadService;
import hudson.util.FormValidation;
import java.io.ByteArrayInputStream;
import java.net.JarURLConnection;
import java.net.URLConnection;
import java.util.jar.JarEntry;

import static java.util.logging.Level.FINE;
import static java.util.logging.Level.INFO;
Expand Down Expand Up @@ -941,7 +945,7 @@ public synchronized void resolveDependantPlugins() {
protected void copyBundledPlugin(URL src, String fileName) throws IOException {
fileName = fileName.replace(".hpi",".jpi"); // normalize fileNames to have the correct suffix
String legacyName = fileName.replace(".jpi",".hpi");
long lastModified = src.openConnection().getLastModified();
long lastModified = getModificationDate(src);
File file = new File(rootDir, fileName);

// normalization first, if the old file exists.
Expand All @@ -952,7 +956,7 @@ protected void copyBundledPlugin(URL src, String fileName) throws IOException {
// - bundled version and current version differs (by timestamp).
if (!file.exists() || file.lastModified() != lastModified) {
FileUtils.copyURLToFile(src, file);
file.setLastModified(src.openConnection().getLastModified());
file.setLastModified(getModificationDate(src));
// lastModified is set for two reasons:
// - to avoid unpacking as much as possible, but still do it on both upgrade and downgrade
// - to make sure the value is not changed after each restart, so we can avoid
Expand All @@ -963,19 +967,19 @@ protected void copyBundledPlugin(URL src, String fileName) throws IOException {
// See https://groups.google.com/d/msg/jenkinsci-dev/kRobm-cxFw8/6V66uhibAwAJ
}

private static @CheckForNull Manifest parsePluginManifest(URL bundledJpi) {
/*package*/ static @CheckForNull Manifest parsePluginManifest(URL bundledJpi) {
try {
URLClassLoader cl = new URLClassLoader(new URL[]{bundledJpi});
InputStream in=null;
try {
URL res = cl.findResource(PluginWrapper.MANIFEST_FILENAME);
if (res!=null) {
in = res.openStream();
in = getBundledJpiManifestStream(res);
Manifest manifest = new Manifest(in);
return manifest;
}
} finally {
IOUtils.closeQuietly(in);
Util.closeAndLogFailures(in, LOGGER, PluginWrapper.MANIFEST_FILENAME, bundledJpi.toString());
if (cl instanceof Closeable)
((Closeable)cl).close();
}
Expand All @@ -984,6 +988,81 @@ protected void copyBundledPlugin(URL src, String fileName) throws IOException {
}
return null;
}

/**
* Retrieves input stream for the Manifest url.
* The method intelligently handles the case of {@link JarURLConnection} pointing to files within JAR.
* @param url Url of the manifest file
* @return Input stream, which allows to retrieve manifest. This stream must be closed outside
* @throws IOException Operation error
*/
@Nonnull
/*package*/ static InputStream getBundledJpiManifestStream(@Nonnull URL url) throws IOException {
URLConnection uc = url.openConnection();
InputStream in = null;
// Magic, which allows to avoid using stream generated for JarURLConnection.
// It prevents getting into JENKINS-37332 due to the file desciptor leak
if (uc instanceof JarURLConnection) {
final JarURLConnection jarURLConnection = (JarURLConnection) uc;
final String entryName = jarURLConnection.getEntryName();

try(final JarFile jarFile = jarURLConnection.getJarFile()) {
final JarEntry entry = (entryName != null && jarFile != null) ? jarFile.getJarEntry(entryName) : null;
if (entry != null && jarFile != null) {
try(InputStream i = jarFile.getInputStream(entry)) {
byte[] manifestBytes = IOUtils.toByteArray(i);
in = new ByteArrayInputStream(manifestBytes);
}
} else {
LOGGER.log(Level.WARNING, "Failed to locate the JAR file for {0}"
+ "The default URLConnection stream access will be used, file descriptor may be leaked.",
url);
}
}
}

// If input stream is undefined, use the default implementation
if (in == null) {
in = url.openStream();
}

return in;
}

/**
* Retrieves modification date of the specified file.
* The method intelligently handles the case of {@link JarURLConnection} pointing to files within JAR.
* @param url Url of the file
* @return Modification date
* @throws IOException Operation error
*/
@Nonnull
/*package*/ static long getModificationDate(@Nonnull URL url) throws IOException {
URLConnection uc = url.openConnection();

// It prevents file desciptor leak if the URL references a file within JAR
// See JENKINS-37332 for more info
// The code idea is taken from https://github.com/jknack/handlebars.java/pull/394
if (uc instanceof JarURLConnection) {
final JarURLConnection connection = (JarURLConnection) uc;
final URL jarURL = connection.getJarFileURL();
if (jarURL.getProtocol().equals("file")) {
uc = null;
String file = jarURL.getFile();
return new File(file).lastModified();
} else {
// We access the data without file protocol
if (connection.getEntryName() != null) {
LOGGER.log(WARNING, "Accessing modification date of {0} file, which is an entry in JAR file. "
+ "The access protocol is not file:, falling back to the default logic (risk of file descriptor leak).",
url);
}
}
}

// Fallbak to the default implementation
return uc.getLastModified();
}

/**
* Rename a legacy file to a new name, with care to Windows where {@link File#renameTo(File)}
Expand Down
22 changes: 22 additions & 0 deletions core/src/main/java/hudson/Util.java
View file
Expand Up @@ -1623,6 +1623,28 @@ public static Properties loadProperties(@Nonnull String properties) throws IOExc
p.load(new StringReader(properties));
return p;
}

/**
* Closes the item and logs error to the log in the case of error.
* Logging will be performed on the {@code WARNING} level.
* @param toClose Item to close. Nothing will happen if it is {@code null}
* @param logger Logger, which receives the error
* @param closeableName Name of the closeable item
* @param closeableOwner String representation of the closeable holder
* @since TODO once merged to the master and un-restricted
*/
@Restricted(NoExternalUse.class)
public static void closeAndLogFailures(@CheckForNull Closeable toClose, @Nonnull Logger logger,
@Nonnull String closeableName, @Nonnull String closeableOwner) {
if (toClose == null) {
return;
}
try {
toClose.close();
} catch(IOException ex) {
logger.log(Level.WARNING, String.format("Failed to close %s of %s", closeableName, closeableOwner), ex);
}
}

public static final FastDateFormat XS_DATETIME_FORMATTER = FastDateFormat.getInstance("yyyy-MM-dd'T'HH:mm:ss'Z'",new SimpleTimeZone(0,"GMT"));

Expand Down
100 changes: 99 additions & 1 deletion core/src/test/java/hudson/PluginManagerTest.java
View file
Expand Up @@ -24,17 +24,33 @@

package hudson;

import java.io.File;
import java.io.FileOutputStream;
import org.apache.tools.ant.filters.StringInputStream;
import org.junit.Test;
import org.xml.sax.SAXException;
import java.io.IOException;
import java.io.InputStream;
import java.net.JarURLConnection;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.jar.Attributes;
import java.util.jar.Manifest;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;
import java.util.zip.ZipOutputStream;
import org.apache.commons.io.FileUtils;
import static org.hamcrest.CoreMatchers.*;
import static org.hamcrest.core.IsInstanceOf.instanceOf;
import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.*;
import org.junit.Rule;
import org.junit.rules.TemporaryFolder;
import org.jvnet.hudson.test.Issue;


/**
* Tests of {@link PluginManager}.
*/
public class PluginManagerTest {

@Rule public TemporaryFolder tmp = new TemporaryFolder();
Expand Down Expand Up @@ -68,4 +84,86 @@ public void parseInvalidRequestedPlugins() throws Exception {
assertThat(ex.getCause().getMessage(), containsString("Refusing to resolve entity with publicId(null) and systemId (file:///)"));
}
}

@Test
public void shouldProperlyParseManifestFromJar() throws IOException {
File jar = createHpiWithManifest();
final Manifest manifest = PluginManager.parsePluginManifest(jar.toURI().toURL());

assertThat("manifest should have been read from the sample", manifest, notNullValue());
assertAttribute(manifest, "Created-By", "Apache Maven");
assertAttribute(manifest, "Short-Name", "matrix-auth");

// Multi-line entries
assertAttribute(manifest, "Specification-Title", "Offers matrix-based security authorization strategies (global and per-project).");
assertAttribute(manifest, "Url", "http://wiki.jenkins-ci.org/display/JENKINS/Matrix+Authorization+Strategy+Plugin");

// Empty field
assertAttribute(manifest, "Plugin-Developers", null);
}

@Test
public void shouldProperlyRetrieveModificationDate() throws IOException {
File jar = createHpiWithManifest();
URL url = toManifestUrl(jar);
assertThat("Manifest last modified date should be equal to the file date",
PluginManager.getModificationDate(url),
equalTo(jar.lastModified()));
}

private static void assertAttribute(Manifest manifest, String attributeName, String value) throws AssertionError {
Attributes attributes = manifest.getMainAttributes();
assertThat("Main attributes must not be empty", attributes, notNullValue());
assertThat("Attribute '" + attributeName + "' does not match the sample",
attributes.getValue(attributeName),
equalTo(value));

}

private static final String SAMPLE_MANIFEST_FILE = "Manifest-Version: 1.0\n" +
"Archiver-Version: Plexus Archiver\n" +
"Created-By: Apache Maven\n" +
"Built-By: jglick\n" +
"Build-Jdk: 1.8.0_92\n" +
"Extension-Name: matrix-auth\n" +
"Specification-Title: \n" +
" Offers matrix-based security \n" +
" authorization strate\n" +
" gies (global and per-project).\n" +
"Implementation-Title: matrix-auth\n" +
"Implementation-Version: 1.4\n" +
"Group-Id: org.jenkins-ci.plugins\n" +
"Short-Name: matrix-auth\n" +
"Long-Name: Matrix Authorization Strategy Plugin\n" +
"Url: http://wiki.jenkins-ci.org/display/JENKINS/Matrix+Authorization+S\n" +
" trategy+Plugin\n" +
"Plugin-Version: 1.4\n" +
"Hudson-Version: 1.609.1\n" +
"Jenkins-Version: 1.609.1\n" +
"Plugin-Dependencies: icon-shim:2.0.3,cloudbees-folder:5.2.2;resolution\n" +
" :=optional\n" +
"Plugin-Developers: ";

private File createHpiWithManifest() throws IOException {
File newFolder = tmp.newFolder("myJar");
String manifestPath = "META-INF/MANIFEST.MF";
new File("META-INF").mkdir();
FileUtils.write(new File(newFolder, manifestPath), SAMPLE_MANIFEST_FILE);

final File f = new File(tmp.getRoot(), "my.hpi");
try(ZipOutputStream out = new ZipOutputStream(new FileOutputStream(f))) {
ZipEntry e = new ZipEntry(manifestPath);
out.putNextEntry(e);
byte[] data = SAMPLE_MANIFEST_FILE.getBytes();
out.write(data, 0, data.length);
out.closeEntry();
}
return f;
}


private URL toManifestUrl(File jarFile) throws MalformedURLException {
final String manifestPath = "META-INF/MANIFEST.MF";
return new URL("jar:" + jarFile.toURI().toURL() + "!/" + manifestPath);
}
}

0 comments on commit 37edc1a

Please sign in to comment.