[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[
https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16657871#comment-16657871
]
ASF GitHub Bot commented on THRIFT-4624:
k32 closed pull request #1585: THRIFT-4624: Fix refc binary leak
URL: https://github.com/apache/thrift/pull/1585
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/lib/erl/src/thrift_binary_protocol.erl
b/lib/erl/src/thrift_binary_protocol.erl
index 85abb62d23..744774f663 100644
--- a/lib/erl/src/thrift_binary_protocol.erl
+++ b/lib/erl/src/thrift_binary_protocol.erl
@@ -35,7 +35,8 @@
-record(binary_protocol, {transport,
strict_read=true,
- strict_write=true
+ strict_write=true,
+ clone_binaries=true
}).
-type state() :: #binary_protocol{}.
-include("thrift_protocol_behaviour.hrl").
@@ -57,7 +58,9 @@ parse_options([], State) ->
parse_options([{strict_read, Bool} | Rest], State) when is_boolean(Bool) ->
parse_options(Rest, State#binary_protocol{strict_read=Bool});
parse_options([{strict_write, Bool} | Rest], State) when is_boolean(Bool) ->
-parse_options(Rest, State#binary_protocol{strict_write=Bool}).
+parse_options(Rest, State#binary_protocol{strict_write=Bool});
+parse_options([{clone_binaries, Bool} | Rest], State) when is_boolean(Bool) ->
+parse_options(Rest, State#binary_protocol{clone_binaries=Bool}).
flush_transport(This = #binary_protocol{transport = Transport}) ->
@@ -172,6 +175,7 @@ write(This = #binary_protocol{transport = Trans}, Data) ->
%%
read(This0, message_begin) ->
+Clone = This0#binary_protocol.clone_binaries,
{This1, Initial} = read(This0, ui32),
case Initial of
{ok, Sz} when Sz band ?VERSION_MASK =:= ?VERSION_1 ->
@@ -193,7 +197,7 @@ read(This0, message_begin) ->
{ok, Sz} when This1#binary_protocol.strict_read =:= false ->
%% strict_read is false, so just read the old way
-{This2, {ok, Name}} = read_data(This1, Sz),
+{This2, {ok, Name}} = read_data(This1, Sz, Clone),
{This3, {ok, Type}} = read(This2, byte),
{This4, {ok, SeqId}} = read(This3, i32),
{This4, #protocol_message_begin{name = binary_to_list(Name),
@@ -259,21 +263,21 @@ read(This0, bool) ->
end;
read(This0, byte) ->
-{This1, Bytes} = read_data(This0, 1),
+{This1, Bytes} = read_data(This0, 1, false),
case Bytes of
{ok, <>} -> {This1, {ok, Val}};
Else -> {This1, Else}
end;
read(This0, i16) ->
-{This1, Bytes} = read_data(This0, 2),
+{This1, Bytes} = read_data(This0, 2, false),
case Bytes of
{ok, <>} -> {This1, {ok, Val}};
Else -> {This1, Else}
end;
read(This0, i32) ->
-{This1, Bytes} = read_data(This0, 4),
+{This1, Bytes} = read_data(This0, 4, false),
case Bytes of
{ok, <>} -> {This1, {ok, Val}};
Else -> {This1, Else}
@@ -283,21 +287,21 @@ read(This0, i32) ->
%% of the packet version header. Without this special function BEAM works fine
%% but hipe thinks it received a bad version header.
read(This0, ui32) ->
-{This1, Bytes} = read_data(This0, 4),
+{This1, Bytes} = read_data(This0, 4, false),
case Bytes of
{ok, <>} -> {This1, {ok, Val}};
Else -> {This1, Else}
end;
read(This0, i64) ->
-{This1, Bytes} = read_data(This0, 8),
+{This1, Bytes} = read_data(This0, 8, false),
case Bytes of
{ok, <>} -> {This1, {ok, Val}};
Else -> {This1, Else}
end;
read(This0, double) ->
-{This1, Bytes} = read_data(This0, 8),
+{This1, Bytes} = read_data(This0, 8, false),
case Bytes of
{ok, <>} -> {This1, {ok, Val}};
Else -> {This1, Else}
@@ -306,28 +310,36 @@ read(This0, double) ->
% returns a binary directly, call binary_to_list if necessary
read(This0, string) ->
{This1, {ok, Sz}} = read(This0, i32),
-read_data(This1, Sz).
+read_data(This1, Sz, This0#binary_protocol.clone_binaries).
--spec read_data(#binary_protocol{}, non_neg_integer()) ->
+-spec read_data(#binary_protocol{}, non_neg_integer(), boolean()) ->
{#binary_protocol{}, {ok, binary()} | {error, _Reason}}.
-read_data(This, 0) -> {This, {ok, <<>>}};
-read_data(This = #binary_protocol{transport = Trans}, Len) when
is_integer(Len) andalso Len > 0 ->
-{NewTransport, Result} = thrift_transport:read(Trans, Len),
+read_data(This, 0, _) -> {This, {ok, <<>>}};
+read_data(This = #binary_protocol{transport = Trans}, Len, Clone) when
is_integer(Len) andalso Len > 0 ->
+{NewTransport, Result0} =
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16656719#comment-16656719 ] ASF GitHub Bot commented on THRIFT-4624: jeking3 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-431341038 If you want to pare down the PR to just the changes in `lib/erl/src/thrift_processor.erl` and it passes the CI build, I can merge that. I'm not sure the clone changes are needed per comments. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16631760#comment-16631760 ] ASF GitHub Bot commented on THRIFT-4624: jeking3 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-425416206 @djnym any thoughts about the comments regarding atom and DOS attacks? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16616670#comment-16616670 ] ASF GitHub Bot commented on THRIFT-4624: jeking3 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-421745316 Are we all saying the changes in `lib/erl/src/thrift_processor.erl` are good but the others are unnecessary and may force additional copies of things unnecessarily? Sorry I don't know erlang and no committer has stepped up to review. Also `ox-thrift` looks interesting, but does not support as many transports or protocols as what exists here today and I assume the interface changed. If it's that much better though (5-10x faster?) we should have a discussion about bringing that in, if the author desires it. As for this one, I'm still not sure what to do because nobody made it clear. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16613869#comment-16613869 ] ASF GitHub Bot commented on THRIFT-4624: k32 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-421101392 I found these errors via code review, while evaluating this library and doing some rapid prototyping. I'm not using Thrift in production. However, I would recommend merging at least part changing list_to_atom to list_to_existing_atom, because (if it wasn't obvious) it fixes a pretty bad DOS vulnerability, allowing attacker to take down the entire Erlang VM by filling up atom table. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16613365#comment-16613365 ] ASF GitHub Bot commented on THRIFT-4624: jeking3 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-420978881 I'm curious to know the answer to: "Is there a case where you are actually seeing this leakage occuring?" This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16602637#comment-16602637 ] ASF GitHub Bot commented on THRIFT-4624: k32 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-418250469 @djnym Thanks for the suggestion; on the first glance ox-thrift looks very promising. Regarding the defaults: it's just "safety first" approach. This can be changed. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16602614#comment-16602614 ] ASF GitHub Bot commented on THRIFT-4624: djnym commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-418246218 The code seems reasonable, but I'm wondering of the need. Do you actually see refc binary leakage? I could maybe see that happen if you have long lived connections, from which you subtract a smallish portion of the binary from a much larger thrift object and pass it around, but I'm not totally certain. Is there a case where you are actually seeing this leakage occuring? @k32 Have you checked out https://github.com/openx/ox-thrift which @dhull wrote? It uses the ranch acceptor pool instead of the standard server and is a complete rewrite of the message encoding/decoding. It's about 5-10x faster at encoding/decoding, so it's an improvement. The only way to make it faster would be to rewrite the compiler which @dhull has looked at but not done (the idea would be to generate a custom module for encoding/decoding instead of the generic structures which then get interpreted, this should lead to even more efficiency as you'd not have to go through an intermediate representation). Anyway, maybe David will take a look at this, but I think it's probably fine to merge. My only real comment would be that this appears to make the default copying the binaries, which could be a surprise if people aren't expecting it and are moving large thrift structures around. So it might be worthwhile to document that somewhere, not sure where though. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16602219#comment-16602219 ] ASF GitHub Bot commented on THRIFT-4624: k32 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-418127936 No hurry This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16602113#comment-16602113 ] ASF GitHub Bot commented on THRIFT-4624: jeking3 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-418103102 All the tests pass however I'd like one independent code review before merging if possible. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[
https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16601193#comment-16601193
]
ASF GitHub Bot commented on THRIFT-4624:
k32 commented on a change in pull request #1585: THRIFT-4624: Fix refc binary
leak
URL: https://github.com/apache/thrift/pull/1585#discussion_r214538830
##
File path: lib/erl/src/thrift_compact_protocol.erl
##
@@ -363,26 +365,32 @@ read(This0, double) ->
% returns a binary directly, call binary_to_list if necessary
read(This0, string) ->
{This1, {ok, Sz}} = read(This0, ui32),
- read_data(This1, Sz).
+ read_data(This1, Sz, This0#t_compact.clone_binaries).
--spec read_data(#t_compact{}, non_neg_integer()) ->
+-spec read_data(#t_compact{}, non_neg_integer(), boolean()) ->
{#t_compact{}, {ok, binary()} | {error, _Reason}}.
-read_data(This, 0) -> {This, {ok, <<>>}};
-read_data(This = #t_compact{transport = Trans}, Len) when is_integer(Len)
andalso Len > 0 ->
-{NewTransport, Result} = thrift_transport:read(Trans, Len),
+read_data(This, 0, _) -> {This, {ok, <<>>}};
+read_data(This, Len, Clone) when is_integer(Len) andalso Len > 0 ->
+#t_compact{transport = Trans} = This,
+{NewTransport, Result0} = thrift_transport:read(Trans, Len),
+Result = case Result0 of
+ {ok, Bin} when is_binary(Bin), Clone, Len > 64 ->
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Refc binary leak in binary and compact protocols
>
>
> Key: THRIFT-4624
> URL: https://issues.apache.org/jira/browse/THRIFT-4624
> Project: Thrift
> Issue Type: Improvement
> Components: Erlang - Library
>Reporter: something
>Priority: Major
>
> Pattern-matching on large (>64B) Erlang binaries merely produces slices of
> objects on the Refc heap. Therefore Thrift binary and compact protocols
> should clone all binaries they send to upper levels, otherwise there's a
> chance that transport-level messages will be never freed.
> Patch is underway.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[
https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16601187#comment-16601187
]
ASF GitHub Bot commented on THRIFT-4624:
jeking3 commented on a change in pull request #1585: THRIFT-4624: Fix refc
binary leak
URL: https://github.com/apache/thrift/pull/1585#discussion_r214537979
##
File path: lib/erl/src/thrift_compact_protocol.erl
##
@@ -363,26 +365,32 @@ read(This0, double) ->
% returns a binary directly, call binary_to_list if necessary
read(This0, string) ->
{This1, {ok, Sz}} = read(This0, ui32),
- read_data(This1, Sz).
+ read_data(This1, Sz, This0#t_compact.clone_binaries).
--spec read_data(#t_compact{}, non_neg_integer()) ->
+-spec read_data(#t_compact{}, non_neg_integer(), boolean()) ->
{#t_compact{}, {ok, binary()} | {error, _Reason}}.
-read_data(This, 0) -> {This, {ok, <<>>}};
-read_data(This = #t_compact{transport = Trans}, Len) when is_integer(Len)
andalso Len > 0 ->
-{NewTransport, Result} = thrift_transport:read(Trans, Len),
+read_data(This, 0, _) -> {This, {ok, <<>>}};
+read_data(This, Len, Clone) when is_integer(Len) andalso Len > 0 ->
+#t_compact{transport = Trans} = This,
+{NewTransport, Result0} = thrift_transport:read(Trans, Len),
+Result = case Result0 of
+ {ok, Bin} when is_binary(Bin), Clone, Len > 64 ->
Review comment:
It sounded like you wanted to make one more change based on your original
comment?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Refc binary leak in binary and compact protocols
>
>
> Key: THRIFT-4624
> URL: https://issues.apache.org/jira/browse/THRIFT-4624
> Project: Thrift
> Issue Type: Improvement
> Components: Erlang - Library
>Reporter: something
>Priority: Major
>
> Pattern-matching on large (>64B) Erlang binaries merely produces slices of
> objects on the Refc heap. Therefore Thrift binary and compact protocols
> should clone all binaries they send to upper levels, otherwise there's a
> chance that transport-level messages will be never freed.
> Patch is underway.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[
https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16601182#comment-16601182
]
ASF GitHub Bot commented on THRIFT-4624:
k32 commented on a change in pull request #1585: THRIFT-4624: Fix refc binary
leak
URL: https://github.com/apache/thrift/pull/1585#discussion_r214537486
##
File path: lib/erl/src/thrift_compact_protocol.erl
##
@@ -363,26 +365,32 @@ read(This0, double) ->
% returns a binary directly, call binary_to_list if necessary
read(This0, string) ->
{This1, {ok, Sz}} = read(This0, ui32),
- read_data(This1, Sz).
+ read_data(This1, Sz, This0#t_compact.clone_binaries).
--spec read_data(#t_compact{}, non_neg_integer()) ->
+-spec read_data(#t_compact{}, non_neg_integer(), boolean()) ->
{#t_compact{}, {ok, binary()} | {error, _Reason}}.
-read_data(This, 0) -> {This, {ok, <<>>}};
-read_data(This = #t_compact{transport = Trans}, Len) when is_integer(Len)
andalso Len > 0 ->
-{NewTransport, Result} = thrift_transport:read(Trans, Len),
+read_data(This, 0, _) -> {This, {ok, <<>>}};
+read_data(This, Len, Clone) when is_integer(Len) andalso Len > 0 ->
+#t_compact{transport = Trans} = This,
+{NewTransport, Result0} = thrift_transport:read(Trans, Len),
+Result = case Result0 of
+ {ok, Bin} when is_binary(Bin), Clone, Len > 64 ->
Review comment:
I'm not sure how one can reliably test this, as refc/heap binary dichotomy
is hidden in the Erlang VM internals and there is no API to check which is
which. The only (known to me) way to detect refc memory leak is through memory
region monitoring, and it's beyond scope of unit/component tests. So regression
testing is probably the best we can do.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Refc binary leak in binary and compact protocols
>
>
> Key: THRIFT-4624
> URL: https://issues.apache.org/jira/browse/THRIFT-4624
> Project: Thrift
> Issue Type: Improvement
> Components: Erlang - Library
>Reporter: something
>Priority: Major
>
> Pattern-matching on large (>64B) Erlang binaries merely produces slices of
> objects on the Refc heap. Therefore Thrift binary and compact protocols
> should clone all binaries they send to upper levels, otherwise there's a
> chance that transport-level messages will be never freed.
> Patch is underway.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[
https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16601179#comment-16601179
]
ASF GitHub Bot commented on THRIFT-4624:
jeking3 commented on a change in pull request #1585: THRIFT-4624: Fix refc
binary leak
URL: https://github.com/apache/thrift/pull/1585#discussion_r214536571
##
File path: lib/erl/src/thrift_compact_protocol.erl
##
@@ -363,26 +365,32 @@ read(This0, double) ->
% returns a binary directly, call binary_to_list if necessary
read(This0, string) ->
{This1, {ok, Sz}} = read(This0, ui32),
- read_data(This1, Sz).
+ read_data(This1, Sz, This0#t_compact.clone_binaries).
--spec read_data(#t_compact{}, non_neg_integer()) ->
+-spec read_data(#t_compact{}, non_neg_integer(), boolean()) ->
{#t_compact{}, {ok, binary()} | {error, _Reason}}.
-read_data(This, 0) -> {This, {ok, <<>>}};
-read_data(This = #t_compact{transport = Trans}, Len) when is_integer(Len)
andalso Len > 0 ->
-{NewTransport, Result} = thrift_transport:read(Trans, Len),
+read_data(This, 0, _) -> {This, {ok, <<>>}};
+read_data(This, Len, Clone) when is_integer(Len) andalso Len > 0 ->
+#t_compact{transport = Trans} = This,
+{NewTransport, Result0} = thrift_transport:read(Trans, Len),
+Result = case Result0 of
+ {ok, Bin} when is_binary(Bin), Clone, Len > 64 ->
Review comment:
Are we missing a test case then? :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Refc binary leak in binary and compact protocols
>
>
> Key: THRIFT-4624
> URL: https://issues.apache.org/jira/browse/THRIFT-4624
> Project: Thrift
> Issue Type: Improvement
> Components: Erlang - Library
>Reporter: something
>Priority: Major
>
> Pattern-matching on large (>64B) Erlang binaries merely produces slices of
> objects on the Refc heap. Therefore Thrift binary and compact protocols
> should clone all binaries they send to upper levels, otherwise there's a
> chance that transport-level messages will be never freed.
> Patch is underway.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[
https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16601153#comment-16601153
]
ASF GitHub Bot commented on THRIFT-4624:
k32 commented on a change in pull request #1585: THRIFT-4624: Fix refc binary
leak
URL: https://github.com/apache/thrift/pull/1585#discussion_r214534468
##
File path: lib/erl/src/thrift_compact_protocol.erl
##
@@ -363,26 +365,32 @@ read(This0, double) ->
% returns a binary directly, call binary_to_list if necessary
read(This0, string) ->
{This1, {ok, Sz}} = read(This0, ui32),
- read_data(This1, Sz).
+ read_data(This1, Sz, This0#t_compact.clone_binaries).
--spec read_data(#t_compact{}, non_neg_integer()) ->
+-spec read_data(#t_compact{}, non_neg_integer(), boolean()) ->
{#t_compact{}, {ok, binary()} | {error, _Reason}}.
-read_data(This, 0) -> {This, {ok, <<>>}};
-read_data(This = #t_compact{transport = Trans}, Len) when is_integer(Len)
andalso Len > 0 ->
-{NewTransport, Result} = thrift_transport:read(Trans, Len),
+read_data(This, 0, _) -> {This, {ok, <<>>}};
+read_data(This, Len, Clone) when is_integer(Len) andalso Len > 0 ->
+#t_compact{transport = Trans} = This,
+{NewTransport, Result0} = thrift_transport:read(Trans, Len),
+Result = case Result0 of
+ {ok, Bin} when is_binary(Bin), Clone, Len > 64 ->
Review comment:
The last condition should be removed. Cursory look at the BEAM code revealed
that only brand new binaries can be allocated on heap, no copying is done for
slices
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Refc binary leak in binary and compact protocols
>
>
> Key: THRIFT-4624
> URL: https://issues.apache.org/jira/browse/THRIFT-4624
> Project: Thrift
> Issue Type: Improvement
> Components: Erlang - Library
>Reporter: something
>Priority: Major
>
> Pattern-matching on large (>64B) Erlang binaries merely produces slices of
> objects on the Refc heap. Therefore Thrift binary and compact protocols
> should clone all binaries they send to upper levels, otherwise there's a
> chance that transport-level messages will be never freed.
> Patch is underway.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16599879#comment-16599879 ] ASF GitHub Bot commented on THRIFT-4624: jeking3 commented on issue #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585#issuecomment-417898482 I wonder if @walter-weinmann or @dhull could take a look at this perhaps? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
[ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16592903#comment-16592903 ] ASF GitHub Bot commented on THRIFT-4624: k32 opened a new pull request #1585: THRIFT-4624: Fix refc binary leak URL: https://github.com/apache/thrift/pull/1585 Client: erl This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > Refc binary leak in binary and compact protocols > > > Key: THRIFT-4624 > URL: https://issues.apache.org/jira/browse/THRIFT-4624 > Project: Thrift > Issue Type: Improvement > Components: Erlang - Library >Reporter: something >Priority: Major > > Pattern-matching on large (>64B) Erlang binaries merely produces slices of > objects on the Refc heap. Therefore Thrift binary and compact protocols > should clone all binaries they send to upper levels, otherwise there's a > chance that transport-level messages will be never freed. > Patch is underway. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
