From 6b8d91581e1875d77a5a67f3c84635469c4007c4 Mon Sep 17 00:00:00 2001 From: duxbbo Date: Wed, 22 Jun 2022 01:21:26 +0000 Subject: [PATCH 1/2] fix crash bugs Signed-off-by: duxbbo Change-Id: I4214d0e4ea5d90a4becc7663e31ecabb144d9442 --- .../include/client_trans_proxy_file_common.h | 11 +- .../src/client_trans_proxy_file_common.c | 127 +++++++----------- tests/sdk/transmission/trans_channel/BUILD.gn | 16 +++ .../client_trans_proxy_file_common_test.cpp | 114 ++++++++++++++++ 4 files changed, 190 insertions(+), 78 deletions(-) create mode 100644 tests/sdk/transmission/trans_channel/proxy/client_trans_proxy_file_common_test.cpp diff --git a/sdk/transmission/trans_channel/proxy/include/client_trans_proxy_file_common.h b/sdk/transmission/trans_channel/proxy/include/client_trans_proxy_file_common.h index e48b1d3978..a4ecc4f610 100644 --- a/sdk/transmission/trans_channel/proxy/include/client_trans_proxy_file_common.h +++ b/sdk/transmission/trans_channel/proxy/include/client_trans_proxy_file_common.h @@ -34,6 +34,10 @@ #define PATH_SEPARATOR '/' +#ifdef __cplusplus +extern "C" { +#endif + typedef struct { uint8_t *buffer; uint32_t bufferSize; @@ -44,9 +48,6 @@ int32_t GetAndCheckRealPath(const char *filePath, char *absPath); bool CheckDestFilePathValid(const char *destFile); int32_t FrameIndexToType(uint64_t index, uint64_t frameNumber); -bool IntToByte(uint32_t value, char *buffer, uint32_t len); -bool ByteToInt(char *buffer, uint32_t len, uint32_t *outValue); - char *BufferToFileList(uint8_t *buffer, uint32_t bufferSize, int32_t *fileCount); int32_t FileListToBuffer(const char **destFile, uint32_t fileCnt, FileListBuffer *outbufferInfo); @@ -56,4 +57,8 @@ int32_t FileLock(int32_t fd, int32_t type, bool isBlock); int32_t TryFileLock(int32_t fd, int32_t type, int32_t retryTimes); int32_t FileUnLock(int32_t fd); +#ifdef __cplusplus +} +#endif + #endif \ No newline at end of file diff --git a/sdk/transmission/trans_channel/proxy/src/client_trans_proxy_file_common.c b/sdk/transmission/trans_channel/proxy/src/client_trans_proxy_file_common.c index be3dac6030..0d7b0db519 100644 --- a/sdk/transmission/trans_channel/proxy/src/client_trans_proxy_file_common.c +++ b/sdk/transmission/trans_channel/proxy/src/client_trans_proxy_file_common.c @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -30,6 +31,14 @@ #include "softbus_log.h" #include "softbus_type_def.h" +#pragma pack(push, 1) +struct FileListItem { + uint32_t index; + uint32_t fileNameLength; + char fileName[0]; +}; +#pragma pack(pop) + bool IsPathValid(char *filePath) { if (filePath == NULL) { @@ -101,34 +110,6 @@ int32_t FrameIndexToType(uint64_t index, uint64_t frameNumber) return TRANS_SESSION_FILE_ONGOINE_FRAME; } -bool IntToByte(uint32_t value, char *buffer, uint32_t len) -{ - if ((buffer == NULL) || (len < sizeof(uint32_t))) { - return false; - } - - for (uint32_t i = 0; i < BYTE_INT_NUM; i++) { - uint32_t offset = BIT_INT_NUM - (i + 1) * BIT_BYTE_NUM; - buffer[i] = (char)((value >> offset) & 0xFF); - } - return true; -} - -bool ByteToInt(char *buffer, uint32_t len, uint32_t *outValue) -{ - if ((outValue == NULL) || (buffer == NULL) || (len < sizeof(uint32_t))) { - return false; - } - uint32_t value = 0; - for (int32_t i = 0; i < BYTE_INT_NUM; i++) { - value <<= BIT_BYTE_NUM; - value |= buffer[i] & 0xFF; - } - - *outValue = value; - return true; -} - // crc校验表 static const unsigned char g_auchCRCHi[] = { 0x00, 0xC1, 0x81, 0x40, 0x01, 0xC0, 0x80, 0x41, 0x01, 0xC0, 0x80, 0x41, 0x00, 0xC1, @@ -186,88 +167,84 @@ uint16_t RTU_CRC(const unsigned char *puchMsg, uint16_t usDataLen) int32_t FileListToBuffer(const char **destFile, uint32_t fileCnt, FileListBuffer *outbufferInfo) { + if (destFile == NULL || outbufferInfo == NULL || fileCnt == 0) { + SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "%s:bad input", __func__); + return SOFTBUS_ERR; + } + int32_t errCode = SOFTBUS_OK; uint32_t totalLength = 0; uint32_t offset = 0; - uint32_t index; - for (index = 0; index < fileCnt; index++) { - totalLength += strlen(destFile[index]); + for (uint32_t i = 0; i < fileCnt; i++) { + size_t fileNameLength = strlen(destFile[i]); + if(fileNameLength > MAX_FILE_PATH_NAME_LEN) { + SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "file name too long at index %" PRIu32, i); + return SOFTBUS_INVALID_PARAM; + } else { + totalLength += fileNameLength; + } } - uint32_t fileNameSize = 0; - uint32_t indexSize = sizeof(index); - uint32_t bufferSize = totalLength + (indexSize + sizeof(fileNameSize)) * fileCnt; + size_t bufferSize = totalLength + (sizeof(struct FileListItem) * fileCnt); uint8_t *buffer = (uint8_t *)SoftBusCalloc(bufferSize); if (buffer == NULL) { SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "calloc filelist failed"); return SOFTBUS_MALLOC_ERR; } - char byteBuff[sizeof(int32_t)] = {0}; - for (index = 0; index < fileCnt; index++) { - if (IntToByte(index, byteBuff, indexSize) == false) { - goto EXIT; - } - if (memcpy_s(buffer + offset, bufferSize - offset, byteBuff, indexSize) != EOK) { - goto EXIT; - } - offset += indexSize; - fileNameSize = strlen(destFile[index]); - if (IntToByte(fileNameSize, byteBuff, indexSize) == false) { - goto EXIT; - } - if (memcpy_s(buffer + offset, bufferSize - offset, byteBuff, sizeof(fileNameSize)) != EOK) { - goto EXIT; - } - offset += sizeof(fileNameSize); - if (memcpy_s(buffer + offset, bufferSize - offset, destFile[index], fileNameSize) != EOK) { - goto EXIT; + for (uint32_t index = 0; index < fileCnt; index++) { + uint32_t fileNameSize = strlen(destFile[index]); + struct FileListItem *fileItem = (struct FileListItem *)(buffer + offset); + fileItem->index = htonl(index); + fileItem->fileNameLength = htonl(fileNameSize); + offset += sizeof(struct FileListItem); + + // note: no \0 here + if (memcpy_s(fileItem->fileName, bufferSize - offset, destFile[index], fileNameSize) != EOK) { + SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "%s:copy file name failed!", __func__); + errCode = SOFTBUS_ERR; + break; } + offset += fileNameSize; } + if (errCode != SOFTBUS_OK) { + SoftBusFree(buffer); + return errCode; + } + outbufferInfo->buffer = buffer; outbufferInfo->bufferSize = offset; return SOFTBUS_OK; -EXIT: - SoftBusFree(buffer); - return SOFTBUS_ERR; } char *BufferToFileList(uint8_t *buffer, uint32_t bufferSize, int32_t *fileCount) { - if ((buffer == NULL) || (fileCount == NULL)) { - SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "BufferToFileList input invalid"); + if ((buffer == NULL) || (fileCount == NULL) || bufferSize < sizeof(struct FileListItem)) { + SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "%s: input invalid", __func__); return NULL; } char *firstFile = (char *)SoftBusCalloc(MAX_FILE_PATH_NAME_LEN + 1); if (firstFile == NULL) { - SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "BufferToFileList calloc fail"); + SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "%s: calloc fail", __func__); return NULL; } uint32_t offset = 0; int32_t count = 0; - uint32_t fileNameLength = 0; - uint32_t byteLen = sizeof(uint32_t); - char byteBuff[sizeof(int32_t)] = {0}; - while (offset < bufferSize) { - offset += sizeof(uint32_t); - if (memcpy_s(byteBuff, sizeof(byteBuff), buffer + offset, byteLen) != EOK) { - SoftBusFree(firstFile); - return NULL; - } - if (ByteToInt(byteBuff, byteLen, &fileNameLength) == false) { - SoftBusFree(firstFile); - return NULL; - } - offset += byteLen; + while (offset < bufferSize - sizeof(struct FileListItem)) { + const struct FileListItem *fileListItem = (const struct FileListItem *)(buffer + offset); + offset += sizeof(struct FileListItem); + + uint32_t fileNameLength = ntohl(fileListItem->fileNameLength); if (fileNameLength > bufferSize - offset || fileNameLength > MAX_FILE_PATH_NAME_LEN) { - SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "BufferToFileList invalid fileLength"); + SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "%s: invalid fileLength", __func__); SoftBusFree(firstFile); return NULL; } /* only output first file path */ if (count == 0) { - if (memcpy_s(firstFile, MAX_FILE_PATH_NAME_LEN, buffer + offset, fileNameLength) != EOK) { + // note: no \0 in buffer + if (memcpy_s(firstFile, MAX_FILE_PATH_NAME_LEN, fileListItem->fileName, fileNameLength) != EOK) { SoftBusFree(firstFile); return NULL; } diff --git a/tests/sdk/transmission/trans_channel/BUILD.gn b/tests/sdk/transmission/trans_channel/BUILD.gn index bbddf65855..5ebfb23cea 100644 --- a/tests/sdk/transmission/trans_channel/BUILD.gn +++ b/tests/sdk/transmission/trans_channel/BUILD.gn @@ -18,6 +18,7 @@ trans_sdk_test_common_src = [ "tcp_direct/unittest/trans_tcp_direct_test.cpp", "udp/stream/trans_sdk_stream_test.cpp", ] + trans_sdk_test_common_inc = [ "$dsoftbus_root_path/core/common/include", "$dsoftbus_root_path/interfaces/inner_kits/transport", @@ -49,12 +50,27 @@ if (defined(ohos_lite)) { } else { import("//build/test.gni") + trans_sdk_proxy_test_src = [ + "proxy/client_trans_proxy_file_common_test.cpp" + ] + + trans_sdk_proxy_test_inc = [ + "$dsoftbus_root_path/sdk/transmission/trans_channel/proxy/include" + ] + + trans_sdk_proxy_test_deps = [ + "$dsoftbus_root_path/sdk:softbus_client" + ] + module_output_path = "dsoftbus/transmission" ohos_unittest("TransSdkTest") { module_out_path = module_output_path sources = trans_sdk_test_common_src + sources += trans_sdk_proxy_test_src include_dirs = trans_sdk_test_common_inc + include_dirs += trans_sdk_proxy_test_inc deps = trans_sdk_test_common_deps + deps += trans_sdk_proxy_test_deps if (is_standard_system) { external_deps = [ "hiviewdfx_hilog_native:libhilog" ] } else { diff --git a/tests/sdk/transmission/trans_channel/proxy/client_trans_proxy_file_common_test.cpp b/tests/sdk/transmission/trans_channel/proxy/client_trans_proxy_file_common_test.cpp new file mode 100644 index 0000000000..ec3299522b --- /dev/null +++ b/tests/sdk/transmission/trans_channel/proxy/client_trans_proxy_file_common_test.cpp @@ -0,0 +1,114 @@ +/* + * Copyright (c) 2022 Huawei Device Co., Ltd. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +#include "securec.h" +#include + +#include "client_trans_proxy_file_common.h" +#include "softbus_adapter_mem.h" +#include "softbus_error_code.h" + +using namespace std; +using namespace testing::ext; + +namespace OHOS { +const char *g_fileSet1[] = { + "/dev/path/to", + "/path/max/length/512/" + "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + "111111111111111111111111111111111111111111111111111" +}; + +class ClientTransProxyFileCommonTest : public testing::Test { +public: + ClientTransProxyFileCommonTest() {} + ~ClientTransProxyFileCommonTest() {} + static void SetUpTestCase(void); + static void TearDownTestCase(void); + void SetUp() override {} + void TearDown() override {} +}; + +void ClientTransProxyFileCommonTest::SetUpTestCase(void) {} +void ClientTransProxyFileCommonTest::TearDownTestCase(void) {} + +HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTest, TestSize.Level0) +{ + FileListBuffer bufferInfo = {0}; + EXPECT_EQ(0, FileListToBuffer(g_fileSet1, sizeof(g_fileSet1)/ sizeof(const char *), &bufferInfo)); + + int32_t fileCount = 0; + const char *oldFirstFileName = BufferToFileList(bufferInfo.buffer, bufferInfo.bufferSize, &fileCount); + + ASSERT_NE(oldFirstFileName, nullptr); + EXPECT_EQ(fileCount, sizeof(g_fileSet1)/ sizeof(const char *)); + + EXPECT_EQ(0, strcmp(oldFirstFileName, g_fileSet1[0])); + + SoftBusFree(bufferInfo.buffer); +} + +HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTestBadInput1, TestSize.Level0) +{ + FileListBuffer bufferInfo = {0}; + EXPECT_NE(0, FileListToBuffer(nullptr, sizeof(g_fileSet1)/ sizeof(const char *), &bufferInfo)); + + EXPECT_EQ(bufferInfo.buffer, nullptr); + EXPECT_EQ(bufferInfo.bufferSize, 0); +} + +HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTestBadInput2, TestSize.Level0) +{ + FileListBuffer bufferInfo = {0}; + EXPECT_NE(0, FileListToBuffer(g_fileSet1, 0, &bufferInfo)); + + EXPECT_EQ(bufferInfo.buffer, nullptr); + EXPECT_EQ(bufferInfo.bufferSize, 0); +} + +HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTestBadInput3, TestSize.Level0) +{ + EXPECT_NE(0, FileListToBuffer(g_fileSet1, sizeof(g_fileSet1) / sizeof(const char *), nullptr)); +} + +HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTestBadInput4, TestSize.Level0) +{ + const char *fileSet[] = { + "/dev/path/to", "", + "/path/max/length/more/than/512/" + "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" + "111111111111111111111111111111111111111111111111111" + }; + FileListBuffer bufferInfo = {0}; + EXPECT_EQ(SOFTBUS_INVALID_PARAM, FileListToBuffer(fileSet, sizeof(fileSet) / sizeof(const char *), &bufferInfo)); +} + +HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTestBadInput5, TestSize.Level0) +{ + const char *fileSet[] = {"/dev/path/to", ""}; + FileListBuffer bufferInfo = {0}; + EXPECT_EQ(SOFTBUS_INVALID_PARAM, FileListToBuffer(fileSet, sizeof(fileSet) / sizeof(const char *), &bufferInfo)); +} + +} // namespace OHOS \ No newline at end of file -- Gitee From bdbc1a11cbc149c84cd0412b1cee0c68694bfcd2 Mon Sep 17 00:00:00 2001 From: duxbbo Date: Thu, 23 Jun 2022 04:00:34 +0000 Subject: [PATCH 2/2] fix br send file issue Signed-off-by: duxbbo Change-Id: I805327141159e91342dc5b8d0bca495416320518 --- .../proxy/src/client_trans_proxy_file_common.c | 11 ++++++----- tests/sdk/transmission/trans_channel/BUILD.gn | 13 ++++--------- .../proxy/client_trans_proxy_file_common_test.cpp | 5 ++--- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/sdk/transmission/trans_channel/proxy/src/client_trans_proxy_file_common.c b/sdk/transmission/trans_channel/proxy/src/client_trans_proxy_file_common.c index 0d7b0db519..b072b983c4 100644 --- a/sdk/transmission/trans_channel/proxy/src/client_trans_proxy_file_common.c +++ b/sdk/transmission/trans_channel/proxy/src/client_trans_proxy_file_common.c @@ -156,9 +156,10 @@ uint16_t RTU_CRC(const unsigned char *puchMsg, uint16_t usDataLen) { unsigned char uchCRCHi = 0xFF; unsigned char uchCRCLo = 0xFF; - unsigned char uIndex; - while (usDataLen--) { - uIndex = uchCRCLo ^ (*puchMsg++); + uint16_t dataLen = usDataLen; + const uint8_t *data = puchMsg; + while (dataLen--) { + unsigned char uIndex = uchCRCLo ^ (*data++); uchCRCLo = uchCRCHi ^ g_auchCRCHi[uIndex]; uchCRCHi = g_auchCRCLo[uIndex]; } @@ -176,8 +177,8 @@ int32_t FileListToBuffer(const char **destFile, uint32_t fileCnt, FileListBuffer uint32_t offset = 0; for (uint32_t i = 0; i < fileCnt; i++) { size_t fileNameLength = strlen(destFile[i]); - if(fileNameLength > MAX_FILE_PATH_NAME_LEN) { - SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "file name too long at index %" PRIu32, i); + if (fileNameLength == 0 || fileNameLength > MAX_FILE_PATH_NAME_LEN) { + SoftBusLog(SOFTBUS_LOG_TRAN, SOFTBUS_LOG_ERROR, "bad file name at index %" PRIu32, i); return SOFTBUS_INVALID_PARAM; } else { totalLength += fileNameLength; diff --git a/tests/sdk/transmission/trans_channel/BUILD.gn b/tests/sdk/transmission/trans_channel/BUILD.gn index 5ebfb23cea..c87d16dd22 100644 --- a/tests/sdk/transmission/trans_channel/BUILD.gn +++ b/tests/sdk/transmission/trans_channel/BUILD.gn @@ -50,17 +50,12 @@ if (defined(ohos_lite)) { } else { import("//build/test.gni") - trans_sdk_proxy_test_src = [ - "proxy/client_trans_proxy_file_common_test.cpp" - ] + trans_sdk_proxy_test_src = [ "proxy/client_trans_proxy_file_common_test.cpp" ] - trans_sdk_proxy_test_inc = [ - "$dsoftbus_root_path/sdk/transmission/trans_channel/proxy/include" - ] + trans_sdk_proxy_test_inc = + [ "$dsoftbus_root_path/sdk/transmission/trans_channel/proxy/include" ] - trans_sdk_proxy_test_deps = [ - "$dsoftbus_root_path/sdk:softbus_client" - ] + trans_sdk_proxy_test_deps = [ "$dsoftbus_root_path/sdk:softbus_client" ] module_output_path = "dsoftbus/transmission" ohos_unittest("TransSdkTest") { diff --git a/tests/sdk/transmission/trans_channel/proxy/client_trans_proxy_file_common_test.cpp b/tests/sdk/transmission/trans_channel/proxy/client_trans_proxy_file_common_test.cpp index ec3299522b..72851a2f0c 100644 --- a/tests/sdk/transmission/trans_channel/proxy/client_trans_proxy_file_common_test.cpp +++ b/tests/sdk/transmission/trans_channel/proxy/client_trans_proxy_file_common_test.cpp @@ -16,8 +16,8 @@ #include #include -#include "securec.h" #include +#include "securec.h" #include "client_trans_proxy_file_common.h" #include "softbus_adapter_mem.h" @@ -92,7 +92,7 @@ HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTestBadInput3, TestSize HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTestBadInput4, TestSize.Level0) { const char *fileSet[] = { - "/dev/path/to", "", + "/dev/path/to", "/path/max/length/more/than/512/" "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" @@ -110,5 +110,4 @@ HWTEST_F(ClientTransProxyFileCommonTest, FileListToBufferTestBadInput5, TestSize FileListBuffer bufferInfo = {0}; EXPECT_EQ(SOFTBUS_INVALID_PARAM, FileListToBuffer(fileSet, sizeof(fileSet) / sizeof(const char *), &bufferInfo)); } - } // namespace OHOS \ No newline at end of file -- Gitee