Skip to content

[Clean-up] Fix MSAN and UBSAN issues found by clang-19#36707

Closed
veblush wants to merge 5 commits intogrpc:masterfrom
veblush:fix-xsan
Closed

[Clean-up] Fix MSAN and UBSAN issues found by clang-19#36707
veblush wants to merge 5 commits intogrpc:masterfrom
veblush:fix-xsan

Conversation

@veblush
Copy link
Copy Markdown
Contributor

@veblush veblush commented May 23, 2024

Fixed various MSAN and UBSAN issues found in an attempt to bump the clang version used for RBE. (#36685) As our xSAN tests are using RBE, it revealed a few new issues. This PR is to fix all of those.

@veblush veblush added the release notes: no Indicates if PR should not be in release notes label May 23, 2024
@veblush veblush requested a review from yashykt May 23, 2024 20:26
//

static void fork_fd_list_remove_node(grpc_fork_fd_list* node) {
gpr_mu_lock(&fork_fd_list_mu);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this seems like a change in logic

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not really. fork_fd_list_remove_grpc_fd is a new replacement for the old fork_fd_list_remove_node and all call sites for fork_fd_list_remove_node are updated to use fork_fd_list_remove_grpc_fd

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@drfloob / @eugeneo should review this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe

if (track_fds_for_fork) {

might be needed so there's no overhead if fork support is not necessary

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

^ fork_fd_list_remove_grpc_fd does. No one calls fork_fd_list_remove_node other than fork_fd_list_remove_grpc_fd

paulosjca pushed a commit to paulosjca/grpc that referenced this pull request May 23, 2024
Fixed various MSAN and UBSAN issues found in an attempt to bump the clang version used for RBE. (grpc#36685) As our xSAN tests are using RBE, it revealed a few new issues. This PR is to fix all of those.

Closes grpc#36707

COPYBARA_INTEGRATE_REVIEW=grpc#36707 from veblush:fix-xsan ebbebc2
PiperOrigin-RevId: 636685138
jdcormie pushed a commit to jdcormie/grpc that referenced this pull request May 24, 2024
Fixed various MSAN and UBSAN issues found in an attempt to bump the clang version used for RBE. (grpc#36685) As our xSAN tests are using RBE, it revealed a few new issues. This PR is to fix all of those.

Closes grpc#36707

COPYBARA_INTEGRATE_REVIEW=grpc#36707 from veblush:fix-xsan ebbebc2
PiperOrigin-RevId: 636685138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants