You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2021/04/08 08:07:54 UTC
[GitHub] [thrift] unfetteredwind opened a new pull request #2369: Abnormal Branch Memory Leak
unfetteredwind opened a new pull request #2369:
URL: https://github.com/apache/thrift/pull/2369
<!-- Explain the changes in the pull request below: -->
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ ] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] unfetteredwind edited a comment on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
unfetteredwind edited a comment on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-823735239
OK, first of all, let me briefly explain why this judgment condition is written.
（Number is the number of lines of code）
There are two writing conditions:
1、The value of allocate is true.
2、The type is not TYPE_STRING type in the base type.
1、In this document, the function generate_deserialize_field is invoked in the following four locations:
struct:generate_struct_reader allocate：default value: false 3579 3664
map: generate_deserialize_map_element allocate：default value: true 4271 4294
set: generate_deserialize_set_element allocate：default value: true 4305 4316
list: generate_deserialize_list_element allocate：default value: true 4325 4338
Check the function invoked by each. If the value of allocate is false, new is not triggered(declare_local_variable is not invoked in the struct ). In this case, free is not required.
When the value of allocate is true, the declare_local_variable function is invoked in the map, set, and list to trigger new. In this case, you need to perform the free operation when an exception occurs.
So that's one of my judgements.
2、And going back to this function : generate_deserialize_list_element.
The judgment condition is in the base_type branch. 4020
As you can see, if the type is TYPE_STRING, there is no leakage problem in this part,。Because he did special treatment., 4023-4028.
For the remaining types, if they enter the exception branch after new, memory leakage will be triggered.
That's another criterion for me. If the condition is met, it needs to be processed.
That's why I chose the condition. And, perhaps the judgment is not rigorous enough, and my intention is to raise this issue so that people can see this issue and push this issue to be resolved.
As for the test..., it's working on it.And....I'm lazy. Excuse me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G edited a comment on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-821577114
Is this related to THRIFT-5399 maybe? A bit more information could really help to move on with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] unfetteredwind commented on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
unfetteredwind commented on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-823735239
OK, first of all, let me briefly explain why this judgment condition is written.
There are two writing conditions:
1、The value of allocate is true.
2、The type is not TYPE_STRING type in the base type.
1、In this document, the function generate_deserialize_field is invoked in the following four locations:
struct:generate_struct_reader allocate：default value: false 3579 3664
map: generate_deserialize_map_element allocate：default value: true 4271 4294
set: generate_deserialize_set_element allocate：default value: true 4305 4316
list: generate_deserialize_list_element allocate：default value: true 4325 4338
Check the function invoked by each. If the value of allocate is false, new is not triggered(declare_local_variable is not invoked in the struct ). In this case, free is not required.
When the value of allocate is true, the declare_local_variable function is invoked in the map, set, and list to trigger new. In this case, you need to perform the free operation when an exception occurs.
So that's one of my judgements.
2、And going back to this function : generate_deserialize_list_element.
The judgment condition is in the base_type branch. 4020
As you can see, if the type is TYPE_STRING, there is no leakage problem in this part,。Because he did special treatment., 4023-4028.
For the remaining types, if they enter the exception branch after new, memory leakage will be triggered.
That's another criterion for me. If the condition is met, it needs to be processed.
That's why I chose the condition. And, perhaps the judgment is not rigorous enough, and my intention is to raise this issue so that people can see this issue and push this issue to be resolved.
As for the test..., it's working on it.And....I'm lazy. Excuse me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] unfetteredwind commented on a change in pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
unfetteredwind commented on a change in pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#discussion_r610294733
##########
File path: compiler/cpp/src/thrift/generate/t_c_glib_generator.cc
##########
@@ -4061,8 +4061,17 @@ void t_c_glib_generator::generate_deserialize_field(ostream& out,
throw "compiler error: no C reader for base type " + t_base_type::t_base_name(tbase) + name;
}
out << ", error)) < 0)" << endl;
- out << indent() << " return " << error_ret << ";" << endl << indent() << "xfer += ret;"
- << endl;
+ if ((tbase != t_base_type::TYPE_STRING) && allocate) {
Review comment:
4022 is :
If it's not equal t_base_type::TYPE_STRING , I've increased the processing。The two are different,and I think it's an omission.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-818988491
> That's indeed an issue.
I was not able to trigger the case that would invoke the new code with my test case. So even if it is a problem, the solution seems not to work at all. Essentially I got the same code generated before and after. That's why I asked for a test case in the first place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ulidtko commented on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
ulidtko commented on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-822393437
> Is this related to THRIFT-5399 maybe? A bit more information could really help to move on with this.
No, that's different AFAICS. There, a socket leaks on `listen` failure; here, a memory allocation leaks on deserialization failure.
I suppose, repro of this issue involves running a `c_glib` client on intentionally malformed buffers & under a leak detector (asan/valgrind). Then, the leak would trigger whenever we expect to read an i32 but get something else instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-818207650
Could you provide a test case that shows the issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] unfetteredwind commented on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
unfetteredwind commented on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-815576909
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] unfetteredwind edited a comment on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
unfetteredwind edited a comment on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-815576909
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#issuecomment-821577114
Is this related to THRIFT-5399 maybe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a change in pull request #2369: Abnormal Branch Memory Leak
Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2369:
URL: https://github.com/apache/thrift/pull/2369#discussion_r610159473
##########
File path: compiler/cpp/src/thrift/generate/t_c_glib_generator.cc
##########
@@ -4061,8 +4061,17 @@ void t_c_glib_generator::generate_deserialize_field(ostream& out,
throw "compiler error: no C reader for base type " + t_base_type::t_base_name(tbase) + name;
}
out << ", error)) < 0)" << endl;
- out << indent() << " return " << error_ret << ";" << endl << indent() << "xfer += ret;"
- << endl;
+ if ((tbase != t_base_type::TYPE_STRING) && allocate) {
Review comment:
Why is that test different to line 4022 (allocate is not tested there)? Shouldn't that be consistent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org