fix: guard against empty alive_vec in GCS liveness check - #61870
Conversation
Root cause: GCS liveness check callback accessed alive_vec[0] without checking if the vector was empty, causing SIGSEGV when the RPC failed and returned an empty vector. Made-with: Cursor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a potential segfault in the GCS liveness check by adding guards against an empty alive_vec. The logic to first check the RPC status and then the vector's emptiness before access is sound. I've added one suggestion to combine the two guard clauses to make the code a bit more concise and reduce duplication. Overall, this is a good fix.
| if (!status.ok()) { | ||
| if (status.IsUnauthenticated()) { | ||
| RAY_LOG(FATAL) | ||
| << "GCS returned an authentication error. This may happen when " | ||
| << "GCS is not backed by a DB and restarted or there is data loss " | ||
| << "in the DB. Local cluster ID: " << gcs_client_.GetClusterId(); | ||
| } | ||
| *checking_ptr = false; | ||
| return; | ||
| } | ||
| if (alive_vec.empty()) { | ||
| *checking_ptr = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
The two separate if statements for checking !status.ok() and alive_vec.empty() can be combined into a single guard clause. This makes the code more concise and reduces the duplication of *checking_ptr = false; return;.
| if (!status.ok()) { | |
| if (status.IsUnauthenticated()) { | |
| RAY_LOG(FATAL) | |
| << "GCS returned an authentication error. This may happen when " | |
| << "GCS is not backed by a DB and restarted or there is data loss " | |
| << "in the DB. Local cluster ID: " << gcs_client_.GetClusterId(); | |
| } | |
| *checking_ptr = false; | |
| return; | |
| } | |
| if (alive_vec.empty()) { | |
| *checking_ptr = false; | |
| return; | |
| } | |
| if (!status.ok() || alive_vec.empty()) { | |
| if (!status.ok() && status.IsUnauthenticated()) { | |
| RAY_LOG(FATAL) | |
| << "GCS returned an authentication error. This may happen when " | |
| << "GCS is not backed by a DB and restarted or there is data loss " | |
| << "in the DB. Local cluster ID: " << gcs_client_.GetClusterId(); | |
| } | |
| *checking_ptr = false; | |
| return; | |
| } |
themavik
left a comment
themavik
left a comment
There was a problem hiding this comment.
Reviewed the changes — the implementation is clean and addresses the reported issue correctly.
themavik
left a comment
themavik
left a comment
There was a problem hiding this comment.
Reviewed the changes — the fix is minimal and targeted. Good contribution.
ray-gardener
Bot
added
core
1 participant
Summary
Fixes #61787.
Root cause: The GCS liveness check callback accessed
alive_vec[0]without checking if the vector was empty. When the RPC failed and returned an empty vector, this caused a SIGSEGV (out-of-bounds access).Fix: Added an empty check for
alive_vecbefore accessing its elements, and handle non-OK status before touching the vector.Changes
alive_vec.empty()guard in the GCS liveness check callback!status.ok()first to avoid accessing alive_vec on RPC failureTesting
Made with Cursor