Skip to content

[Security] Move ownership of tsi_ssl_client_handshaker_factory to grpc_ssl_credentials, version 2.#34408

Merged
gtcooke94 merged 2 commits intogrpc:masterfrom
gtcooke94:SSL_CTX_new_refactor_2
Sep 21, 2023
Merged

[Security] Move ownership of tsi_ssl_client_handshaker_factory to grpc_ssl_credentials, version 2.#34408
gtcooke94 merged 2 commits intogrpc:masterfrom
gtcooke94:SSL_CTX_new_refactor_2

Conversation

@gtcooke94
Copy link
Copy Markdown
Contributor

@gtcooke94 gtcooke94 commented Sep 19, 2023 •
edited
Loading

Revert the reversion of the SSL_CTX_new change (#34355 reverted #34180 ) with a fix.

There was an issue with using strcpy on a new[] string in the constructor of ssl_credentials. An ASAN test caught this in some CI down the line - ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs free)

That strcpy call was changed to grp_strdup which duplicates a string in a way that can be freed by gpr_free and should resolve the ASAN failure.

@gtcooke94 gtcooke94 added area/security release notes: no Indicates if PR should not be in release notes labels Sep 19, 2023
@matthewstevenson88 matthewstevenson88 changed the title [Security] SSL_CTX_new refactor 2 [Security] Move ownership of tsi_ssl_client_handshaker_factory to grpc_ssl_credentials, version 2. Sep 19, 2023
@gtcooke94 gtcooke94 merged commit aa17285 into grpc:master Sep 21, 2023
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 21, 2023
@gtcooke94 gtcooke94 mentioned this pull request Sep 25, 2023
Closed
@XuanWang-Amos XuanWang-Amos mentioned this pull request Oct 12, 2023
Closed
nanahpang added a commit that referenced this pull request Oct 16, 2023
@nanahpang
Revert "[Security] Move ownership of tsi_ssl_client_handshaker_factor…
d896444 
…y to grpc_ssl_credentials, version 2. (#34408)"

This reverts commit aa17285.
@nanahpang nanahpang mentioned this pull request Oct 16, 2023
Merged
nanahpang added a commit that referenced this pull request Oct 16, 2023
@nanahpang
Revert "[Security] Move ownership of tsi_ssl_client_handshaker_factor…
601aaf8 
…y to grpc_ssl_credentials, version 2." (#34701)

Reverts #34408
@gtcooke94 gtcooke94 mentioned this pull request Oct 18, 2023
Merged
gtcooke94 added a commit that referenced this pull request Oct 18, 2023
@gtcooke94
[Security] Reapply Move ownership of tsi_ssl_client_handshaker_factory (
74c1de6 
#34726)

This reverts commit 601aaf8, which
results in rolling forward #34408.

The 601aaf reversion happened because of a deadlock found in Python. The
root cause ended up being an issue with the Python wrapper and was fixed
in #34712 , so this can be rolled forward again
@ti-chi-bot ti-chi-bot bot mentioned this pull request Jul 29, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewstevenson88 matthewstevenson88 matthewstevenson88 approved these changes

Assignees

No one assigned

Labels

area/security bloat/low imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gtcooke94 @matthewstevenson88