CRAS: rclient: Only return errors to cras_client

If rclient gets error from handle_message_from_client, CRAS server will
clean up the rclient and drop all the streams on it.

Therefore, in handle_message_from_client for rclients,
- If the message from clients has incorrect length (truncated message),
  return an error up to CRAS server.
- If the message from clients has invalid content, should return the
  errors to clients by send_message_to_client and return 0.

Then CRAS can keep all valid streams working while rejecting invalid
streams.

BUG=b:155048379
BUG=b:155125014
TEST=Client playback and capture normally on legacy socket.
TEST=Client playback and capture on playback only socket, reject
     capture stream but playback keep working

Change-Id: Id45dbac0d9a2272fb791d56921989f3843d58a03
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/adhd/+/2172840
Reviewed-by: Chih-Yang Hsia <paulhsia@chromium.org>
Tested-by: Chih-Yang Hsia <paulhsia@chromium.org>
Commit-Queue: Chih-Yang Hsia <paulhsia@chromium.org>

6 files changed, 30 insertions, 17 deletions

diff --git a/cras/src/server/cras_control_rclient.c b/cras/src/server/cras_control_rclient.c
index a7c40a1f..98dc28f4 100644
--- a/cras/src/server/cras_control_rclient.c
+++ b/cras/src/server/cras_control_rclient.c
@@ -270,7 +270,14 @@ static int direction_valid(enum CRAS_STREAM_DIRECTION direction)
 }
 
 /* Entry point for handling a message from the client.  Called from the main
- * server context. */
+ * server context.
+ *
+ * If the message from clients has incorrect length (truncated message), return
+ * an error up to CRAS server.
+ * If the message from clients has invalid content, should return the errors to
+ * clients by send_message_to_client and return 0 here.
+ *
+ */
 static int ccr_handle_message_from_client(struct cras_rclient *client,
 					  const struct cras_server_message *msg,
 					  int *fds, unsigned int num_fds)
