[SPARK-29971][CORE] Fix buffer leaks in `TransportFrameDecoder/TransportCipher` #26609

normanmaurer · 2019-11-20T08:43:39Z

What changes were proposed in this pull request?

Correctly release ByteBuf in TransportCipher in all cases
Move closing / releasing logic to handlerRemoved(...) so we are guaranteed that is always called.
Correctly release frameBuf it is not null when the handler is removed (and so also when the channel becomes inactive)

Why are the changes needed?

We need to carefully manage the ownership / lifecycle of ByteBuf instances so we don't leak any of these. We did not correctly do this in all cases:

when end up in invalid cipher state.
when partial data was received and the channel is closed before the full frame is decoded

Fixes netty/netty#9784.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the newly added UTs.

SparkQA · 2019-11-20T08:57:19Z

Test build #114150 has finished for PR 26609 at commit a8420ee.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-20T11:00:15Z

Test build #114151 has finished for PR 26609 at commit c766150.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-20T11:37:50Z

Test build #114155 has finished for PR 26609 at commit bf1299b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-20T12:23:21Z

Test build #114159 has finished for PR 26609 at commit d35f381.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin

ByteArrayReadableChannel is kind of a weird class, I'd rather make the (only) caller do the right thing instead.

common/network-common/src/main/java/org/apache/spark/network/util/ByteArrayReadableChannel.java

common/network-common/src/main/java/org/apache/spark/network/util/TransportFrameDecoder.java

dongjoon-hyun · 2019-11-20T17:19:04Z

Thank you for making a PR, @normanmaurer !
I updated the PR description according to Apache Spark community style.

normanmaurer · 2019-11-20T19:33:05Z

@vanzin also related to your comment of ByteArrayReadableChannel.... imho you transfer ownership of the ByteBuf to ByteArrayReadableChannel so it should be its responsibility to ensure we release it.

vanzin · 2019-11-20T21:49:39Z

you transfer ownership of the ByteBuf to ByteArrayReadableChannel

As I mentioned that's a weird class. It's meant to be fed the buffer, and the caller is expected to consume all the data right away, as TransportCipher does. So technically TransportCipher never loses ownership of that buffer, and this class is just an optimization to avoid instantiating a new one every time data arrives.

If you're really so worried about all this you could make this class a private static class inside TransportCipher which is the only place where it's used.

normanmaurer · 2019-11-21T10:51:44Z

@vanzin like mentioned above I am not sure it is safe to assume it always consume all data as an InternalError may be thrown (you even guard against it). So I would suggest to be better safe then sorry. If you are super concerned about the extra logic in feedData(...) I would also be happy to let it throw if we already have a reference to a ByteBuf. Just let me know if you would prefer this and I will change the code. That said I am not sure why it is such a big deal to add the code to feedData as I did as I don't think it is overly complex. But I am happy to adapt if needed :)

normanmaurer · 2019-11-21T11:36:07Z

@dongjoon-hyun @vanzin I just added another commit to add a test case related to your handling of InternalError which would have leaked two buffers before these changes in here.

SparkQA · 2019-11-21T11:56:19Z

Test build #114238 has finished for PR 26609 at commit b06ec10.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

normanmaurer · 2019-11-21T13:26:31Z

@vanzin @dongjoon-hyun can you let me know how I can ignore the linter for the two "InternalError" instances that are thrown by the testcode ?

vanzin · 2019-11-21T17:57:57Z

I am not sure it is safe to assume it always consume all data as an InternalError

I'm not sure I follow you. If there's an internal error the whole channel is pretty much gone, nothing gets transferred anymore.

In the caller, you just do a try...finally and release the buffer in the finally. In the absence of errors, the contract is that all data in the buffer fed to the feedData method is read by TransportCipher.

Again, I understand you're thinking of this as generic code anyone can use, but it's not. This is internal Spark code that serves a specific purpose, behaves in a very particular way (it actually only exists to work around a bug in the commons-crypto library) and is not used anywhere else.

In other words, the buffer copying code you're adding is basically dead code. If it's ever triggered it's actually a bug in the existing code, so instead of adding that code you should be adding a check and throwing an exception, as I suggested.

