Skip to content

Commit 40b23c3

Browse files
alexander-e1offhallfox
alexander-e1off
authored and
hallfox
committed
Fix UBSAN errors in unit tests
- 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]>
1 parent 3f51a52 commit 40b23c3

23 files changed

+200
-329
lines changed

‎docker/sanitizers/sanitizers.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,8 @@
4040
"UBSAN_OPTIONS": {
4141
"external_symbolizer_path": "/usr/bin/llvm-symbolizer",
4242
"suppressions": "%%SRC%%/etc/ubsansup.txt",
43-
"print_stacktrace": "1"
43+
"print_stacktrace": "1",
44+
"halt_on_error": 1
4445
}
4546
}
4647
}

‎src/groups/bmq/bmqimp/bmqimp_brokersession.cpp‎

Lines changed: 64 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,44 @@ void makeDeconfigure(bmqp_ctrlmsg::ControlMessage* request)
224224
}
225225
}
226226

227+
/// @brief This function translates other result code types into the
228+
/// less-specific StatusCategory error code.
229+
bmqp_ctrlmsg::StatusCategory::Value intoStatusCategory(int res)
230+
{
231+
switch (res) {
232+
case bmqt::GenericResult::e_SUCCESS: {
233+
return bmqp_ctrlmsg::StatusCategory::E_SUCCESS;
234+
} break;
235+
case bmqt::GenericResult::e_UNKNOWN: {
236+
return bmqp_ctrlmsg::StatusCategory::E_UNKNOWN;
237+
} break;
238+
case bmqt::GenericResult::e_TIMEOUT: {
239+
return bmqp_ctrlmsg::StatusCategory::E_TIMEOUT;
240+
} break;
241+
case bmqt::GenericResult::e_NOT_CONNECTED: {
242+
return bmqp_ctrlmsg::StatusCategory::E_NOT_CONNECTED;
243+
} break;
244+
case bmqt::GenericResult::e_CANCELED: {
245+
return bmqp_ctrlmsg::StatusCategory::E_CANCELED;
246+
} break;
247+
case bmqt::GenericResult::e_NOT_SUPPORTED: {
248+
return bmqp_ctrlmsg::StatusCategory::E_NOT_SUPPORTED;
249+
} break;
250+
case bmqt::GenericResult::e_REFUSED: {
251+
return bmqp_ctrlmsg::StatusCategory::E_REFUSED;
252+
} break;
253+
case bmqt::GenericResult::e_INVALID_ARGUMENT: {
254+
return bmqp_ctrlmsg::StatusCategory::E_INVALID_ARGUMENT;
255+
} break;
256+
case bmqt::GenericResult::e_NOT_READY: {
257+
return bmqp_ctrlmsg::StatusCategory::E_NOT_READY;
258+
} break;
259+
default: {
260+
return bmqp_ctrlmsg::StatusCategory::E_UNKNOWN;
261+
} break;
262+
}
263+
}
264+
227265
} // close unnamed namespace
228266

