From 4c8c2a80799012972bbcfe7abadc9727c41f9097 Mon Sep 17 00:00:00 2001 From: Jakob Hasse Date: Wed, 1 Dec 2021 12:16:49 +0800 Subject: [PATCH] bugfix (nvs): Fixed issues found by Coverity * Fixed potential memory leak * Fixed wrong strncpy usage * Fixed potential out of bounds access --- components/nvs_flash/include/nvs.h | 20 +++++--------- components/nvs_flash/src/nvs_page.cpp | 27 +++++++++---------- components/nvs_flash/src/nvs_storage.cpp | 34 ++++++++++-------------- tools/ci/check_copyright_ignore.txt | 3 --- 4 files changed, 32 insertions(+), 52 deletions(-) diff --git a/components/nvs_flash/include/nvs.h b/components/nvs_flash/include/nvs.h index 26183853b7..6a45327bba 100644 --- a/components/nvs_flash/include/nvs.h +++ b/components/nvs_flash/include/nvs.h @@ -1,16 +1,8 @@ -// Copyright 2015-2016 Espressif Systems (Shanghai) PTE 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. +/* + * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #ifndef ESP_NVS_H #define ESP_NVS_H @@ -105,7 +97,7 @@ typedef enum { */ typedef struct { char namespace_name[16]; /*!< Namespace to which key-value belong */ - char key[16]; /*!< Key of stored key-value pair */ + char key[NVS_KEY_NAME_MAX_SIZE]; /*!< Key of stored key-value pair */ nvs_type_t type; /*!< Type of stored key-value pair */ } nvs_entry_info_t; diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index 8097c096ce..6350c7b6aa 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -1,16 +1,8 @@ -// Copyright 2015-2016 Espressif Systems (Shanghai) PTE 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. +/* + * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include "nvs_page.hpp" #include #include @@ -200,6 +192,10 @@ esp_err_t Page::writeItem(uint8_t nsIndex, ItemType datatype, const char* key, c return ESP_ERR_NVS_VALUE_TOO_LONG; } + if ((!isVariableLengthType(datatype)) && dataSize > 8) { + return ESP_ERR_INVALID_ARG; + } + size_t totalSize = ENTRY_SIZE; size_t entriesCount = 1; if (isVariableLengthType(datatype)) { @@ -244,7 +240,8 @@ esp_err_t Page::writeItem(uint8_t nsIndex, ItemType datatype, const char* key, c return err; } - size_t left = dataSize / ENTRY_SIZE * ENTRY_SIZE; + size_t rest = dataSize % ENTRY_SIZE; + size_t left = dataSize - rest; if (left > 0) { err = writeEntryData(static_cast(data), left); if (err != ESP_OK) { @@ -252,7 +249,7 @@ esp_err_t Page::writeItem(uint8_t nsIndex, ItemType datatype, const char* key, c } } - size_t tail = dataSize - left; + size_t tail = rest; if (tail > 0) { std::fill_n(item.rawData, ENTRY_SIZE, 0xff); memcpy(item.rawData, static_cast(data) + left, tail); diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index ee04d869d3..ef1baa83bf 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -1,16 +1,8 @@ -// Copyright 2015-2016 Espressif Systems (Shanghai) PTE 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. +/* + * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include "nvs_storage.hpp" #ifndef ESP_PLATFORM @@ -419,11 +411,6 @@ esp_err_t Storage::createOrOpenNamespace(const char* nsName, bool canCreate, uin return ESP_ERR_NVS_NOT_ENOUGH_SPACE; } - NamespaceEntry* entry = new (std::nothrow) NamespaceEntry; - if (!entry) { - return ESP_ERR_NO_MEM; - } - auto err = writeItem(Page::NS_INDEX, ItemType::U8, nsName, &ns, sizeof(ns)); if (err != ESP_OK) { return err; @@ -431,6 +418,11 @@ esp_err_t Storage::createOrOpenNamespace(const char* nsName, bool canCreate, uin mNamespaceUsage.set(ns, true); nsIndex = ns; + NamespaceEntry* entry = new (std::nothrow) NamespaceEntry; + if (!entry) { + return ESP_ERR_NO_MEM; + } + entry->mIndex = ns; strncpy(entry->mName, nsName, sizeof(entry->mName) - 1); entry->mName[sizeof(entry->mName) - 1] = 0; @@ -734,11 +726,13 @@ esp_err_t Storage::calcEntriesInNamespace(uint8_t nsIndex, size_t& usedEntries) void Storage::fillEntryInfo(Item &item, nvs_entry_info_t &info) { info.type = static_cast(item.datatype); - strncpy(info.key, item.key, sizeof(info.key)); + strncpy(info.key, item.key, sizeof(info.key) - 1); + info.key[sizeof(info.key) - 1] = '\0'; for (auto &name : mNamespaces) { if(item.nsIndex == name.mIndex) { - strncpy(info.namespace_name, name.mName, sizeof(info.namespace_name)); + strncpy(info.namespace_name, name.mName, sizeof(info.namespace_name) - 1); + info.namespace_name[sizeof(info.namespace_name) -1] = '\0'; break; } } diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index cbcef964d5..d98eff2328 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -1383,7 +1383,6 @@ components/newlib/test_apps/main/test_newlib_main.c components/newlib/test_apps/main/test_stdatomic.c components/nvs_flash/host_test/fixtures/test_fixtures.hpp components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp -components/nvs_flash/include/nvs.h components/nvs_flash/include/nvs_flash.h components/nvs_flash/include/nvs_handle.hpp components/nvs_flash/nvs_partition_generator/nvs_partition_gen.py @@ -1400,7 +1399,6 @@ components/nvs_flash/src/nvs_handle_simple.cpp components/nvs_flash/src/nvs_handle_simple.hpp components/nvs_flash/src/nvs_item_hash_list.cpp components/nvs_flash/src/nvs_item_hash_list.hpp -components/nvs_flash/src/nvs_page.cpp components/nvs_flash/src/nvs_page.hpp components/nvs_flash/src/nvs_pagemanager.cpp components/nvs_flash/src/nvs_pagemanager.hpp @@ -1410,7 +1408,6 @@ components/nvs_flash/src/nvs_partition_lookup.cpp components/nvs_flash/src/nvs_partition_lookup.hpp components/nvs_flash/src/nvs_partition_manager.hpp components/nvs_flash/src/nvs_platform.hpp -components/nvs_flash/src/nvs_storage.cpp components/nvs_flash/src/nvs_storage.hpp components/nvs_flash/src/nvs_test_api.h components/nvs_flash/src/nvs_types.cpp