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.

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

Fix file leak when using metadata functions #1359

Merged
merged 2 commits into from
Oct 1, 2023

Conversation

pablobaxter
Copy link
Contributor

@pablobaxter pablobaxter commented Sep 30, 2023

Discovered this issue via the wire project.

The issue here is that the ZipFileSystem creates a Source object, which is then returned out of the use lambda, which the lambda is supposed to close the FileHandle that is created, however, because the Source object had not been closed, the FileHandle remains open.

val source = fileSystem.openReadOnly(zipPath).use { fileHandle ->
  fileHandle.source(entry.offset).buffer()
}
return source.readLocalHeader(basicMetadata)

Going into a bit more detail, the openStreamCount is not 0 because the Source object wasn't closed, so we never close the underlying RandomAccessFile that was opened.

@Throws(IOException::class)
final override fun close() {
  lock.withLock {
    if (closed) return
    closed = true
    if (openStreamCount != 0) return
  }
  protectedClose()
}

This fix ensures the Source object is closed once the metadata has been extracted. This issue also occurs when calling exists() in the ZipFileSystem.

Closes #1356

@JakeWharton
Copy link
Member

#1356 has a test. Can we copy that in so as to not regress?

@@ -99,10 +99,11 @@ internal class ZipFileSystem internal constructor(
return basicMetadata
}

val source = fileSystem.openReadOnly(zipPath).use { fileHandle ->
fileHandle.source(entry.offset).buffer()
return fileSystem.openReadOnly(zipPath).use { fileHandle ->
Copy link
Member

Choose a reason for hiding this comment

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

Much better

Copy link
Member

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

Thanks for finding this and also for fixing this. Your test is brilliant too. Yay!

@swankjesse swankjesse enabled auto-merge (squash) October 1, 2023 21:00
@swankjesse swankjesse merged commit ce4df5e into square:master Oct 1, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZipFileSystem.metadata leaves file open
3 participants