From ac2b0cab19c13de4d74e2f4bfd9fb299df42cb51 Mon Sep 17 00:00:00 2001 From: Zheng Yongjun Date: Fri, 18 Feb 2022 09:40:28 +0800 Subject: [PATCH] =?UTF-8?q?=E7=BB=99appspawn=E4=BB=93=E8=BF=9B=E8=A1=8C?= =?UTF-8?q?=E5=9F=BA=E7=A1=80=E8=B4=A8=E9=87=8F=E5=8A=A0=E5=9B=BA=E7=9A=84?= =?UTF-8?q?=E8=A1=A5=E4=B8=81?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. 修改错误码，让函数返回的错误码返回的有意义 2. 修复connectFd在错误分支没有被关闭的问题 3. 修改错误码返回的不对，(-errno) 4. 增加参数校验 5. 修复部分函数出现错误没有return 6. 修复部分函数失败时，没有把资源释放 Signed-off-by: Zheng Yongjun --- src/appspawn_msg_peer.cpp | 10 ++--- src/appspawn_server.cpp | 12 ++++-- src/socket/appspawn_socket.cpp | 8 ++-- src/socket/client_socket.cpp | 3 +- src/socket/server_socket.cpp | 37 ++++++++++++------- .../app_spawn_msg_peer_test.cpp | 13 ++++--- .../app_spawn_socket_test.cpp | 5 ++- .../server_socket_test/server_socket_test.cpp | 7 ++-- 8 files changed, 57 insertions(+), 38 deletions(-) diff --git a/src/appspawn_msg_peer.cpp b/src/appspawn_msg_peer.cpp index 57e3826b..e658e755 100644 --- a/src/appspawn_msg_peer.cpp +++ b/src/appspawn_msg_peer.cpp @@ -50,12 +50,12 @@ int AppSpawnMsgPeer::Response(pid_t pid) { if ((socket_ == nullptr) || (connectFd_ < 0)) { HiLog::Error(LABEL, "Invalid socket params: connectFd %d", connectFd_); - return -1; + return -EINVAL; } if (socket_->WriteSocketMessage(connectFd_, &pid, sizeof(pid)) != sizeof(pid)) { HiLog::Error(LABEL, "Failed to write message: connectFd %d", connectFd_); - return -1; + return (-errno); } return 0; @@ -65,14 +65,14 @@ int AppSpawnMsgPeer::MsgPeer() { if ((socket_ == nullptr) || (connectFd_ < 0)) { HiLog::Error(LABEL, "Failed to init socket: connectFd %{public}d", connectFd_); - return -1; + return -EINVAL; } int32_t msgLen = sizeof(ClientSocket::AppProperty); buf_ = std::make_unique(msgLen); if (buf_ == nullptr) { HiLog::Error(LABEL, "buf_ is null pointer!"); - return -1; + return -EINVAL; } int32_t rLen = 0; @@ -86,7 +86,7 @@ int AppSpawnMsgPeer::MsgPeer() if ((rLen < 0) || (rLen > msgLen)) { HiLog::Error(LABEL, "AppSpawnMsgPeer::Failed to read msg from socket %{public}d", connectFd_); - return -1; + return -EINVAL; } } diff --git a/src/appspawn_server.cpp b/src/appspawn_server.cpp index e0a0a150..d8de7078 100755 --- a/src/appspawn_server.cpp +++ b/src/appspawn_server.cpp @@ -190,6 +190,7 @@ bool AppSpawnServer::ServerMain(char *longProcName, int64_t longProcNameLen) ClientSocket::AppProperty *appProperty = msg->GetMsg(); if (!CheckAppProperty(appProperty)) { msg->Response(-EINVAL); + socket_->CloseConnection(connectFd); continue; } @@ -198,6 +199,7 @@ bool AppSpawnServer::ServerMain(char *longProcName, int64_t longProcNameLen) if (pipe(fd) == -1) { HiLog::Error(LABEL, "create pipe fail, errno = %{public}d", errno); msg->Response(ERR_PIPE_FAIL); + socket_->CloseConnection(connectFd); continue; } @@ -208,6 +210,7 @@ bool AppSpawnServer::ServerMain(char *longProcName, int64_t longProcNameLen) close(fd[0]); close(fd[1]); msg->Response(-errno); + socket_->CloseConnection(connectFd); continue; } else if (pid == 0) { SpecialHandle(appProperty); @@ -282,6 +285,7 @@ int32_t AppSpawnServer::SetUidGid( HiLog::Error(LABEL, "gitTable is nullptr"); return (-errno); } + // set gids if (setgroups(gidCount, reinterpret_cast(&gitTable[0])) == -1) { HiLog::Error(LABEL, "setgroups failed: %{public}d, gids.size=%{public}u", errno, gidCount); @@ -299,6 +303,7 @@ int32_t AppSpawnServer::SetUidGid( HiLog::Error(LABEL, "setuid(%{public}u) failed: %{public}d", uid, errno); return (-errno); } + return ERR_OK; } @@ -341,6 +346,7 @@ int32_t AppSpawnServer::SetCapabilities() { // init cap __user_cap_header_struct cap_header; + if (memset_s(&cap_header, sizeof(cap_header), 0, sizeof(cap_header)) != EOK) { HiLog::Error(LABEL, "Failed to memset cap header"); return -EINVAL; @@ -375,7 +381,7 @@ int32_t AppSpawnServer::SetCapabilities() // set capabilities if (capset(&cap_header, &cap_data[0]) == -1) { HiLog::Error(LABEL, "capset failed: %{public}d", errno); - return errno; + return (-errno); } return ERR_OK; @@ -693,8 +699,8 @@ void AppSpawnServer::SetAppAccessToken(const ClientSocket::AppProperty *appPrope bool AppSpawnServer::SetAppProcProperty(int connectFd, const ClientSocket::AppProperty *appProperty, char *longProcName, int64_t longProcNameLen, const int32_t fd[FDLEN2]) { - if (appProperty == nullptr) { - HiLog::Error(LABEL, "appProperty is nullptr"); + if (appProperty == nullptr || longProcName == nullptr) { + HiLog::Error(LABEL, "appProperty or longProcName paramenter is nullptr"); return false; } diff --git a/src/socket/appspawn_socket.cpp b/src/socket/appspawn_socket.cpp index 83152802..4e63b4d1 100755 --- a/src/socket/appspawn_socket.cpp +++ b/src/socket/appspawn_socket.cpp @@ -50,7 +50,7 @@ int AppSpawnSocket::PackSocketAddr() { if (socketName_.empty()) { HiLog::Error(LABEL, "Invalid socket name: empty"); - return -1; + return -EINVAL; } if (memset_s(&socketAddr_, sizeof(socketAddr_), 0, sizeof(socketAddr_)) != EOK) { @@ -83,7 +83,7 @@ int AppSpawnSocket::CreateSocket() int socketFd = socket(AF_LOCAL, SOCK_SEQPACKET, 0); if (socketFd < 0) { HiLog::Error(LABEL, "Failed to create socket: %d", errno); - return -1; + return (-errno); } HiLog::Debug(LABEL, "Created socket with fd %d", socketFd); @@ -114,7 +114,7 @@ int AppSpawnSocket::ReadSocketMessage(int socketFd, void *buf, int len) ssize_t rLen = TEMP_FAILURE_RETRY(read(socketFd, buf, len)); if (rLen < 0) { HiLog::Error(LABEL, "Read message from fd %d error %zd: %d", socketFd, rLen, errno); - return -1; + return -EFAULT; } return rLen; @@ -135,7 +135,7 @@ int AppSpawnSocket::WriteSocketMessage(int socketFd, const void *buf, int len) HiLog::Debug(LABEL, "socket fd %d, wLen %zd", socketFd, wLen); if ((wLen <= 0) && (errno != EINTR)) { HiLog::Error(LABEL, "Failed to write message to fd %d, error %zd: %d", socketFd, wLen, errno); - return -1; + return (-errno); } } diff --git a/src/socket/client_socket.cpp b/src/socket/client_socket.cpp index d45eb1fd..44ea1688 100644 --- a/src/socket/client_socket.cpp +++ b/src/socket/client_socket.cpp @@ -36,7 +36,7 @@ int ClientSocket::CreateClient() socketFd_ = CreateSocket(); if (socketFd_ < 0) { HiLog::Error(LABEL, "Client: Create socket failed"); - return -1; + return socketFd_; } } @@ -69,6 +69,7 @@ int ClientSocket::ConnectSocket(int connectFd) if ((setsockopt(connectFd, SOL_SOCKET, SO_RCVTIMEO, &SOCKET_TIMEOUT, sizeof(SOCKET_TIMEOUT)) != 0) || (setsockopt(connectFd, SOL_SOCKET, SO_SNDTIMEO, &SOCKET_TIMEOUT, sizeof(SOCKET_TIMEOUT)) != 0)) { HiLog::Warn(LABEL, "Client: Failed to set opt of socket %d, err %d", connectFd, errno); + return -1; } if (connect(connectFd, reinterpret_cast(&socketAddr_), socketAddrLen_) < 0) { diff --git a/src/socket/server_socket.cpp b/src/socket/server_socket.cpp index 9a305338..05671538 100644 --- a/src/socket/server_socket.cpp +++ b/src/socket/server_socket.cpp @@ -83,6 +83,11 @@ void ServerSocket::CloseServer() HiLog::Debug(LABEL, "Server: Closed connection fd %d", fd); close(fd); } + + if ((unlink(socketAddr_.sun_path) != 0) && (errno != ENOENT)) { + HiLog::Error(LABEL, "Server: Failed to unlink, err %d", errno); + } + connectFds_.clear(); if (socketFd_ >= 0) { CloseSocket(socketFd_); @@ -102,7 +107,7 @@ int ServerSocket::BindSocket(int connectFd) { if (connectFd < 0) { HiLog::Error(LABEL, "Server: Invalid socket fd: %d", connectFd); - return -1; + return -EINVAL; } if (PackSocketAddr() != 0) { @@ -111,7 +116,7 @@ int ServerSocket::BindSocket(int connectFd) if ((unlink(socketAddr_.sun_path) != 0) && (errno != ENOENT)) { HiLog::Error(LABEL, "Server: Failed to unlink, err %d", errno); - return -1; + return (-errno); } int reuseAddr = 0; @@ -119,25 +124,24 @@ int ServerSocket::BindSocket(int connectFd) (setsockopt(connectFd, SOL_SOCKET, SO_RCVTIMEO, &SOCKET_TIMEOUT, sizeof(SOCKET_TIMEOUT)) != 0) || (setsockopt(connectFd, SOL_SOCKET, SO_SNDTIMEO, &SOCKET_TIMEOUT, sizeof(SOCKET_TIMEOUT)) != 0)) { HiLog::Warn(LABEL, "Server: Failed to set opt of socket %d, err %d", connectFd, errno); + return (-errno); } if (bind(connectFd, reinterpret_cast(&socketAddr_), socketAddrLen_) < 0) { HiLog::Error(LABEL, "Server: Bind socket fd %d, failed: %d", connectFd, errno); - return -1; + return (-errno); } if (chown(socketAddr_.sun_path, APPSPAWN_ID_ROOT, APPSPAWN_ID_SYSTEM)) { HiLog::Error(LABEL, "Server: failed to chown socket fd %d, failed: %d", connectFd, errno); - return -1; + return (-errno); } if (chmod(socketAddr_.sun_path, SOCKET_PERM)) { HiLog::Error(LABEL, "Server: failed to chmod socket fd %d, failed: %d", connectFd, errno); - if ((unlink(socketAddr_.sun_path) != 0) && (errno != ENOENT)) { - HiLog::Error(LABEL, "Server: Failed to unlink, err %d", errno); - } - return -1; + return (-errno); } - HiLog::Debug(LABEL, "Server: Bind socket fd %d", connectFd); + + HiLog::Debug(LABEL, "Server: Bind socket fd %d success", connectFd); return 0; } @@ -145,12 +149,12 @@ int ServerSocket::RegisterServerSocket(int &connectFd) { if (socketName_.empty()) { HiLog::Error(LABEL, "Server: Invalid socket name: empty"); - return -1; + return -EINVAL; } connectFd = CreateSocket(); if (connectFd < 0) { - return -1; + return connectFd; } if ((BindSocket(connectFd) != 0) || (listen(connectFd, listenBacklog_) < 0)) { @@ -159,9 +163,12 @@ int ServerSocket::RegisterServerSocket(int &connectFd) connectFd, listenBacklog_, errno); + if ((unlink(socketAddr_.sun_path) != 0) && (errno != ENOENT)) { + HiLog::Error(LABEL, "Server: Failed to unlink, err %d", errno); + } close(connectFd); connectFd = -1; - return -1; + return (-errno); } HiLog::Debug(LABEL, "Server: Suc to register server socket fd %d", connectFd); @@ -182,23 +189,25 @@ int ServerSocket::WaitForConnection(int connectFd) { if (connectFd < 0) { HiLog::Error(LABEL, "Server: Invalid args: connectFd %d", connectFd); - return -1; + return -EINVAL; } struct sockaddr_un clientAddr; socklen_t clientLen = sizeof(clientAddr); if (memset_s(&clientAddr, clientLen, 0, clientLen) != EOK) { HiLog::Warn(LABEL, "Server: Failed to memset client addr"); + return -EINVAL; } int connFd = accept(connectFd, reinterpret_cast(&clientAddr), &clientLen); if (connFd < 0) { - return -1; + return (-errno); } if ((setsockopt(connFd, SOL_SOCKET, SO_RCVTIMEO, &SOCKET_TIMEOUT, sizeof(SOCKET_TIMEOUT)) < 0) || (setsockopt(connFd, SOL_SOCKET, SO_SNDTIMEO, &SOCKET_TIMEOUT, sizeof(SOCKET_TIMEOUT)) < 0)) { HiLog::Warn(LABEL, "Server: Failed to set opt of Connection %d, err %d", connFd, errno); + return (-errno); } HiLog::Debug(LABEL, "Server: Connection accepted, connect fd %d", connFd); diff --git a/test/unittest/app_spawn_msg_peer_test/app_spawn_msg_peer_test.cpp b/test/unittest/app_spawn_msg_peer_test/app_spawn_msg_peer_test.cpp index 57192929..a38c96b4 100644 --- a/test/unittest/app_spawn_msg_peer_test/app_spawn_msg_peer_test.cpp +++ b/test/unittest/app_spawn_msg_peer_test/app_spawn_msg_peer_test.cpp @@ -14,6 +14,7 @@ */ #include +#include #include "mock_server_socket.h" #include "appspawn_msg_peer.h" @@ -82,7 +83,7 @@ HWTEST(AppSpawnMsgPeerTest, App_Spawn_Msg_Peer_002, TestSize.Level0) int32_t connectFd = -1; std::unique_ptr appSpawnMsgPeer = std::make_unique(serverSocket, connectFd); - EXPECT_EQ(-1, appSpawnMsgPeer->MsgPeer()); + EXPECT_EQ(-EINVAL, appSpawnMsgPeer->MsgPeer()); GTEST_LOG_(INFO) << "App_Spawn_Msg_Peer_002 end"; } @@ -103,7 +104,7 @@ HWTEST(AppSpawnMsgPeerTest, App_Spawn_Msg_Peer_003, TestSize.Level0) int32_t connectFd = 1; std::unique_ptr appSpawnMsgPeer = std::make_unique(serverSocket, connectFd); - EXPECT_EQ(-1, appSpawnMsgPeer->MsgPeer()); + EXPECT_EQ(-EINVAL, appSpawnMsgPeer->MsgPeer()); GTEST_LOG_(INFO) << "App_Spawn_Msg_Peer_003 end"; } @@ -124,7 +125,7 @@ HWTEST(AppSpawnMsgPeerTest, App_Spawn_Msg_Peer_004, TestSize.Level0) int32_t connectFd = -1; std::unique_ptr appSpawnMsgPeer = std::make_unique(serverSocket, connectFd); - EXPECT_EQ(-1, appSpawnMsgPeer->Response(1)); + EXPECT_EQ(-EINVAL, appSpawnMsgPeer->Response(1)); GTEST_LOG_(INFO) << "App_Spawn_Msg_Peer_004 end"; } @@ -145,7 +146,7 @@ HWTEST(AppSpawnMsgPeerTest, App_Spawn_Msg_Peer_005, TestSize.Level0) int32_t connectFd = 1; std::unique_ptr appSpawnMsgPeer = std::make_unique(serverSocket, connectFd); - EXPECT_EQ(-1, appSpawnMsgPeer->Response(1)); + EXPECT_EQ(-EINVAL, appSpawnMsgPeer->Response(1)); GTEST_LOG_(INFO) << "App_Spawn_Msg_Peer_005 end"; } @@ -168,7 +169,7 @@ HWTEST(AppSpawnMsgPeerTest, App_Spawn_Msg_Peer_006, TestSize.Level0) EXPECT_CALL(*mockServerSocket, ReadSocketMessage(_, _, _)).WillOnce(Return(-1)); EXPECT_CALL(*mockServerSocket, CloseConnection(_)).WillOnce(Return()); - EXPECT_EQ(-1, appSpawnMsgPeer->MsgPeer()); + EXPECT_EQ(-EINVAL, appSpawnMsgPeer->MsgPeer()); GTEST_LOG_(INFO) << "App_Spawn_Msg_Peer_006 end"; } @@ -191,7 +192,7 @@ HWTEST(AppSpawnMsgPeerTest, App_Spawn_Msg_Peer_007, TestSize.Level0) EXPECT_CALL(*mockServerSocket, ReadSocketMessage(_, _, _)).WillOnce(Return(sizeof(ClientSocket::AppProperty) + 1)); EXPECT_CALL(*mockServerSocket, CloseConnection(_)).WillOnce(Return()); - EXPECT_EQ(-1, appSpawnMsgPeer->MsgPeer()); + EXPECT_EQ(-EINVAL, appSpawnMsgPeer->MsgPeer()); GTEST_LOG_(INFO) << "App_Spawn_Msg_Peer_007 end"; } diff --git a/test/unittest/app_spawn_socket_test/app_spawn_socket_test.cpp b/test/unittest/app_spawn_socket_test/app_spawn_socket_test.cpp index ca5a5bb4..29b75608 100644 --- a/test/unittest/app_spawn_socket_test/app_spawn_socket_test.cpp +++ b/test/unittest/app_spawn_socket_test/app_spawn_socket_test.cpp @@ -15,6 +15,7 @@ #include #include +#include // redefine private and protected since testcase need to invoke and test private function #define private public @@ -111,7 +112,7 @@ HWTEST(AppSpawnSocketTest, App_Spawn_Socket_003, TestSize.Level0) std::unique_ptr appSpawnSocket = std::make_unique(""); EXPECT_TRUE(appSpawnSocket); - EXPECT_EQ(-1, appSpawnSocket->PackSocketAddr()); + EXPECT_EQ(-EINVAL, appSpawnSocket->PackSocketAddr()); GTEST_LOG_(INFO) << "App_Spawn_Socket_003 end"; } @@ -424,4 +425,4 @@ HWTEST(AppSpawnSocketTest, App_Spawn_Socket_015, TestSize.Level0) close(fd[1]); GTEST_LOG_(INFO) << "App_Spawn_Socket_015 end"; -} \ No newline at end of file +} diff --git a/test/unittest/server_socket_test/server_socket_test.cpp b/test/unittest/server_socket_test/server_socket_test.cpp index 0cfdbfe9..81059c47 100644 --- a/test/unittest/server_socket_test/server_socket_test.cpp +++ b/test/unittest/server_socket_test/server_socket_test.cpp @@ -15,6 +15,7 @@ #include #include +#include // redefine private and protected since testcase need to invoke and test private function #define private public @@ -86,7 +87,7 @@ HWTEST(ServerSocketTest, Server_Socket_002, TestSize.Level0) GTEST_LOG_(INFO) << "Server_Socket_002 start"; std::unique_ptr serverSocket = std::make_unique(""); - EXPECT_EQ(-1, serverSocket->RegisterServerSocket()); + EXPECT_EQ(-EINVAL, serverSocket->RegisterServerSocket()); GTEST_LOG_(INFO) << "Server_Socket_002 end"; } @@ -104,7 +105,7 @@ HWTEST(ServerSocketTest, Server_Socket_003, TestSize.Level0) GTEST_LOG_(INFO) << "Server_Socket_003 start"; std::unique_ptr serverSocket = std::make_unique("ServerSocketTest"); - EXPECT_EQ(-1, serverSocket->WaitForConnection(0)); + EXPECT_EQ(-ENOTSOCK, serverSocket->WaitForConnection(0)); GTEST_LOG_(INFO) << "Server_Socket_003 end"; } @@ -177,7 +178,7 @@ HWTEST(ServerSocketTest, Server_Socket_006, TestSize.Level0) "InvalidInvalidInvalidInvalidInvalidInvalidInvalidInvalidInvalidInvalidInvalidInvalid"; std::unique_ptr serverSocket = std::make_unique(invalidSocketName.c_str()); - EXPECT_EQ(-1, serverSocket->RegisterServerSocket()); + EXPECT_EQ(-ENOENT, serverSocket->RegisterServerSocket()); EXPECT_EQ(-1, serverSocket->GetSocketFd()); GTEST_LOG_(INFO) << "Server_Socket_006 end"; -- Gitee