@@ -292,16 +299,17 @@ static int ccr_handle_message_from_client(struct cras_rclient *client,
 		int client_shm_fd = num_fds > 1 ? fds[1] : -1;
 		struct cras_connect_message cmsg;
 		if (MSG_LEN_VALID(msg, struct cras_connect_message)) {
-			return rclient_handle_client_stream_connect(
+			rclient_handle_client_stream_connect(
 				client,
 				(const struct cras_connect_message *)msg, fd,
 				client_shm_fd);
 		} else if (!convert_connect_message_old(msg, &cmsg)) {
-			return rclient_handle_client_stream_connect(
-				client, &cmsg, fd, client_shm_fd);
+			rclient_handle_client_stream_connect(client, &cmsg, fd,
+							     client_shm_fd);
 		} else {
 			return -EINVAL;
 		}
+		break;
 	}
 	case CRAS_SERVER_DISCONNECT_STREAM:
 		if (!MSG_LEN_VALID(msg, struct cras_disconnect_stream_message))
diff --git a/cras/src/server/cras_rclient_util.c b/cras/src/server/cras_rclient_util.c
index cd0b7f36..da991282 100644
--- a/cras/src/server/cras_rclient_util.c
+++ b/cras/src/server/cras_rclient_util.c
@@ -281,13 +281,13 @@ int rclient_handle_message_from_client(struct cras_rclient *client,
 		int client_shm_fd = num_fds > 1 ? fds[1] : -1;
 		struct cras_connect_message cmsg;
 		if (MSG_LEN_VALID(msg, struct cras_connect_message)) {
-			rc = rclient_handle_client_stream_connect(
+			rclient_handle_client_stream_connect(
 				client,
 				(const struct cras_connect_message *)msg, fd,
 				client_shm_fd);
 		} else if (!convert_connect_message_old(msg, &cmsg)) {
-			rc = rclient_handle_client_stream_connect(
-				client, &cmsg, fd, client_shm_fd);
+			rclient_handle_client_stream_connect(client, &cmsg, fd,
+							     client_shm_fd);
 		} else {
 			return -EINVAL;
 		}
@@ -296,7 +296,7 @@ int rclient_handle_message_from_client(struct cras_rclient *client,
 	case CRAS_SERVER_DISCONNECT_STREAM:
 		if (!MSG_LEN_VALID(msg, struct cras_disconnect_stream_message))
 			return -EINVAL;
-		rc = rclient_handle_client_stream_disconnect(
+		rclient_handle_client_stream_disconnect(
 			client,
 			(const struct cras_disconnect_stream_message *)msg);
 		break;
diff --git a/cras/src/server/cras_rclient_util.h b/cras/src/server/cras_rclient_util.h
index 3a3d499a..e00f87c9 100644
--- a/cras/src/server/cras_rclient_util.h
+++ b/cras/src/server/cras_rclient_util.h
@@ -105,6 +105,11 @@ struct cras_rclient *rclient_generic_create(int fd, size_t id,
 /* Generic handle_message_from_client function for different types of rlicnets.
  * Supports only stream connect and stream disconnect messages.
  *
+ * If the message from clients has incorrect length (truncated message), return
+ * an error up to CRAS server.
+ * If the message from clients has invalid content, should return the errors to
+ * clients by send_message_to_client and return 0 here.
+ *
  * Args:
  *   client - The cras_rclient which gets the message.
  *   msg - The cras_server_message from client.
diff --git a/cras/src/tests/capture_rclient_unittest.cc b/cras/src/tests/capture_rclient_unittest.cc
index 5ca0f9d5..4c1ab07f 100644
--- a/cras/src/tests/capture_rclient_unittest.cc
+++ b/cras/src/tests/capture_rclient_unittest.cc
@@ -139,7 +139,7 @@ TEST_F(CCRMessageSuite, StreamConnectMessageInvalidDirection) {
     fd_ = 100;
     rc = rclient_->ops->handle_message_from_client(rclient_, &msg.header, &fd_,
                                                    1);
-    EXPECT_EQ(-EINVAL, rc);
+    EXPECT_EQ(0, rc);
     EXPECT_EQ(0, cras_make_fd_nonblocking_called);
     EXPECT_EQ(0, stream_list_add_called);
     EXPECT_EQ(0, stream_list_rm_called);
@@ -166,7 +166,7 @@ TEST_F(CCRMessageSuite, StreamConnectMessageInvalidClientId) {
   fd_ = 100;
   rc =
       rclient_->ops->handle_message_from_client(rclient_, &msg.header, &fd_, 1);
-  EXPECT_EQ(-EINVAL, rc);
+  EXPECT_EQ(0, rc);
   EXPECT_EQ(0, cras_make_fd_nonblocking_called);
   EXPECT_EQ(0, stream_list_add_called);
   EXPECT_EQ(0, stream_list_rm_called);
diff --git a/cras/src/tests/control_rclient_unittest.cc b/cras/src/tests/control_rclient_unittest.cc
index eca2580f..e0e34b6f 100644
--- a/cras/src/tests/control_rclient_unittest.cc
+++ b/cras/src/tests/control_rclient_unittest.cc
@@ -186,7 +186,7 @@ TEST_F(RClientMessagesSuite, AudThreadAttachFail) {
   fd_ = 100;
   rc = rclient_->ops->handle_message_from_client(rclient_, &connect_msg_.header,
                                                  &fd_, 1);
-  EXPECT_EQ(-EINVAL, rc);
+  EXPECT_EQ(0, rc);
 
   rc = read(pipe_fds_[0], &out_msg, sizeof(out_msg));
   EXPECT_EQ(sizeof(out_msg), rc);
@@ -203,7 +203,7 @@ TEST_F(RClientMessagesSuite, ConnectMsgWithBadFd) {
 
   rc = rclient_->ops->handle_message_from_client(rclient_, &connect_msg_.header,
                                                  NULL, 0);
-  EXPECT_EQ(-EBADF, rc);
+  EXPECT_EQ(0, rc);
 
   rc = read(pipe_fds_[0], &out_msg, sizeof(out_msg));
   EXPECT_EQ(sizeof(out_msg), rc);
@@ -276,7 +276,7 @@ TEST_F(RClientMessagesSuite, StreamConnectMessageInvalidDirection) {
   fd_ = 100;
   rc = rclient_->ops->handle_message_from_client(rclient_, &connect_msg_.header,
                                                  &fd_, 1);
-  EXPECT_EQ(-EINVAL, rc);
+  EXPECT_EQ(0, rc);
   EXPECT_EQ(0, cras_make_fd_nonblocking_called);
 
   rc = read(pipe_fds_[0], &out_msg, sizeof(out_msg));
@@ -296,7 +296,7 @@ TEST_F(RClientMessagesSuite, StreamConnectMessageInvalidClientId) {
   fd_ = 100;
   rc = rclient_->ops->handle_message_from_client(rclient_, &connect_msg_.header,
                                                  &fd_, 1);
-  EXPECT_EQ(-EINVAL, rc);
+  EXPECT_EQ(0, rc);
   EXPECT_EQ(0, cras_make_fd_nonblocking_called);
   EXPECT_EQ(0, stream_list_add_stream_called);
   EXPECT_EQ(0, stream_list_disconnect_stream_called);
diff --git a/cras/src/tests/playback_rclient_unittest.cc b/cras/src/tests/playback_rclient_unittest.cc
index f135680b..7056d2fe 100644
--- a/cras/src/tests/playback_rclient_unittest.cc
+++ b/cras/src/tests/playback_rclient_unittest.cc
@@ -141,7 +141,7 @@ TEST_F(CPRMessageSuite, StreamConnectMessageInvalidDirection) {
     fd_ = 100;
     rc = rclient_->ops->handle_message_from_client(rclient_, &msg.header, &fd_,
                                                    1);
-    EXPECT_EQ(-EINVAL, rc);
+    EXPECT_EQ(0, rc);
     EXPECT_EQ(0, cras_make_fd_nonblocking_called);
     EXPECT_EQ(0, stream_list_add_called);
     EXPECT_EQ(0, stream_list_rm_called);
@@ -168,7 +168,7 @@ TEST_F(CPRMessageSuite, StreamConnectMessageInvalidClientId) {
   fd_ = 100;
   rc =
       rclient_->ops->handle_message_from_client(rclient_, &msg.header, &fd_, 1);
-  EXPECT_EQ(-EINVAL, rc);
+  EXPECT_EQ(0, rc);
   EXPECT_EQ(0, cras_make_fd_nonblocking_called);
   EXPECT_EQ(0, stream_list_add_called);
   EXPECT_EQ(0, stream_list_rm_called);
@@ -196,7 +196,7 @@ TEST_F(CPRMessageSuite, StreamConnectMessageInvalidAudioFormat) {
   fd_ = 100;
   rc =
       rclient_->ops->handle_message_from_client(rclient_, &msg.header, &fd_, 1);
-  EXPECT_EQ(-EINVAL, rc);
+  EXPECT_EQ(0, rc);
   EXPECT_EQ(0, cras_make_fd_nonblocking_called);
   EXPECT_EQ(0, stream_list_add_called);
   EXPECT_EQ(0, stream_list_rm_called);