229267
// ------------
@@ -1013,7 +1051,7 @@ void BrokerSession::QueueFsm::setQueueId(
10131051

10141052
void BrokerSession::QueueFsm::injectErrorResponse(
10151053
const RequestManagerType::RequestSp& context,
1016-
bmqp_ctrlmsg::StatusCategory::Value status,
1054+
int status,
10171055
const bslstl::StringRef& reason)
10181056
{
10191057
// executed by the FSM thread
@@ -1022,7 +1060,7 @@ void BrokerSession::QueueFsm::injectErrorResponse(
10221060

10231061
// Inject an error response which will be handled outside the Queue FSM
10241062
bmqp::ControlMessageUtil::makeStatus(&context->response(),
1025-
status,
1063+
intoStatusCategory(status),
10261064
status,
10271065
reason);
10281066
}
@@ -1526,10 +1564,7 @@ bmqt::OpenQueueResult::Enum BrokerSession::QueueFsm::handleOpenRequest(
15261564
context,
15271565
timeout);
15281566
if (rc != bmqt::OpenQueueResult::e_SUCCESS) {
1529-
handleRequestNotSent(
1530-
queue,
1531-
context,
1532-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc));
1567+
handleRequestNotSent(queue, context, rc);
15331568
}
15341569

15351570
// Return SUCCESS result. Any error will be reported through the
@@ -1564,7 +1599,7 @@ bmqt::OpenQueueResult::Enum BrokerSession::QueueFsm::handleOpenRequest(
15641599
void BrokerSession::QueueFsm::handleRequestNotSent(
15651600
const bsl::shared_ptr<Queue>& queue,
15661601
const RequestManagerType::RequestSp& context,
1567-
bmqp_ctrlmsg::StatusCategory::Value status)
1602+
int status)
15681603
{
15691604
// executed by the FSM thread
15701605

@@ -1769,10 +1804,7 @@ void BrokerSession::QueueFsm::handleReopenRequest(
17691804
context,
17701805
timeout);
17711806
if (res != bmqt::OpenQueueResult::e_SUCCESS) {
1772-
handleRequestNotSent(
1773-
queue,
1774-
context,
1775-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(res));
1807+
handleRequestNotSent(queue, context, res);
17761808
}
17771809
} break;
17781810
case QueueState::e_OPENED:
@@ -1930,10 +1962,7 @@ bmqt::CloseQueueResult::Enum BrokerSession::QueueFsm::handleCloseRequest(
19301962
context,
19311963
timeout);
19321964
if (rc != bmqt::ConfigureQueueResult::e_SUCCESS) {
1933-
handleRequestNotSent(
1934-
queue,
1935-
context,
1936-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc));
1965+
handleRequestNotSent(queue, context, rc);
19371966
}
19381967
} break;
19391968
case QueueState::e_PENDING: {
@@ -2134,10 +2163,7 @@ void BrokerSession::QueueFsm::handleResponseError(
21342163
absTimeout);
21352164
if (rc != bmqt::GenericResult::e_SUCCESS) {
21362165
// Failed to send close request.
2137-
handleRequestNotSent(
2138-
queue,
2139-
context,
2140-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc));
2166+
handleRequestNotSent(queue, context, rc);
21412167
}
21422168
} break;
21432169
case QueueState::e_OPENING_OPN_EXPIRED:
@@ -2650,10 +2676,7 @@ void BrokerSession::QueueFsm::handleResponseOk(
26502676
timeout,
26512677
false); // isReopenRequest
26522678
if (rc != bmqt::ConfigureQueueResult::e_SUCCESS) {
2653-
handleRequestNotSent(
2654-
queue,
2655-
context,
2656-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc));
2679+
handleRequestNotSent(queue, context, rc);
26572680
}
26582681
} break;
26592682
case QueueState::e_REOPENING_OPN: {
@@ -2691,10 +2714,7 @@ void BrokerSession::QueueFsm::handleResponseOk(
26912714
timeout,
26922715
true); // isReopenRequest
26932716
if (rc != bmqt::ConfigureQueueResult::e_SUCCESS) {
2694-
handleRequestNotSent(
2695-
queue,
2696-
context,
2697-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc));
2717+
handleRequestNotSent(queue, context, rc);
26982718
}
26992719
} break;
27002720
case QueueState::e_OPENING_CFG: {
@@ -2746,10 +2766,7 @@ void BrokerSession::QueueFsm::handleResponseOk(
27462766
timeout);
27472767
if (rc != bmqt::GenericResult::e_SUCCESS) {
27482768
// Failed to send close request.
2749-
handleRequestNotSent(
2750-
queue,
2751-
context,
2752-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc));
2769+
handleRequestNotSent(queue, context, rc);
27532770
}
27542771
} break;
27552772
case QueueState::e_CLOSING_CLS: {
@@ -2837,10 +2854,7 @@ void BrokerSession::QueueFsm::handleLateResponse(
28372854
bmqt::ConfigureQueueResult::Enum rc = actionDeconfigureExpiredQueue(
28382855
queue);
28392856
if (rc != bmqt::ConfigureQueueResult::e_SUCCESS) {
2840-
handleRequestNotSent(
2841-
queue,
2842-
context,
2843-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc));
2857+
handleRequestNotSent(queue, context, rc);
28442858
}
28452859
} break;
28462860
case QueueState::e_CLOSING_CFG_EXPIRED: {
@@ -2881,10 +2895,7 @@ void BrokerSession::QueueFsm::handleLateResponse(
28812895
bmqt::ConfigureQueueResult::Enum rc =
28822896
actionReconfigureQueue(queue, context->response());
28832897
if (rc != bmqt::ConfigureQueueResult::e_SUCCESS) {
2884-
handleRequestNotSent(
2885-
queue,
2886-
context,
2887-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc));
2898+
handleRequestNotSent(queue, context, rc);
28882899
}
28892900
} break;
28902901
case QueueState::e_CLOSING_CFG:
@@ -3274,7 +3285,7 @@ void BrokerSession::asyncRequestNotifier(
32743285
eventCallback);
32753286
}
32763287
else if (context->isError()) {
3277-
bmqt::GenericResult::Enum result = context->result();
3288+
int result = context->code();
32783289

32793290
enqueueSessionEvent(eventType,
32803291
result,
@@ -3303,13 +3314,13 @@ void BrokerSession::syncRequestNotifier(
33033314
BSLS_ASSERT_SAFE(semaphore);
33043315
BSLS_ASSERT_SAFE(status);
33053316

3306-
bmqt::GenericResult::Enum result = bmqt::GenericResult::e_SUCCESS;
3317+
int result = bmqt::GenericResult::e_SUCCESS;
33073318

33083319
if (!context) {
33093320
result = bmqt::GenericResult::e_REFUSED;
33103321
}
33113322
else if (context->isError()) {
3312-
result = context->result();
3323+
result = context->code();
33133324
}
33143325
else {
33153326
result = bmqt::GenericResult::e_SUCCESS;
@@ -4448,7 +4459,7 @@ void BrokerSession::onResumeQueueConfigured(
44484459

44494460
if (queueSp->isOpened() &&
44504461
context->result() != bmqt::GenericResult::e_CANCELED) {
4451-
bmqt::GenericResult::Enum status = context->result();
4462+
int status = context->code();
44524463

44534464
// We issue two e_QUEUE_RESUMED events in succession.
44544465
// 1. An event with callback attached, which allows PUTs to queue.
@@ -5047,10 +5058,9 @@ void BrokerSession::doOpenQueue(
50475058
// Failure - we still want to notify the user, so we need to "manually"
50485059
// enqueue an event on the event queue.
50495060
// Inject an error response which will be handled outside the Queue FSM
5050-
d_queueFsm.injectErrorResponse(
5051-
context,
5052-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc),
5053-
bmqt::OpenQueueResult::toAscii(rc));
5061+
d_queueFsm.injectErrorResponse(context,
5062+
rc,
5063+
bmqt::OpenQueueResult::toAscii(rc));
50545064

50555065
context->signal();
50565066
}
@@ -5095,10 +5105,9 @@ void BrokerSession::doConfigureQueue(
50955105

50965106
// Failure - we still want to notify the user, so we need to "manually"
50975107
// enqueue an event on the event queue.
5098-
d_queueFsm.injectErrorResponse(
5099-
context,
5100-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(rc),
5101-
bmqt::ConfigureQueueResult::toAscii(rc));
5108+
d_queueFsm.injectErrorResponse(context,
5109+
rc,
5110+
bmqt::ConfigureQueueResult::toAscii(rc));
51025111

51035112
context->signal();
51045113
}
@@ -5132,10 +5141,9 @@ void BrokerSession::doCloseQueue(
51325141
if (res != bmqt::CloseQueueResult::e_SUCCESS) {
51335142
// Failure - we still want to notify the user, so we need to "manually"
51345143
// enqueue an event on the event queue.
5135-
d_queueFsm.injectErrorResponse(
5136-
closeContext,
5137-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(res),
5138-
bmqt::CloseQueueResult::toAscii(res));
5144+
d_queueFsm.injectErrorResponse(closeContext,
5145+
res,
5146+
bmqt::CloseQueueResult::toAscii(res));
51395147
closeContext->signal();
51405148
}
51415149
}
@@ -6471,7 +6479,7 @@ void BrokerSession::onCloseQueueConfigured(
64716479
// Set user response
64726480
d_queueFsm.injectErrorResponse(
64736481
closeQueueContext,
6474-
static_cast<bmqp_ctrlmsg::StatusCategory::Value>(result),
6482+
context->code(),
64756483
"The request was canceled [reason: connection was lost]");
64766484
}
64776485
else if (configureQueueContext->isLocalTimeout()) {

‎src/groups/bmq/bmqimp/bmqimp_brokersession.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -652,7 +652,7 @@ class BrokerSession BSLS_CPP11_FINAL {
652652
/// due to some error described with the specified `status`.
653653
void handleRequestNotSent(const bsl::shared_ptr<Queue>& queue,
654654
const RequestManagerType::RequestSp& context,
655-
bmqp_ctrlmsg::StatusCategory::Value status);
655+
int status);
656656

657657
/// Handle response error.
658658
void handleResponseError(const bsl::shared_ptr<Queue>& queue,
@@ -697,9 +697,9 @@ class BrokerSession BSLS_CPP11_FINAL {
697697
void handleQueueResume(const bsl::shared_ptr<Queue>& queue);
698698

699699
/// Create a status response for the specified `context` using the
700-
/// specified `status` code and error `reason` description.
700+
/// specified `status` and error `reason` description.
701701
void injectErrorResponse(const RequestManagerType::RequestSp& context,
702-
bmqp_ctrlmsg::StatusCategory::Value status,
702+
int status,
703703
const bslstl::StringRef& reason = "");
704704
};
705705

‎src/groups/bmq/bmqimp/bmqimp_brokersession.t.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3823,8 +3823,10 @@ static void queueOpenCloseAsync(bsls::Types::Uint64 queueFlags)
38233823
BMQTST_ASSERT_EQ(pQueue->state(), bmqimp::QueueState::e_CLOSED);
38243824
BMQTST_ASSERT_EQ(pQueue->isValid(), false);
38253825

3826+
int rc;
3827+
38263828
PVV_SAFE("Close unopened queue async");
3827-
int rc = obj.session().closeQueueAsync(pQueue, timeout);
3829+
rc = obj.session().closeQueueAsync(pQueue, timeout);
38283830

38293831
// Verify the result
38303832
BMQTST_ASSERT_EQ(rc, bmqt::CloseQueueResult::e_SUCCESS);

‎src/groups/bmq/bmqimp/bmqimp_event.t.cpp‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -855,9 +855,6 @@ static void test7_printing()
855855
// matches the expected value.
856856
// 2. Check that 'bmqimp::Event' object directed to output stream with
857857
// error doesn't make impact on stream (output is empty).
858-
// 3. Check that 'bmqimp::Event' object with incorrect event type
859-
// directed to output stream causes 'bsls::AssertTestException'
860-
// exception.
861858
//
862859
// Testing:
863860
// bmqimp::Event::print()
@@ -1075,17 +1072,8 @@ static void test7_printing()
10751072
out.clear();
10761073
out.reset();
10771074
}
1078-
1079-
{
1080-
PV("Bad enum value test");
1081-
bmqimp::Event obj(&bufferFactory, bmqtst::TestHelperUtil::allocator());
1082-
obj.setType(static_cast<bmqimp::Event::EventType::Enum>(
1083-
bsl::numeric_limits<int>::min()));
1084-
1085-
BMQTST_ASSERT_OPT_FAIL(out << obj);
1086-
out.reset();
1087-
}
10881075
}
1076+
10891077
static void test8_putEventBuilder()
10901078
{
10911079
// ------------------------------------------------------------------------

0 commit comments

Comments
 (0)