[jira] [Created] (ZOOKEEPER-4246) Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream

["Martin Kellogg (Jira)" <ji...@apache.org>]

Martin Kellogg created ZOOKEEPER-4246:
-----------------------------------------

             Summary: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
                 Key: ZOOKEEPER-4246
                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-4246
             Project: ZooKeeper
          Issue Type: Bug
          Components: server
            Reporter: Martin Kellogg


 There are three (related) possible resource leaks in the `getInputStream` and `getOutputStream` methods in `SnapStream.java`. I noticed the first because of the use of the error-prone `GZIPOutputStream`, and the other two after looking at the surrounding code.

Here is the offending code (copied from [here|https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/SnapStream.java#L102]):
{noformat}
    /**
     * Return the CheckedInputStream based on the extension of the fileName.
     *
     * @param file the file the InputStream read from
     * @return the specific InputStream
     * @throws IOException
     */
    public static CheckedInputStream getInputStream(File file) throws IOException {
        FileInputStream fis = new FileInputStream(file);
        InputStream is;
        switch (getStreamMode(file.getName())) {
        case GZIP:
            is = new GZIPInputStream(fis);
            break;
        case SNAPPY:
            is = new SnappyInputStream(fis);
            break;
        case CHECKED:
        default:
            is = new BufferedInputStream(fis);
        }
        return new CheckedInputStream(is, new Adler32());
    }

    /**
     * Return the OutputStream based on predefined stream mode.
     *
     * @param file the file the OutputStream writes to
     * @param fsync sync the file immediately after write
     * @return the specific OutputStream
     * @throws IOException
     */
    public static CheckedOutputStream getOutputStream(File file, boolean fsync) throws IOException {
        OutputStream fos = fsync ? new AtomicFileOutputStream(file) : new FileOutputStream(file);
        OutputStream os;
        switch (streamMode) {
        case GZIP:
            os = new GZIPOutputStream(fos);
            break;
        case SNAPPY:
            os = new SnappyOutputStream(fos);
            break;
        case CHECKED:
        default:
            os = new BufferedOutputStream(fos);
        }
        return new CheckedOutputStream(os, new Adler32());
    }
{noformat}

All three possible resource leaks are caused by the constructors of the intermediate streams (i.e. `is` and `os`), some of which might throw `IOException`s:
 * in `getOutputStream`, the call to `new GZIPOutputStream` can throw an exception, because `GZIPOutputStream` writes out the header in the constructor. If it does throw, then `fos` is never closed. That it does so makes it hard to use correctly; someone raised this as an issue with the JDK folks [here|https://bugs.openjdk.java.net/browse/JDK-8180899], but they closed it as "won't fix" because the constructor is documented to throw (hence the need to catch the exception here).
 * in `getInputStream`, the call to `new GZIPInputStream` can throw an `IOException` for a similar reason, causing the file handle held by `fis` to leak.
 * similarly, the call to `new SnappyInputStream` can throw an `IOException`, because it tries to read the file header during construction, which also causes `fis` to leak. `SnappyOutputStream` cannot throw; I checked [here|https://github.com/xerial/snappy-java/blob/master/src/main/java/org/xerial/snappy/SnappyOutputStream.java].

I'll submit a PR with a (simple) fix shortly after this bug report goes up and gets assigned an issue number, and add a link to this issue.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)