fix code issues in object manager that are reported by scanning tool #3649
Conversation
|
Test FAILed. |
| switch (message_type) { | ||
| case static_cast<int64_t>(object_manager_protocol::MessageType::PushRequest): { | ||
| if (static_cast<protocol::MessageType>(message_type) == | ||
| protocol::MessageType::DisconnectClient) { |
There was a problem hiding this comment.
It seems problematic that message_type can have type either protocol::MessageType or object_manager_protocol::MessageType because those two enums could have overlapping values. For example, couldn't protocol::MessageType::DisconnectClient be the same integer value as some value in object_manager_protocol::MessageType?
There was a problem hiding this comment.
That's right. Currently object_manager_protocol::MessageType doesn't have the same value as protocol::MessageType::DisconnectClient. But we may accidentally introduce bugs in the future when we add new values to object_manager_protocol::MessageType.
I don't know what's the reason why this message_type uses mixed types. Can we also add a DisconnectClient to object_manager_protocol::MessageType? @zhijunfu
If that cannot be easily fixed, we should at least add a warning comment in object_manager.fbs
There was a problem hiding this comment.
Yes this is indeed a problem, although it's not brought in by this PR.
How about we support both protocol::MessageType::DisconnectClient and object_manager_protocol::MessageType::DisconnectClient? Then two different sets of MessageType enums can be totally decoupled.
There was a problem hiding this comment.
yeah, that would be nicer.
|
Looks like there are some linting errors. |
| ReceiveFreeRequest(conn, message); | ||
| break; | ||
| } | ||
| case static_cast<int64_t>(protocol::MessageType::DisconnectClient): { | ||
| case object_manager_protocol::MessageType::DisconnectClient: { |
There was a problem hiding this comment.
the below TDO can be removed.
|
Test PASSed. |
|
Test FAILed. |
|
Test FAILed. |
Fix some code issues found by code scanning tool:
1. Macro compares unsigned to 0(NO_EFFECT)
CWE570: An unsigned value can never be less than 0
This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "this->create_buffer_state_[object_id].num_seals_remaining >= 0UL".
~/ray/src/ray/object_manager/object_buffer_pool.cc: ray::ObjectBufferPool::SealChunk(const ray::UniqueID &, unsigned long)
2. Inferred misuse of enum(MIXED_ENUMS)
CWE398: An integer expression which was inferred to have an enum type is mixed with a different enum type
This case, "static_cast(ray::object_manager::protocol::MessageType::PushRequest)", implies the effective type of "message_type" is "ray::object_manager::protocol::MessageType".
~/ray/src/ray/object_manager/object_manager.cc: ray::ObjectManager::ProcessClientMessage(std::shared_ptr> &, long, const unsigned char *)