[
https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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, <<Val:8/integer-signed-big, _/binary>>} -> {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, <<Val:16/integer-signed-big, _/binary>>} -> {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, <<Val:32/integer-signed-big, _/binary>>} -> {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, <<Val:32/integer-unsigned-big, _/binary>>} -> {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, <<Val:64/integer-signed-big, _/binary>>} -> {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, <<Val:64/float-signed-big, _/binary>>} -> {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} = thrift_transport:read(Trans, Len),
+ Result = case Result0 of
+ {ok, Bin} when is_binary(Bin), Clone ->
+ {ok, binary:copy(Bin)};
+ _ ->
+ Result0
+ end,
{This#binary_protocol{transport = NewTransport}, Result}.
%%%% FACTORY GENERATION %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
-record(tbp_opts, {strict_read = true,
- strict_write = true}).
+ strict_write = true,
+ clone_binaries = true}).
parse_factory_options([], Opts) ->
Opts;
parse_factory_options([{strict_read, Bool} | Rest], Opts) when
is_boolean(Bool) ->
parse_factory_options(Rest, Opts#tbp_opts{strict_read=Bool});
parse_factory_options([{strict_write, Bool} | Rest], Opts) when
is_boolean(Bool) ->
- parse_factory_options(Rest, Opts#tbp_opts{strict_write=Bool}).
-
+ parse_factory_options(Rest, Opts#tbp_opts{strict_write=Bool});
+parse_factory_options([{clone_binaries, Bool} | Rest], Opts) when
is_boolean(Bool) ->
+ parse_factory_options(Rest, Opts#tbp_opts{clone_binaries=Bool}).
%% returns a (fun() -> thrift_protocol())
new_protocol_factory(TransportFactory, Options) ->
@@ -337,11 +349,11 @@ new_protocol_factory(TransportFactory, Options) ->
{ok, Transport} ->
thrift_binary_protocol:new(
Transport,
- [{strict_read, ParsedOpts#tbp_opts.strict_read},
- {strict_write,
ParsedOpts#tbp_opts.strict_write}]);
+ [{strict_read, ParsedOpts#tbp_opts.strict_read},
+ {strict_write,
ParsedOpts#tbp_opts.strict_write},
+ {clone_binaries,
ParsedOpts#tbp_opts.clone_binaries}]);
{error, Error} ->
{error, Error}
end
end,
{ok, F}.
-
diff --git a/lib/erl/src/thrift_compact_protocol.erl
b/lib/erl/src/thrift_compact_protocol.erl
index 0f14221914..335b6f06ca 100644
--- a/lib/erl/src/thrift_compact_protocol.erl
+++ b/lib/erl/src/thrift_compact_protocol.erl
@@ -42,7 +42,8 @@
read_stack=[],
read_value=?CBOOL_NONE,
write_stack=[],
- write_id=?ID_NONE
+ write_id=?ID_NONE,
+ clone_binaries=true
}).
-type state() :: #t_compact{}.
-include("thrift_protocol_behaviour.hrl").
@@ -87,7 +88,8 @@ cbool_to_bool(Value) -> Value =:= ?CBOOL_TRUE.
new(Transport) -> new(Transport, _Options = []).
-new(Transport, _Options) ->
+new(Transport, Options) ->
+ CloneBinaries = proplists:get_value(clone_binaries, Options, true),
State = #t_compact{transport = Transport},
thrift_protocol:new(?MODULE, State).
@@ -329,11 +331,11 @@ read(This0 = #t_compact{read_value = Bool}, bool) ->
{This0#t_compact{read_value = ?CBOOL_NONE}, {ok, Bool}};
read(This0, ubyte) ->
- {This1, {ok, <<Val:8/integer-unsigned-big, _/binary>>}} = read_data(This0,
1),
+ {This1, {ok, <<Val:8/integer-unsigned-big, _/binary>>}} = read_data(This0,
1, false),
{This1, {ok, Val}};
read(This0, byte) ->
- {This1, Bytes} = read_data(This0, 1),
+ {This1, Bytes} = read_data(This0, 1, false),
case Bytes of
{ok, <<Val:8/integer-signed-big, _/binary>>} -> {This1, {ok, Val}};
Else -> {This1, Else}
@@ -354,7 +356,7 @@ read(This0, i64) ->
{This1, {ok, from_zigzag(Zigzag)}};
read(This0, double) ->
- {This1, Bytes} = read_data(This0, 8),
+ {This1, Bytes} = read_data(This0, 8, false),
case Bytes of
{ok, <<Val:64/float-signed-little, _/binary>>} -> {This1, {ok, Val}};
Else -> {This1, Else}
@@ -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 ->
+ {ok, binary:copy(Bin)};
+ _ ->
+ Result0
+ end,
{This#t_compact{transport = NewTransport}, Result}.
-
%%%% FACTORY GENERATION %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%% returns a (fun() -> thrift_protocol())
-new_protocol_factory(TransportFactory, _Options) ->
+new_protocol_factory(TransportFactory, Options) ->
F = fun() ->
case TransportFactory() of
{ok, Transport} ->
thrift_compact_protocol:new(
Transport,
- []);
+ Options);
{error, Error} ->
{error, Error}
end
diff --git a/lib/erl/src/thrift_processor.erl b/lib/erl/src/thrift_processor.erl
index 5c9f26f52a..c4adb1942e 100644
--- a/lib/erl/src/thrift_processor.erl
+++ b/lib/erl/src/thrift_processor.erl
@@ -53,18 +53,18 @@ loop(State0 = #thrift_processor{protocol = Proto0,
[ServiceName, FunctionName] ->
ServiceModule =
thrift_multiplexed_map_wrapper:fetch(ServiceName, Service),
ServiceHandler =
thrift_multiplexed_map_wrapper:fetch(ServiceName, Handler),
- case
handle_function(State1#thrift_processor{service=ServiceModule,
handler=ServiceHandler}, list_to_atom(FunctionName), Seqid) of
+ case
handle_function(State1#thrift_processor{service=ServiceModule,
handler=ServiceHandler}, list_to_existing_atom(FunctionName), Seqid) of
{State2, ok} ->
loop(State2#thrift_processor{service=Service, handler=Handler});
{_State2, {error, Reason}} ->
-
apply(ErrorHandler(Handler), handle_error, [list_to_atom(Function), Reason]),
+
apply(ErrorHandler(Handler), handle_error, [list_to_existing_atom(Function),
Reason]),
thrift_protocol:close_transport(Proto1),
ok
end;
_ ->
- case handle_function(State1, list_to_atom(Function),
Seqid) of
+ case handle_function(State1,
list_to_existing_atom(Function), Seqid) of
{State2, ok} -> loop(State2);
{_State2, {error, Reason}} ->
-
apply(ErrorHandler(Handler), handle_error, [list_to_atom(Function), Reason]),
+
apply(ErrorHandler(Handler), handle_error, [list_to_existing_atom(Function),
Reason]),
thrift_protocol:close_transport(Proto1),
ok
end
----------------------------------------------------------------
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:
[email protected]
> 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)
