From ebf2a1ae8fc66fc908f5e700f2c8ba431e7a2a48 Mon Sep 17 00:00:00 2001 From: Roman Kiryanov Date: Thu, 23 Jul 2026 01:40:34 +0000 Subject: [PATCH 1/5] modem_simulator: prevent unhandled exceptions for strings shorter than 2 std::string::compare > The overloads taking parameters named pos1 or pos2 > throws std::out_of_range if the argument is out of range. Bug: 509614113 Signed-off-by: Roman Kiryanov --- .../host/commands/modem_simulator/modem_service.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/modem_service.cpp b/base/cvd/cuttlefish/host/commands/modem_simulator/modem_service.cpp index 54151593be2..e5806c32371 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/modem_service.cpp +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/modem_service.cpp @@ -34,14 +34,13 @@ CommandHandler::CommandHandler(const std::string& command, p_func handler) p_command_handler(handler) {} int CommandHandler::Compare(const std::string& command) const { - int result = -1; - if (match_mode == PARTIAL_MATCH) { - result = - command.compare(2, command_prefix.size(), command_prefix); // skip "AT" - } else { - result = command.compare(2, command.size(), command_prefix); + if (command.size() < 2) { // the "AT" prefix + return -1; } - return result; + + return (match_mode == PARTIAL_MATCH) + ? command.compare(2, command_prefix.size(), command_prefix) + : command.compare(2, command.size(), command_prefix); } void CommandHandler::HandleCommand(const Client& client, From 35a1c4c01d5e5a77a8d1bebe944f63dc81d18dbf Mon Sep 17 00:00:00 2001 From: Roman Kiryanov Date: Thu, 23 Jul 2026 17:20:45 +0000 Subject: [PATCH 2/5] modem_simulator: prevent unhandled exceptions for incorrect input std::stoi: > std::invalid_argument if no conversion could be performed. > std::out_of_range if the converted value would fall out of the range Bug: 509614113 Signed-off-by: Roman Kiryanov --- .../cuttlefish/host/commands/modem_simulator/call_service.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.cpp b/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.cpp index 554878d4ff0..248ca45b5c6 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.cpp +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.cpp @@ -400,8 +400,8 @@ void CallService::HandleHangup(const Client& client, CommandParser cmd(command); cmd.SkipPrefix(); - std::string action(*cmd); - int n = std::stoi(action.substr(0, 1)); + const std::string action(*cmd); + const int n = action.empty() ? -1 : (action[0] - '0'); int index = -1; if (cmd->length() > 1) { index = std::stoi(action.substr(1)); From cc2c3d231f01840c7e8d1dd9fd7148cb94fca066 Mon Sep 17 00:00:00 2001 From: Roman Kiryanov Date: Thu, 23 Jul 2026 17:28:59 +0000 Subject: [PATCH 3/5] modem_simulator: fix the possible OOB if the buffer is not null terminated If the `Read` call fills the whole buffer, `commands.append(buffer.data())` with no zero characters will read past the buffer. If the `Read` produces data with zero characters in the middle, it will cause a loss of data. Bug: 509614113 Signed-off-by: Roman Kiryanov --- .../commands/modem_simulator/channel_monitor.cpp | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.cpp b/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.cpp index 959060e7581..58a5f368c2f 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.cpp +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.cpp @@ -83,8 +83,8 @@ void ChannelMonitor::AcceptIncomingConnection() { } void ChannelMonitor::ReadCommand(Client& client) { - std::vector buffer(kMaxCommandLength); - auto bytes_read = client.client_read_fd_->Read(buffer.data(), buffer.size()); + char buffer[kMaxCommandLength]; + auto bytes_read = client.client_read_fd_->Read(buffer, kMaxCommandLength); if (bytes_read <= 0) { if (errno == EAGAIN && client.type == Client::REMOTE && client.first_read_command_) { @@ -107,13 +107,9 @@ void ChannelMonitor::ReadCommand(Client& client) { return; } - std::string& incomplete_command = client.incomplete_command; - // Add the incomplete command from the last read - auto commands = std::string{incomplete_command.data()}; - commands.append(buffer.data()); - - incomplete_command.clear(); + std::string commands(std::move(client.incomplete_command)); + commands.append(buffer, bytes_read); // Replacing '\n' with '\r' absl::StrReplaceAll({{"\n", "\r"}}, &commands); @@ -134,8 +130,8 @@ void ChannelMonitor::ReadCommand(Client& client) { } pos = r_pos + 1; // Skip '\r' } else if (pos < commands.length()) { // Incomplete command - incomplete_command = commands.substr(pos); - VLOG(1) << "incomplete command: " << incomplete_command; + client.incomplete_command = commands.substr(pos); + VLOG(1) << "incomplete command: " << client.incomplete_command; } } } From 5a19bfababcd8175643f2849f02cd32356faa625 Mon Sep 17 00:00:00 2001 From: Roman Kiryanov Date: Thu, 23 Jul 2026 19:29:54 +0000 Subject: [PATCH 4/5] modem_simulator: avoid referring to a dead object The `sms_pdu` is used in an async callback, potentially after the control leaves the scope. Bug: 509614113 Signed-off-by: Roman Kiryanov --- .../cuttlefish/host/commands/modem_simulator/sms_service.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/sms_service.cpp b/base/cvd/cuttlefish/host/commands/modem_simulator/sms_service.cpp index 166868c66ca..5068ca18880 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/sms_service.cpp +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/sms_service.cpp @@ -297,7 +297,7 @@ void SmsService::HandleSendSMSPDU(const Client& client, std::string& command) { thread_looper_->Post( makeSafeCallback( this, - [&sms_pdu](SmsService* me) { me->HandleReceiveSMS(sms_pdu); }), + [sms_pdu](SmsService* me) { me->HandleReceiveSMS(sms_pdu); }), std::chrono::seconds(1)); } else { // Send SMS to remote host port SendSmsToRemote(remote_host_port, sms_pdu); From 34bb3fdf0df0a6b0a3823c3b3a4580dd12c79fbd Mon Sep 17 00:00:00 2001 From: Roman Kiryanov Date: Thu, 23 Jul 2026 19:32:10 +0000 Subject: [PATCH 5/5] modem_simulator: fix simulateneous access to collections * The code used to erase elements from the `clients_` and `remote_clients_` while iterating over them in `MonitorLoop`. * `SetRemoteClient` is called from two threads (main.cpp and `CallService::HandleDial`), so it needs a mutex. * `CallService::HandleRemoteCall` can also be called from two threads (host-guest and guest-host calls), so `CallService` also needs a mutex. Bug: 509614113 Signed-off-by: Roman Kiryanov --- .../commands/modem_simulator/call_service.cpp | 8 +++ .../commands/modem_simulator/call_service.h | 5 +- .../modem_simulator/channel_monitor.cpp | 65 +++++++++++-------- .../modem_simulator/channel_monitor.h | 3 + .../host/commands/modem_simulator/client.h | 2 +- 5 files changed, 55 insertions(+), 28 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.cpp b/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.cpp index 248ca45b5c6..783c643066c 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.cpp +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.cpp @@ -90,6 +90,7 @@ std::vector CallService::InitializeCommandHandlers() { // This also resumes held calls void CallService::SimulatePendingCallsAnswered() { + std::lock_guard lock(active_calls_mutex_); for (auto& iter : active_calls_) { if (iter.second.isCallDialing()) { iter.second.SetCallActive(); @@ -100,6 +101,7 @@ void CallService::SimulatePendingCallsAnswered() { void CallService::TimerWaitingRemoteCallResponse(CallToken call_token) { VLOG(0) << "Dialing id: " << call_token.first << ", number: " << call_token.second << "timeout, cancel"; + std::lock_guard lock(active_calls_mutex_); auto iter = active_calls_.find(call_token.first); if (iter != active_calls_.end() && iter->second.number == call_token.second) { if (iter->second.remote_client != std::nullopt) { @@ -112,6 +114,7 @@ void CallService::TimerWaitingRemoteCallResponse(CallToken call_token) { /* ATD */ void CallService::HandleDial(const Client& client, const std::string& command) { + std::lock_guard lock(active_calls_mutex_); // Check the network registration state auto registration_state = NetworkService::NET_REGISTRATION_UNKNOWN; if (network_service_) { @@ -265,6 +268,7 @@ void CallService::SendCallStatusToRemote(CallStatus& call, /* ATA */ void CallService::HandleAcceptCall(const Client& client) { + std::lock_guard lock(active_calls_mutex_); for (auto& iter : active_calls_) { if (iter.second.isCallIncoming()) { iter.second.SetCallActive(); @@ -280,6 +284,7 @@ void CallService::HandleAcceptCall(const Client& client) { /* ATH */ void CallService::HandleRejectCall(const Client& client) { + std::lock_guard lock(active_calls_mutex_); for (auto iter = active_calls_.begin(); iter != active_calls_.end();) { /* ATH: hangup, since user is busy */ if (iter->second.isCallIncoming()) { @@ -339,6 +344,7 @@ void CallService::HandleRejectCall(const Client& client) { *see RIL_REQUEST_GET_CURRENT_CALLS in RIL */ void CallService::HandleCurrentCalls(const Client& client) { + std::lock_guard lock(active_calls_mutex_); std::vector responses; std::stringstream ss; @@ -396,6 +402,7 @@ void CallService::HandleCurrentCalls(const Client& client) { */ void CallService::HandleHangup(const Client& client, const std::string& command) { + std::lock_guard lock(active_calls_mutex_); std::vector responses; CommandParser cmd(command); cmd.SkipPrefix(); @@ -643,6 +650,7 @@ void CallService::CallStateUpdate() { */ void CallService::HandleRemoteCall(const Client& client, const std::string& command) { + std::lock_guard lock(active_calls_mutex_); CommandParser cmd(command); cmd.SkipPrefix(); diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.h b/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.h index 5d36918ef20..7e707942568 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.h +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/call_service.h @@ -15,6 +15,8 @@ #pragma once +#include + #include "cuttlefish/host/commands/modem_simulator/modem_service.h" #include "cuttlefish/host/commands/modem_simulator/network_service.h" #include "cuttlefish/host/commands/modem_simulator/sim_service.h" @@ -32,6 +34,7 @@ class CallService : public ModemService, public std::enable_shared_from_this InitializeCommandHandlers(); void SimulatePendingCallsAnswered(); @@ -148,6 +150,7 @@ class CallService : public ModemService, public std::enable_shared_from_this active_calls_; bool in_emergency_mode_; bool mute_on_; diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.cpp b/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.cpp index 58a5f368c2f..f99bb5a74c7 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.cpp +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.cpp @@ -46,16 +46,15 @@ ClientId ChannelMonitor::SetRemoteClient(SharedFD client, bool is_accepted) { if (is_accepted) { // There may be new data from remote client before select. remote_client->first_read_command_ = true; - ReadCommand(*remote_client); } - if (remote_client->client_read_fd_->IsOpen() && - remote_client->client_write_fd_->IsOpen()) { - remote_client->first_read_command_ = false; - remote_clients_.push_back(std::move(remote_client)); - VLOG(0) << "added one remote client"; + { + std::lock_guard lock(clients_mutex_); + pending_remote_.push_back(std::move(remote_client)); } + VLOG(0) << "added one remote client"; + // Trigger monitor loop if (write_pipe_->IsOpen()) { write_pipe_->Write("OK", sizeof("OK")); @@ -96,16 +95,14 @@ void ChannelMonitor::ReadCommand(Client& client) { << client.client_read_fd_->StrError(); client.client_read_fd_->Close(); // Ignore errors here client.client_write_fd_->Close(); - // Erase client from the vector clients - auto& clients = client.type == Client::REMOTE ? remote_clients_ : clients_; - auto iter = std::find_if( - clients.begin(), clients.end(), - [&](std::unique_ptr& other) { return *other == client; }); - if (iter != clients.end()) { - clients.erase(iter); - } + + // Do NOT erase here — we are called from inside MonitorLoop's range-for + // over the same vector. Mark invalid and erase in removeInvalidClients + // outside of the loop. + client.is_valid = false; return; } + client.first_read_command_ = false; // Add the incomplete command from the last read std::string commands(std::move(client.incomplete_command)); @@ -138,6 +135,7 @@ void ChannelMonitor::ReadCommand(Client& client) { void ChannelMonitor::SendUnsolicitedCommand(std::string& response) { // The first accepted client default to be unsolicited command channel? + std::lock_guard lock(clients_mutex_); auto iter = clients_.begin(); if (iter != clients_.end()) { iter->get()->SendCommandResponse(response); @@ -147,6 +145,7 @@ void ChannelMonitor::SendUnsolicitedCommand(std::string& response) { } void ChannelMonitor::SendRemoteCommand(ClientId client, std::string& response) { + std::lock_guard lock(clients_mutex_); auto iter = remote_clients_.begin(); for (; iter != remote_clients_.end(); ++iter) { if (iter->get()->Id() == client) { @@ -158,6 +157,7 @@ void ChannelMonitor::SendRemoteCommand(ClientId client, std::string& response) { } void ChannelMonitor::CloseRemoteConnection(ClientId client) { + std::lock_guard lock(clients_mutex_); auto iter = remote_clients_.begin(); for (; iter != remote_clients_.end(); ++iter) { if (iter->get()->Id() == client) { @@ -207,16 +207,27 @@ void ChannelMonitor::MonitorLoop() { cuttlefish::SharedFDSet read_set; read_set.Set(server_); read_set.Set(read_pipe_); - for (auto& client : clients_) { - if (client->is_valid) { - read_set.Set(client->client_read_fd_); + + { + std::lock_guard lock(clients_mutex_); + remote_clients_.insert( + remote_clients_.end(), + std::make_move_iterator(pending_remote_.begin()), + std::make_move_iterator(pending_remote_.end())); + pending_remote_.clear(); + + for (auto& client : clients_) { + if (client->is_valid) { + read_set.Set(client->client_read_fd_); + } } - } - for (auto& client : remote_clients_) { - if (client->is_valid) { - read_set.Set(client->client_read_fd_); + for (auto& client : remote_clients_) { + if (client->is_valid) { + read_set.Set(client->client_read_fd_); + } } } + int num_fds = cuttlefish::Select(&read_set, nullptr, nullptr, nullptr); if (num_fds < 0) { LOG(ERROR) << "Select call returned error : " << strerror(errno); @@ -233,20 +244,22 @@ void ChannelMonitor::MonitorLoop() { VLOG(0) << "requested to exit now"; break; } - // clean the lists - removeInvalidClients(clients_); - removeInvalidClients(remote_clients_); } + + std::lock_guard lock(clients_mutex_); for (auto& client : clients_) { - if (read_set.IsSet(client->client_read_fd_)) { + if (client->is_valid && read_set.IsSet(client->client_read_fd_)) { ReadCommand(*client); } } for (auto& client : remote_clients_) { - if (read_set.IsSet(client->client_read_fd_)) { + if (client->is_valid && read_set.IsSet(client->client_read_fd_)) { ReadCommand(*client); } } + + removeInvalidClients(clients_); + removeInvalidClients(remote_clients_); } else { // Ignore errors here LOG(ERROR) << "Select call returned error : " << strerror(errno); diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.h b/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.h index b9256a8c182..7ba0eba7276 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.h +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/channel_monitor.h @@ -15,6 +15,7 @@ #pragma once +#include #include #include @@ -53,8 +54,10 @@ class ChannelMonitor { cuttlefish::SharedFD server_; cuttlefish::SharedFD read_pipe_; cuttlefish::SharedFD write_pipe_; + std::mutex clients_mutex_; std::vector> clients_; std::vector> remote_clients_; + std::vector> pending_remote_; void AcceptIncomingConnection(); void OnClientSocketClosed(int sock); diff --git a/base/cvd/cuttlefish/host/commands/modem_simulator/client.h b/base/cvd/cuttlefish/host/commands/modem_simulator/client.h index 0d43c554d7f..024b144680f 100644 --- a/base/cvd/cuttlefish/host/commands/modem_simulator/client.h +++ b/base/cvd/cuttlefish/host/commands/modem_simulator/client.h @@ -78,7 +78,7 @@ class Client { SharedFD client_write_fd_; std::string incomplete_command; mutable std::mutex write_mutex; - bool first_read_command_; // Only used when ClientType::REMOTE + bool first_read_command_ = false; // Only used when ClientType::REMOTE bool is_valid = true; };