Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase reference count on state used in tcp connect. #12733

Merged

Conversation

vosst
Copy link
Contributor

@vosst vosst commented Sep 27, 2017

The state is used both in the callback for the actual connect as well as
in the additional timeout that is setup for the operation. Both code
paths decrease the reference count and if they happen to be queued at
the same time, memory is corrupted. Subsequent behavior is undefined and
segfaults can be observed as a result.

Fixes #12608

The state is used both in the callback for the actual connect as well as
in the additional timeout that is setup for the operation. Both code
paths decrease the reference count and if they happen to be queued at
the same time, memory is corrupted. Subsequent behavior is undefined and
segfaults can be observed as a result.

Fixes grpc#12608
@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@vosst
Copy link
Contributor Author

vosst commented Sep 27, 2017

Please see the following gists for valgrind results:

These are obviously not conclusive but seem to give a good indication that the fix is valid and does not lead to a memory leak. Happy to provide further details.

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__2M_2_0.opt.old: 1


[microbenchmarks] No significant performance differences

@vosst
Copy link
Contributor Author

vosst commented Sep 29, 2017

@murgatroid99 I see the tests failing, happy to investigate. Is there any way I can get access to the test results or reproduce the failures on my local machine?

@murgatroid99
Copy link
Member

Some of those failures are definitely flakes. But the required test is failing because of a clang format error: it wants an extra space between the code and the // at the beginning of your comment.

@vosst
Copy link
Contributor Author

vosst commented Sep 30, 2017

Thanks, fixed up the whitespace.

@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@murgatroid99 murgatroid99 merged commit 5abced9 into grpc:master Oct 3, 2017
@murgatroid99 murgatroid99 mentioned this pull request Oct 3, 2017
@vosst
Copy link
Contributor Author

vosst commented Oct 4, 2017

@murgatroid99 Thanks for merging and for kicking off the backport to 1.7.x. Any idea when a release with the fix included would hit https://www.npmjs.com/package/grpc?

@murgatroid99
Copy link
Member

I would expect within the next couple of weeks. This came in the middle of some changes we're making to our repository structure and release process, so there may be a bit of a delay.

apolcyn added a commit that referenced this pull request Oct 4, 2017
@murgatroid99
Copy link
Member

I've published 1.6.6 with this fix.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants