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

[CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exceptional paths #104

Merged
merged 2 commits into from
Aug 26, 2016
Merged

[CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exceptional paths #104

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
947f01c
[CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exce…
oleg-nenashev Aug 20, 2016
f182023
[CID-152202] - Fix typo noticed by @olivergondza
oleg-nenashev Aug 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/main/java/org/jenkinsci/remoting/engine/EngineUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@
*/
package org.jenkinsci.remoting.engine;

import java.io.BufferedInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.io.*;
import java.util.Properties;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
* Engine utility methods.
Expand Down Expand Up @@ -92,4 +93,24 @@ protected static Properties readResponseHeaders(BufferedInputStream inputStream)
response.put(line.substring(0,idx).trim(), line.substring(idx+1).trim());
}
}

/**
* 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
*/
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);
}
}
}
24 changes: 19 additions & 5 deletions src/main/java/org/jenkinsci/remoting/engine/JnlpServer3Handshake.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,25 @@ public Channel connect() throws IOException, InterruptedException {
Jnlp3Util.keyFromString(aesKeyString),
Jnlp3Util.keyFromString(specKeyString));

Channel channel = createChannelBuilder(nodeName).build(
new CipherInputStream(SocketChannelStream.in(socket),
channelCiphers.getDecryptCipher()),
new CipherOutputStream(SocketChannelStream.out(socket),
channelCiphers.getEncryptCipher()));

CipherInputStream in = null;
CipherOutputStream out = null;
final Channel channel;
try {
in = new CipherInputStream(SocketChannelStream.in(socket), channelCiphers.getDecryptCipher());
out = new CipherOutputStream(SocketChannelStream.out(socket), channelCiphers.getEncryptCipher());
channel = createChannelBuilder(nodeName).build(in, out);
} catch (Exception ex) {
// Something went wrong. We want to gracefully close Cipher stream in the case they were open
EngineUtil.closeAndLogFailures(in, LOGGER, "CipherInputStream", socket.toString());
EngineUtil.closeAndLogFailures(out, LOGGER, "CipherOutputStream", socket.toString());
Copy link
Member

Choose a reason for hiding this comment

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

🐜 better (under Java 7) to use Throwable.addSuppressed. Or simpler still:

Channel channel = null;
CipherInputStream in = new CipherInputStream(SocketChannelStream.in(socket), channelCiphers.getDecryptCipher());
try {
    CipherOutputStream out = new CipherOutputStream(SocketChannelStream.out(socket), channelCiphers.getEncryptCipher());
    try {
        return channel = createChannelBuilder(nodeName).build(in, out);
    } finally {
        if (channel == null) {
            out.close();
        }
    }
} finally {
    if (channel == null) {
        in.close();
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Even better would be a utility (should I file a suggestion for Guava?):

public static class ClosePartials implements AutoCloseable {
    private final List<AutoCloseable> things = new ArrayList<>();
    public ClosePartials() {}
    public <T extends AutoCloseable> T with(T thing) {
        things.add(thing);
        return thing;
    }
    /** @param <T> recommended to also be {@link AutoCloseable} */
    public <T> T done(T thing) {
        things.clear();
        return thing;
    }
    @Override
    public void close() throws Exception {
        Exception x = null;
        for (AutoCloseable thing : things) {
            try {
                thing.close();
            } catch (Throwable t) {
                if (x == null) {
                    if (t instanceof Exception) {
                        x = (Exception) t;
                    } else {
                        x = new Exception(t);
                    }
                } else {
                    x.addSuppressed(t);
                }
            }
        }
        if (x != null) {
            throw x;
        }
    }
}

Example usage:

public static Channel example(Socket s, Cipher c, ChannelBuilder cb) throws Exception {
    try (ClosePartials cp = new ClosePartials();
         InputStream sin = cp.with(SocketChannelStream.in(s));
         CipherInputStream cin = cp.with(new CipherInputStream(sin, c));
         OutputStream sout = cp.with(SocketChannelStream.out(s));
         CipherOutputStream cout = cp.with(new CipherOutputStream(sout, c))) {
        return cp.done(cb.build(cin, cout));
    }
}

In general, methods which deliberately create closeable [sic] objects but then conceal them as references inside some higher-level closeable object are risky, because even if the ultimate caller is careful to close the final object, its close method might not be foolproof. Channel.close does not even pretend to guarantee that all streams it was originally passed will be closed by the time the method completes.

if (ex instanceof IOException) {
throw (IOException) ex;
Copy link
Member

Choose a reason for hiding this comment

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

The cast seems unsafe.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is unsafe... I've messed up something

} else {
throw new IOException("Unexpected runtime exception during creation of the channel", ex);
}
}


channel.setProperty(COOKIE_NAME, newCookie);

Expand Down