Skip to content

Conversation

hallfox
Copy link
Collaborator

@hallfox hallfox commented Sep 17, 2025

This is mostly addressing the outstanding issues in #829. There are a handful of changes:

  • Deleted tests that were intentionally invoking UB by casting an out of range int to an enum
  • Fixed the status code handling in queue FSM mode to appropriately convert out of range error codes into the UKNOWN category
  • Fixed other test cases with UB

@hallfox hallfox marked this pull request as ready for review September 17, 2025 17:57
@hallfox hallfox requested a review from a team as a code owner September 17, 2025 17:57
@hallfox hallfox force-pushed the ubsan branch 6 times, most recently from c963b40 to 161e5a4 Compare September 17, 2025 18:44
@678098
Copy link
Collaborator

678098 commented Sep 17, 2025

=========================== short test summary info ============================
FAILED test_cluster_node_shutdown.py::TestClusterNodeShutdown::test_primary_shutdown_with_proxy[multi_node-eventual_consistency] - assert 0 == 1
 +  where 0 = len([])
======== 1 failed, 171 passed, 1 skipped, 5 rerun in 1510.55s (0:25:10) ========

I think I've seen this error in another PR, might be a problem with IT itself

@hallfox hallfox force-pushed the ubsan branch 4 times, most recently from 0fa87a1 to 92f5486 Compare September 17, 2025 20:26
Comment on lines 231 to 262
switch (res) {
case bmqt::GenericResult::Enum::e_SUCCESS: {
return bmqp_ctrlmsg::StatusCategory::E_SUCCESS;
} break;
case bmqt::GenericResult::Enum::e_UNKNOWN: {
return bmqp_ctrlmsg::StatusCategory::E_UNKNOWN;
} break;
case bmqt::GenericResult::Enum::e_TIMEOUT: {
return bmqp_ctrlmsg::StatusCategory::E_TIMEOUT;
} break;
case bmqt::GenericResult::Enum::e_NOT_CONNECTED: {
return bmqp_ctrlmsg::StatusCategory::E_NOT_CONNECTED;
} break;
case bmqt::GenericResult::Enum::e_CANCELED: {
return bmqp_ctrlmsg::StatusCategory::E_CANCELED;
} break;
case bmqt::GenericResult::Enum::e_NOT_SUPPORTED: {
return bmqp_ctrlmsg::StatusCategory::E_NOT_SUPPORTED;
} break;
case bmqt::GenericResult::Enum::e_REFUSED: {
return bmqp_ctrlmsg::StatusCategory::E_REFUSED;
} break;
case bmqt::GenericResult::Enum::e_INVALID_ARGUMENT: {
return bmqp_ctrlmsg::StatusCategory::E_INVALID_ARGUMENT;
} break;
case bmqt::GenericResult::Enum::e_NOT_READY: {
return bmqp_ctrlmsg::StatusCategory::E_NOT_READY;
} break;
default: {
return bmqp_ctrlmsg::StatusCategory::E_UNKNOWN;
} break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
switch (res) {
case bmqt::GenericResult::Enum::e_SUCCESS: {
return bmqp_ctrlmsg::StatusCategory::E_SUCCESS;
} break;
case bmqt::GenericResult::Enum::e_UNKNOWN: {
return bmqp_ctrlmsg::StatusCategory::E_UNKNOWN;
} break;
case bmqt::GenericResult::Enum::e_TIMEOUT: {
return bmqp_ctrlmsg::StatusCategory::E_TIMEOUT;
} break;
case bmqt::GenericResult::Enum::e_NOT_CONNECTED: {
return bmqp_ctrlmsg::StatusCategory::E_NOT_CONNECTED;
} break;
case bmqt::GenericResult::Enum::e_CANCELED: {
return bmqp_ctrlmsg::StatusCategory::E_CANCELED;
} break;
case bmqt::GenericResult::Enum::e_NOT_SUPPORTED: {
return bmqp_ctrlmsg::StatusCategory::E_NOT_SUPPORTED;
} break;
case bmqt::GenericResult::Enum::e_REFUSED: {
return bmqp_ctrlmsg::StatusCategory::E_REFUSED;
} break;
case bmqt::GenericResult::Enum::e_INVALID_ARGUMENT: {
return bmqp_ctrlmsg::StatusCategory::E_INVALID_ARGUMENT;
} break;
case bmqt::GenericResult::Enum::e_NOT_READY: {
return bmqp_ctrlmsg::StatusCategory::E_NOT_READY;
} break;
default: {
return bmqp_ctrlmsg::StatusCategory::E_UNKNOWN;
} break;
}
#define BMQIMP_CASE(CODE) \
case bmqt::GenericResult::Enum::CODE: { \
return bmqp_ctrlmsg::StatusCategory::CODE; \
} break;
switch (res) {
BMQIMP_CASE(e_SUCCESS);
BMQIMP_CASE(e_UNKNOWN);
BMQIMP_CASE(e_TIMEOUT);
BMQIMP_CASE(e_NOT_CONNECTED);
BMQIMP_CASE(e_CANCELED);
BMQIMP_CASE(e_NOT_SUPPORTED);
BMQIMP_CASE(e_REFUSED);
BMQIMP_CASE(e_INVALID_ARGUMENT);
BMQIMP_CASE(e_NOT_READY);
default: {
return bmqp_ctrlmsg::StatusCategory::E_UNKNOWN;
} break;
}
#undef BMQIMP_CASE

Possible alternative. I guess the original variant is easier for search though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would be alright, but there is a slight issue in that one of these enums are named with e_VALUE, and the other is named E_VALUE

@hallfox hallfox force-pushed the ubsan branch 2 times, most recently from 389d6f6 to f20277d Compare September 19, 2025 18:07
- Deleted tests that were intentionally invoking UB by casting an out of range int to an enum
- Fixed the status code handling in queue FSM mode to appropriately convert out of range error codes into the UKNOWN category
- Fixed other test cases with UB

Signed-off-by: Aleksandr Ivanov <[email protected]>
Signed-off-by: Taylor Foxhall <[email protected]>
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