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

Fix channel credentials memory leak in shims #369

Merged
merged 1 commit into from
Feb 26, 2019

Conversation

kevints
Copy link
Contributor

@kevints kevints commented Feb 14, 2019

No description provided.

@kevints
Copy link
Contributor Author

kevints commented Feb 14, 2019

Tracing through the code it does seem that grpc_secure_channel_create copies the credentials object, but I can't find that documented anywhere so I need someone more knowledgable about the grpc-core implementation to review this PR. Cc @timburks @MrMage

In the case that it doesn't we could maintain it as a property on cgrpc_channel and release it in cgrpc_channel_destroy.

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Anyone else we should ask to review this?

@MrMage
Copy link
Collaborator

MrMage commented Feb 25, 2019

Given that @kevints and I have both traced through the code and think that releasing channel credentials after use is "correct" (because they are retained/copied by the channel), shall we merge this? @rebello95, what do you think?

@rebello95
Copy link
Collaborator

Sure, I’m fine with merging if you’re good with the change 👍 (not super familiar with this piece of code myself)

@MrMage
Copy link
Collaborator

MrMage commented Feb 26, 2019

Merging this now.

@MrMage MrMage merged commit 587218a into grpc:master Feb 26, 2019
@kevints kevints deleted the fix-credentials-leak branch April 11, 2019 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants