From 69404db82a26f84909ef9d4a9cba9bc4acc505ef Mon Sep 17 00:00:00 2001 From: Anton Danshin Date: Sat, 17 Sep 2022 16:27:54 -0400 Subject: [PATCH 1/5] Fix file descriptor leak in SentryFileInputStream and SentryFileOutputStream --- .../instrumentation/file/SentryFileInputStream.java | 10 +++++++++- .../instrumentation/file/SentryFileOutputStream.java | 11 ++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java index b7a88688aa2..d3937049dac 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java +++ b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java @@ -54,7 +54,7 @@ private SentryFileInputStream( private SentryFileInputStream(final @NotNull FileInputStreamInitData data) throws FileNotFoundException { - super(data.file); + super(getFileDescriptor(data.delegate)); spanManager = new FileIOSpanManager(data.span, data.file, data.isSendDefaultPii); delegate = data.delegate; } @@ -114,6 +114,14 @@ public void close() throws IOException { spanManager.finish(delegate); } + private static FileDescriptor getFileDescriptor(FileInputStream stream) throws FileNotFoundException { + try { + return stream.getFD(); + } catch (IOException error) { + throw new FileNotFoundException("No file descriptor"); + } + } + public static final class Factory { public static FileInputStream create( final @NotNull FileInputStream delegate, final @Nullable String name) diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java index 18c7f4811ed..53f5109825f 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java +++ b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java @@ -5,6 +5,7 @@ import io.sentry.ISpan; import java.io.File; import java.io.FileDescriptor; +import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; @@ -59,7 +60,7 @@ private SentryFileOutputStream( private SentryFileOutputStream(final @NotNull FileOutputStreamInitData data) throws FileNotFoundException { - super(data.file, data.append); + super(getFileDescriptor(data.delegate)); spanManager = new FileIOSpanManager(data.span, data.file, data.isSendDefaultPii); delegate = data.delegate; } @@ -120,6 +121,14 @@ public void close() throws IOException { spanManager.finish(delegate); } + private static FileDescriptor getFileDescriptor(FileOutputStream stream) throws FileNotFoundException { + try { + return stream.getFD(); + } catch (IOException error) { + throw new FileNotFoundException("No file descriptor"); + } + } + public static final class Factory { public static FileOutputStream create( final @NotNull FileOutputStream delegate, final @Nullable String name) From a132a8e3c2d35e9e99dd1c4f9b4403fd0d7dec28 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 19 Sep 2022 09:01:09 +0200 Subject: [PATCH 2/5] Add test; format --- .../instrumentation/file/SentryFileInputStream.java | 3 ++- .../instrumentation/file/SentryFileOutputStream.java | 4 ++-- .../instrumentation/file/SentryFileInputStreamTest.kt | 8 ++++++++ .../instrumentation/file/SentryFileOutputStreamTest.kt | 8 ++++++++ 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java index d3937049dac..c74774a905b 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java +++ b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java @@ -114,7 +114,8 @@ public void close() throws IOException { spanManager.finish(delegate); } - private static FileDescriptor getFileDescriptor(FileInputStream stream) throws FileNotFoundException { + private static FileDescriptor getFileDescriptor(FileInputStream stream) + throws FileNotFoundException { try { return stream.getFD(); } catch (IOException error) { diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java index 53f5109825f..0cf2b1fbb36 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java +++ b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java @@ -5,7 +5,6 @@ import io.sentry.ISpan; import java.io.File; import java.io.FileDescriptor; -import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; @@ -121,7 +120,8 @@ public void close() throws IOException { spanManager.finish(delegate); } - private static FileDescriptor getFileDescriptor(FileOutputStream stream) throws FileNotFoundException { + private static FileDescriptor getFileDescriptor(FileOutputStream stream) + throws FileNotFoundException { try { return stream.getFD(); } catch (IOException error) { diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt index d9bc69a3ee9..fb9ba9c75b6 100644 --- a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt +++ b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt @@ -16,6 +16,7 @@ import java.io.FileInputStream import java.io.IOException import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse class SentryFileInputStreamTest { @@ -96,6 +97,13 @@ class SentryFileInputStreamTest { assertEquals(fileIOSpan.status, SpanStatus.OK) } + @Test + fun `when stream is closed, releases file descriptor`() { + val fis = fixture.getSut(tmpFile) + fis.use { it.readAllBytes() } + assertFalse(fis.fd.valid()) + } + @Test fun `read one byte`() { fixture.getSut(tmpFile).use { it.read() } diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt index 8fe22f238c0..214930dc376 100644 --- a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt +++ b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt @@ -12,6 +12,7 @@ import org.junit.rules.TemporaryFolder import java.io.File import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse class SentryFileOutputStreamTest { class Fixture { @@ -68,6 +69,13 @@ class SentryFileOutputStreamTest { assertEquals(fileIOSpan.status, SpanStatus.OK) } + @Test + fun `when stream is closed file descriptor is also closed`() { + val fos = fixture.getSut(tmpFile) + fos.use { it.write("hello".toByteArray()) } + assertFalse(fos.fd.valid()) + } + @Test fun `write one byte`() { fixture.getSut(tmpFile).use { it.write(29) } From 3d9b2ac46de198f2ab4edb2f0cdd96108dbe663b Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 19 Sep 2022 09:04:03 +0200 Subject: [PATCH 3/5] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b6a105adda..44c8a2e7f17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Fixed AbstractMethodError when getting Lifecycle ([#2228](https://github.com/getsentry/sentry-java/pull/2228)) - Missing unit fields for Android measurements ([#2204](https://github.com/getsentry/sentry-java/pull/2204)) - Avoid sending empty profiles ([#2232](https://github.com/getsentry/sentry-java/pull/2232)) +- Fix file descriptor leak in FileIO instrumentation ([#2248](https://github.com/getsentry/sentry-java/pull/2248)) ## 6.4.1 From 5986588992ee325dad260d659a127b1ff865d4eb Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 19 Sep 2022 09:29:32 +0200 Subject: [PATCH 4/5] Update sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> --- .../io/sentry/instrumentation/file/SentryFileInputStream.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java index c74774a905b..1c65de2a694 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java +++ b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java @@ -114,7 +114,7 @@ public void close() throws IOException { spanManager.finish(delegate); } - private static FileDescriptor getFileDescriptor(FileInputStream stream) + private static FileDescriptor getFileDescriptor(final @NotNull FileInputStream stream) throws FileNotFoundException { try { return stream.getFD(); From d1a390ba7b4bf8737abf2223d87321b8509612d9 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 19 Sep 2022 09:29:37 +0200 Subject: [PATCH 5/5] Update sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> --- .../io/sentry/instrumentation/file/SentryFileOutputStream.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java index 0cf2b1fbb36..a48e10def0b 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java +++ b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java @@ -120,7 +120,7 @@ public void close() throws IOException { spanManager.finish(delegate); } - private static FileDescriptor getFileDescriptor(FileOutputStream stream) + private static FileDescriptor getFileDescriptor(final @NotNull FileOutputStream stream) throws FileNotFoundException { try { return stream.getFD();