normanmaurer · 2019-11-21T17:59:47Z

@vanzin I am not talking about the changes in feedData(...) here (which I think we can also just let it throw an exception as stated above). I am saying that if we not release the buffer in close() we may leak as there is no guarantee that we consume the whole buffer and so release it if an InternalError was thrown.

vanzin · 2019-11-21T18:03:20Z

Still not following you.

feedData is called once in channelRead, and the contract in the code is that all that buffer's data must be read in that same call to channelRead. So you can release the buffer in channelRead just fine as far as I can see. close doesn't need to do anything.

(You need to release the buffer in channelRead in any case, don't you? Since close is not called for each call to channelRead).

normanmaurer · 2019-11-21T18:10:11Z

@vanzin yes we could also just remove all the release magic in ByteArrayReadableChannel.read(...) and close(...) and handle all in channelRead(...) that would be fine with me as well. When I started the patch my assumption was that ByteArrayReadableChannel may be used by users and so we should not do this because of this. If this not a problem then sure lets do it. Please let me know.

common/network-common/src/test/java/org/apache/spark/network/crypto/TransportCipherTest.java

vanzin · 2019-11-21T18:14:31Z

remove all the release magic in ByteArrayReadableChannel.read(...) and close(...) and handle all in channelRead(...)

That sounds good to me.

normanmaurer · 2019-11-21T18:28:44Z

@vanzin done..

SparkQA · 2019-11-21T18:45:54Z

Test build #114251 has finished for PR 26609 at commit 34440e8.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

common/network-common/src/main/java/org/apache/spark/network/util/ByteArrayReadableChannel.java

common/network-common/src/test/java/org/apache/spark/network/crypto/TransportCipherTest.java

...network-common/src/test/java/org/apache/spark/network/util/ByteArrayReadableChannelTest.java

common/network-common/src/test/java/org/apache/spark/network/crypto/TransportCipherTest.java

normanmaurer · 2019-11-21T19:15:55Z

@vanzin can you have a look again ?

common/network-common/src/test/java/org/apache/spark/network/crypto/TransportCipherTest.java

dongjoon-hyun · 2019-11-21T19:19:29Z

Oh, you changed to Mock. Then, please ignore my outdated comments~

vanzin

Other than the style issues the linter picked up, looks ok.

common/network-common/src/test/java/org/apache/spark/network/crypto/TransportCipherTest.java

common/network-common/src/main/java/org/apache/spark/network/crypto/TransportCipher.java

normanmaurer · 2019-11-21T19:49:48Z

@vanzin I hope I fixed all now.

SparkQA · 2019-11-21T20:25:12Z

Test build #114252 has finished for PR 26609 at commit b4b1b76.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-21T22:12:47Z

Test build #114254 has finished for PR 26609 at commit c745649.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2019-11-22T00:01:03Z

Merging to master / 2.4. I'll fix the import order in the new test during merge.

vanzin · 2019-11-22T00:03:00Z

Actually, while merging I noticed something else... could you name the new test class TransportCipherSuite to match all the other test suites?

(While there, the import order convention is to have java/javax imports first.)

dongjoon-hyun · 2019-11-22T00:08:04Z

So, this is still under review status for those comments, right? @vanzin

vanzin · 2019-11-22T00:29:11Z

Yes, as you can see by the fact that it's still open...

…coder` and `TransportCipher` Motivation: We need to carefully manage the ownership / lifecycle of `ByteBuf` instances so we don't leak any of these. We did not correctly do this in all cases: - when end up in invalid cipher state. - when partial data was received and the channel is closed before the full frame is decoded Modifications: - Release message in finally block to ensure it is also released in case of cipher is not valid - Move closing / releasing logic to `handlerRemoved(...)` so we are guarenteed that is always called. - Add tests for `TransportCipher` - Correctly release `frameBuf` it is not null when the handler is removed (and so also when the channel becomes inactive) Result: Fixes netty/netty#9784.

normanmaurer · 2019-11-22T07:27:26Z

@vanzin @dongjoon-hyun addressed all + squashed and adjusted commit message to reflect the final patch.

SparkQA · 2019-11-22T08:05:02Z

Test build #114281 has finished for PR 26609 at commit fb79d0a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-22T08:05:02Z

Test build #114280 has finished for PR 26609 at commit d8d2bbe.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class TransportCipherSuite

vanzin · 2019-11-22T16:18:22Z

retest this please

dongjoon-hyun

+1, LGTM.

SparkQA · 2019-11-22T18:29:51Z

Test build #114306 has finished for PR 26609 at commit fb79d0a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2019-11-22T19:01:51Z

common/network-common/src/main/java/org/apache/spark/network/util/ByteArrayReadableChannel.java

 import java.nio.channels.ReadableByteChannel;

 import io.netty.buffer.ByteBuf;

 public class ByteArrayReadableChannel implements ReadableByteChannel {
  private ByteBuf data;
+  private boolean closed;


Not sure, does this need to be volatile? I'm not sure if there are thread safety concerns here.

streams / channels are not thread-safe.

srowen · 2019-11-22T19:03:19Z

common/network-common/src/main/java/org/apache/spark/network/util/ByteArrayReadableChannel.java

    int totalRead = 0;
    while (data.readableBytes() > 0 && dst.remaining() > 0) {
      int bytesToRead = Math.min(data.readableBytes(), dst.remaining());
      dst.put(data.readSlice(bytesToRead).nioBuffer());
      totalRead += bytesToRead;
    }

-    if (data.readableBytes() == 0) {
-      data.release();


Just checking this is no longer needed because of the new cleanup?

vanzin · 2019-11-22T19:16:24Z

Norman, it's not clear whether you wanted your last commit message to be the PR description? Github does not do that automatically.

normanmaurer · 2019-11-22T22:08:39Z

I think the PR description is fine

…

Am 22.11.2019 um 20:16 schrieb Marcelo Vanzin ***@***.***>: Norman, it's not clear whether you wanted your last commit message to be the PR description? Github does not do that automatically. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

vanzin · 2019-11-22T23:20:32Z

Merging to master / 2.4.

vanzin · 2019-11-22T23:26:25Z

2.4 does not have SPARK-26674 so the cherry-pick was not clean (and it's not a super trivial fix). So need to either cherry-pick that change too, or post a fixed PR for 2.4, if you think this is important in that branch.

normanmaurer · 2019-11-23T07:57:28Z

@vanzin let me have a look ... imho this fix is important as it could result in memory leaks

dongjoon-hyun · 2019-11-23T17:58:30Z

Thank you, @vanzin and @normanmaurer .
Yes. Please create a PR against branch-2.4 for branch-2.4 memory leak, @normanmaurer .

…ortCipher` - Correctly release `ByteBuf` in `TransportCipher` in all cases - Move closing / releasing logic to `handlerRemoved(...)` so we are guaranteed that is always called. We need to carefully manage the ownership / lifecycle of `ByteBuf` instances so we don't leak any of these. We did not correctly do this in all cases: - when end up in invalid cipher state. - when partial data was received and the channel is closed before the full frame is decoded Fixes netty/netty#9784. No. Pass the newly added UTs. Closes apache#26609 from normanmaurer/leaks_2_4. Authored-by: Norman Maurer <norman_maurer@apple.com>

normanmaurer · 2019-11-25T08:26:42Z

@dongjoon-hyun @vanzin done #26660

normanmaurer mentioned this pull request Nov 20, 2019

ResourceLeakDetector: LEAK: ByteBuf.release() was not called before it's garbage-collected. netty/netty#9784

Closed

normanmaurer force-pushed the fix_leaks branch from a8420ee to c766150 Compare November 20, 2019 08:51

normanmaurer force-pushed the fix_leaks branch 2 times, most recently from bf1299b to d35f381 Compare November 20, 2019 09:29

vanzin reviewed Nov 20, 2019

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-29971] Fix multiple possible buffer leaks in `TransportFrameDe…~~ [SPARK-29971] Fix multiple possible buffer leaks in TransportFrameDecoder/TransportCipher Nov 20, 2019

dongjoon-hyun added the SPARK CORE label Nov 20, 2019

dongjoon-hyun reviewed Nov 21, 2019

View reviewed changes

common/network-common/src/test/java/org/apache/spark/network/crypto/TransportCipherTest.java Outdated Show resolved Hide resolved

vanzin reviewed Nov 21, 2019

View reviewed changes

dongjoon-hyun reviewed Nov 21, 2019

View reviewed changes

common/network-common/src/test/java/org/apache/spark/network/crypto/TransportCipherTest.java Outdated Show resolved Hide resolved

vanzin reviewed Nov 21, 2019

View reviewed changes

common/network-common/src/test/java/org/apache/spark/network/crypto/TransportCipherTest.java Outdated Show resolved Hide resolved

common/network-common/src/main/java/org/apache/spark/network/crypto/TransportCipher.java Outdated Show resolved Hide resolved

normanmaurer force-pushed the fix_leaks branch from d8d2bbe to fb79d0a Compare November 22, 2019 07:26

dongjoon-hyun changed the title ~~[SPARK-29971] Fix multiple possible buffer leaks in TransportFrameDecoder/TransportCipher~~ [SPARK-29971][CORE] Fix buffer leaks in TransportFrameDecoder/TransportCipher Nov 22, 2019

dongjoon-hyun approved these changes Nov 22, 2019

View reviewed changes

srowen reviewed Nov 22, 2019

View reviewed changes

vanzin closed this in f28eab2 Nov 22, 2019

normanmaurer mentioned this pull request Nov 25, 2019

[SPARK-29971][CORE][2.4] Fix buffer leaks in TransportFrameDecoder/TransportCipher #26660

Closed

normanmaurer deleted the fix_leaks branch November 25, 2019 08:26

[SPARK-29971][CORE] Fix buffer leaks in TransportFrameDecoder/TransportCipher #26609

[SPARK-29971][CORE] Fix buffer leaks in TransportFrameDecoder/TransportCipher #26609

Conversation

normanmaurer commented Nov 20, 2019 • edited by vanzin

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Nov 20, 2019

SparkQA commented Nov 20, 2019

SparkQA commented Nov 20, 2019

SparkQA commented Nov 20, 2019

vanzin left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 20, 2019 • edited

normanmaurer commented Nov 20, 2019

vanzin commented Nov 20, 2019

normanmaurer commented Nov 21, 2019

normanmaurer commented Nov 21, 2019

SparkQA commented Nov 21, 2019

normanmaurer commented Nov 21, 2019

vanzin commented Nov 21, 2019

normanmaurer commented Nov 21, 2019

vanzin commented Nov 21, 2019 • edited

normanmaurer commented Nov 21, 2019

vanzin commented Nov 21, 2019

normanmaurer commented Nov 21, 2019

SparkQA commented Nov 21, 2019

normanmaurer commented Nov 21, 2019

dongjoon-hyun commented Nov 21, 2019

vanzin left a comment

Choose a reason for hiding this comment

normanmaurer commented Nov 21, 2019

SparkQA commented Nov 21, 2019

SparkQA commented Nov 21, 2019

vanzin commented Nov 22, 2019

vanzin commented Nov 22, 2019

dongjoon-hyun commented Nov 22, 2019

vanzin commented Nov 22, 2019

normanmaurer commented Nov 22, 2019

SparkQA commented Nov 22, 2019

SparkQA commented Nov 22, 2019

vanzin commented Nov 22, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Nov 22, 2019

srowen Nov 22, 2019

Choose a reason for hiding this comment

vanzin Nov 22, 2019

Choose a reason for hiding this comment

srowen Nov 22, 2019

Choose a reason for hiding this comment

vanzin commented Nov 22, 2019

normanmaurer commented Nov 22, 2019 via email

vanzin commented Nov 22, 2019

vanzin commented Nov 22, 2019

normanmaurer commented Nov 23, 2019

dongjoon-hyun commented Nov 23, 2019

normanmaurer commented Nov 25, 2019

[SPARK-29971][CORE] Fix buffer leaks in `TransportFrameDecoder/TransportCipher` #26609

[SPARK-29971][CORE] Fix buffer leaks in `TransportFrameDecoder/TransportCipher` #26609

normanmaurer commented Nov 20, 2019 •

edited by vanzin

dongjoon-hyun commented Nov 20, 2019 •

edited

vanzin commented Nov 21, 2019 •

edited