Fix[bmqp::RequestManager]: handle int32 signed overflow - #1715
Conversation
678098
commented
Aug 14, 2026
678098
commented
- Wrap to 1 on reaching int32::max() when selecting the next requestId
- Attempt to find the next unused requestId in a map
- Wrap to 1 on reaching int32::max() when selecting the next requestId - Attempt to find the next unused requestId in a map Signed-off-by: Evgeny Malygin <emalygin@bloomberg.net>
| d_nextRequestId = (d_nextRequestId == k_LAST_REQUEST_ID) | ||
| ? k_FIRST_REQUEST_ID | ||
| : d_nextRequestId + 1; | ||
| } while (d_requests.find(d_nextRequestId) != d_requests.end()); |
There was a problem hiding this comment.
This lookup should be performed just once every time we call this function, before we reach 2.147kk requests.
If we reached it, we might theoretically have multiple iterations of this loop.
What do you think? Is it worth attempting to find the next unused rId?
| // Insert the request in the map | ||
| BSLA_MAYBE_UNUSED bsl::pair<RequestMapIter, bool> insertRC = | ||
| d_requests.insert(bsl::make_pair(requestId, request)); | ||
| BSLS_ASSERT_SAFE(insertRC.second); |
There was a problem hiding this comment.
This code might have failed on int32::max wrap without an attempt to find next; with this PR, we are guaranteed to find the next unused.
Also, changing this to ASSERT_OPT
| : d_nextRequestId + 1; | ||
| } while (d_requests.find(d_nextRequestId) != d_requests.end()); | ||
|
|
||
| return d_nextRequestId; |
There was a problem hiding this comment.
This code is also difficult to test.
UT that calls bmqp::RequestManager::sendRequest 2kkk times might take hours to execute. The only way to write a good test is to expose bmqp::RequestManager internal state to fast-forward d_nextRequestId
2 participants
