Skip to content

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