[
https://issues.apache.org/jira/browse/ZOOKEEPER-4246?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ling Mao resolved ZOOKEEPER-4246.
---------------------------------
Resolution: Fixed
> 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
> Priority: Major
> Labels: pull-request-available
> Time Spent: 2h 20m
> Remaining Estimate: 0h
>
> 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)
