zookeeper-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ling Mao (Jira)" <j...@apache.org>
Subject [jira] [Resolved] (ZOOKEEPER-4246) Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Date Sat, 15 May 2021 15:53:00 GMT

     [ 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)

Mime
View raw message