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

[oleg-nenashev]

• https://scan8.coverity.com/reports.htm#v14462/p10499/fileInstanceId=12836377&defectInstanceId=4427377&mergedDefectId=152202
• https://scan8.coverity.com/reports.htm#v14462/p10499/fileInstanceId=12836377&defectInstanceId=4427377&mergedDefectId=152200

[oleg-nenashev]

@reviewbybees @jenkinsci/code-reviewers

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[olivergondza]

The cast seems unsafe.

[oleg-nenashev]

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

[oleg-nenashev]

@olivergondza fixed

[jglick]

🐜 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();
    }
}

[jglick]

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.

[jglick]

Anyway, 🐝 as it stands.

[oleg-nenashev]

All these PRs go against stable-2.x, which will stay on Java6

[oleg-nenashev]

@reviewbybees done