Merge branch 'bugfix/esp_partition_unload_fix' into 'master'

esp_partition: Fixed use-after-free issue (coverity)

Closes IDF-6165 and IDF-6999

See merge request espressif/esp-idf!22608

components/esp_partition/host_test/partition_api_test/main/partition_api_test.c

@@ -8,19 +8,27 @@
 
 #include <string.h>
 #if __has_include(<bsd/string.h>)
-// for strlcpy
 #include <bsd/string.h>
 #endif
 #include <unistd.h>
+#include <sys/time.h>
 #include "esp_err.h"
 #include "esp_partition.h"
 #include "esp_private/partition_linux.h"
 #include "unity.h"
 #include "unity_fixture.h"
-
 #include "esp_log.h"
+
 const char *TAG = "partition_api_test";
 
+/* generate timestamp-based filename in /tmp dir */
+static void partition_test_get_unique_filename(char *filename, size_t len)
+{
+    struct timeval  tv;
+    gettimeofday(&tv, NULL);
+    long long int nanotimestamp = tv.tv_sec * 1000000000 + tv.tv_usec;
+    snprintf(filename, len, "/tmp/espparttest%lld", nanotimestamp);
+}
 
 TEST_GROUP(partition_api);
 
@@ -91,16 +99,16 @@ TEST(partition_api, test_partition_ops)
     size_t bufsize = sizeof(buff);
     size_t off = 0x100;
 
-    //8. esp_partition_write/raw
+    // esp_partition_write/raw
     esp_err_t err = esp_partition_write(partition_data, off, (const void *)buff, bufsize);
     TEST_ESP_OK(err);
 
-    //9. esp_partition_read/raw
+    // esp_partition_read/raw
     uint8_t buffout[32] = {0};
     err = esp_partition_read(partition_data, off, (void *)buffout, bufsize);
     TEST_ESP_OK(err);
 
-    //10. esp_partition_erase_range
+    // esp_partition_erase_range
     uint8_t buferase[bufsize];
     memset(buferase, 0xFF, bufsize);
     memset(buffout, 0, sizeof(buffout));
@@ -111,7 +119,7 @@ TEST(partition_api, test_partition_ops)
     TEST_ESP_OK(err);
     TEST_ASSERT_EQUAL(0, memcmp(buffout, buferase, bufsize));
 
-    //11. esp_partition_verify (partition_data)
+    // esp_partition_verify (partition_data)
     const esp_partition_t *verified_partition = esp_partition_verify(partition_data);
     TEST_ASSERT_NOT_NULL(verified_partition);
 }
@@ -241,6 +249,10 @@ TEST(partition_api, test_partition_mmap_reopen)
     esp_partition_file_munmap();
 }
 
+/* Positive TC to prove temporary file removal after file unmap.
+ * Error is reported during subsequent attempt to map already removed file.
+ * This error proves that file was removed after unmap as requested.
+ */
 TEST(partition_api, test_partition_mmap_remove)
 {
     // Scenario: default temporary flash file, write some data
@@ -294,10 +306,13 @@ TEST(partition_api, test_partition_mmap_remove)
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
 }
 
+/* Negative TC to ensure mmap setup is consistent prior call to mmap.
+ * Configuration specifies both flash file name to be mapped and its size.
+ * This is invalid combination as size is determined by the file itself.
+ */
 TEST(partition_api, test_partition_mmap_name_size)
 {
-    // Negative Scenario: conflicting settings - flash_file_name together with one or both of
+    // Negative Scenario: conflicting settings - flash_file_name together with one or both of flash_file_size, partition_file_name
-    //                    flash_file_size, partition_file_name
     // esp_partition_file_mmap should return ESP_ERR_INVALID_ARG
 
     // unmap file to have correct initial conditions, regardless of result
@@ -323,10 +338,13 @@ TEST(partition_api, test_partition_mmap_name_size)
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
 }
 
+/* Negative TC to ensure mmap setup checks presence of partition file name (partition table binary file)
+ * if flash size parameter was specified.
+ * This test case specifies just flash file size but omits partition table binary file name.
+ */
 TEST(partition_api, test_partition_mmap_size_no_partition)
 {
-    // Negative Scenario: conflicting settings - flash_file_name empty, flash_file_size set
+    // Negative Scenario: conflicting settings - flash_file_name empty, flash_file_size set and partition_file_name not set
-    //                    and partition_file_name not set
     // esp_partition_file_mmap should return ESP_ERR_INVALID_ARG
 
     // unmap file to have correct initial conditions, regardless of result
@@ -350,10 +368,12 @@ TEST(partition_api, test_partition_mmap_size_no_partition)
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
 }
 
+/* Negative TC to ensure mmap setup checks presence of flash size parameter if partition file name (partition table binary file) was specified.
+ * This test case specifies just partition table binary file name but omits flash file size.
+ */
 TEST(partition_api, test_partition_mmap_no_size_partition)
 {
-    // Negative Scenario: conflicting settings - flash_file_name empty, flash_file_size not set
+    // Negative Scenario: conflicting settings - flash_file_name empty, flash_file_size not set and partition_file_name set
-    //                    and partition_file_name set
     // esp_partition_file_mmap should return ESP_ERR_INVALID_ARG
 
     // unmap file to have correct initial conditions, regardless of result
@@ -378,6 +398,8 @@ TEST(partition_api, test_partition_mmap_no_size_partition)
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
 }
 
+/*  Negative TC to ensure missing flash file to be mapped is reported with correct error code.
+ */
 TEST(partition_api, test_partition_mmap_ffile_nf)
 {
     // Negative Scenario: specified flash_file_name file not found
@@ -391,12 +413,11 @@ TEST(partition_api, test_partition_mmap_ffile_nf)
     TEST_ASSERT_NOT_NULL(p_file_mmap_ctrl_input);
 
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
-    const char *flash_file_name = "/tmp/strangefilename.tmp";
 
-    // make sure file doesn't exist
+    // timestamp-based unique filename, the file is very unlikely to exist => no extra check
-    if (access(flash_file_name, F_OK) == 0) {
+    char flash_file_name[40] = {0};
-        // our strange file exists, skip rest of test
+    partition_test_get_unique_filename(flash_file_name, sizeof(flash_file_name));
-    } else {
+
     strlcpy(p_file_mmap_ctrl_input->flash_file_name, flash_file_name, sizeof(p_file_mmap_ctrl_input->flash_file_name));
 
     const uint8_t *p_mem_block = NULL;
@@ -409,8 +430,9 @@ TEST(partition_api, test_partition_mmap_ffile_nf)
     esp_partition_file_munmap();
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
 }
-}
 
+/* Negative TC to ensure missing binary partition file to be loaded is reported with correct error code.
+ */
 TEST(partition_api, test_partition_mmap_pfile_nf)
 {
     // Negative Scenario: specified partition_file_name file not found
@@ -424,12 +446,11 @@ TEST(partition_api, test_partition_mmap_pfile_nf)
     TEST_ASSERT_NOT_NULL(p_file_mmap_ctrl_input);
 
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
-    const char *partition_file_name = "/tmp/strangefilename.tmp";
 
-    // make sure file doesn't exist
+    // timestamp-based unique filename, the file is very unlikely to exist => no extra check
-    if (access(partition_file_name, F_OK) == 0) {
+    char partition_file_name[40] = {0};
-        // our strange file exists, skip rest of test
+    partition_test_get_unique_filename(partition_file_name, sizeof(partition_file_name));
-    } else {
+
     strlcpy(p_file_mmap_ctrl_input->partition_file_name, partition_file_name, sizeof(p_file_mmap_ctrl_input->partition_file_name));
     p_file_mmap_ctrl_input->flash_file_size = 0x10000;    // any non zero value to pass validation
 
@@ -443,12 +464,13 @@ TEST(partition_api, test_partition_mmap_pfile_nf)
     esp_partition_file_munmap();
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
 }
-}
 
+/* Negative TC to check that requested size of emulated flash is at least so big to be able to load binary partition table.
+ * Too small emulated flash size is introduced and respective error code is evaluated after mmap call.
+ */
 TEST(partition_api, test_partition_mmap_size_too_small)
 {
-    // Negative Scenario: specified flash file size too small
+    // Negative Scenario: specified flash file size too small to hold at least partition table at default offset
-    //                    to hold at least partition table at default offset
     // esp_partition_file_mmap should return ESP_ERR_INVALID_SIZE
 
     // unmap file to have correct initial conditions, regardless of result
@@ -475,8 +497,7 @@ TEST(partition_api, test_partition_mmap_size_too_small)
     memset(p_file_mmap_ctrl_input, 0, sizeof(*p_file_mmap_ctrl_input));
 }
 
-typedef struct
+typedef struct {
-{
     size_t read_ops;
     size_t write_ops;
     size_t erase_ops;
@@ -521,31 +542,36 @@ void read_stats(t_stats *p_stats)
     p_stats->write_bytes = esp_partition_get_write_bytes();
     p_stats->total_time = esp_partition_get_total_time();
 
-    for(size_t i = 0; i < p_stats->sector_erase_count_size; i++)
+    for (size_t i = 0; i < p_stats->sector_erase_count_size; i++) {
         p_stats->sector_erase_count[i] = esp_partition_get_sector_erase_count(i);
     }
+}
 
 // evaluates if final stats differ from initial stats by expected difference stats.
 // if there is no need to evaluate some stats, set respective expeted difference stats members to SIZE_MAX
 bool evaluate_stats(const t_stats *p_initial_stats, const t_stats *p_final_stats, const t_stats *p_expected_difference_stats)
 {
-    if(p_expected_difference_stats->read_ops != SIZE_MAX)
+    if (p_expected_difference_stats->read_ops != SIZE_MAX) {
         TEST_ASSERT_EQUAL(p_initial_stats->read_ops + p_expected_difference_stats->read_ops, p_final_stats->read_ops);
-    if(p_expected_difference_stats->write_ops != SIZE_MAX)
+    }
+    if (p_expected_difference_stats->write_ops != SIZE_MAX) {
         TEST_ASSERT_EQUAL(p_initial_stats->write_ops + p_expected_difference_stats->write_ops, p_final_stats->write_ops);
-    if(p_expected_difference_stats->erase_ops != SIZE_MAX)
+    }
+    if (p_expected_difference_stats->erase_ops != SIZE_MAX) {
         TEST_ASSERT_EQUAL(p_initial_stats->erase_ops + p_expected_difference_stats->erase_ops, p_final_stats->erase_ops);
-    if(p_expected_difference_stats->read_bytes != SIZE_MAX)
+    }
+    if (p_expected_difference_stats->read_bytes != SIZE_MAX) {
         TEST_ASSERT_EQUAL(p_initial_stats->read_bytes + p_expected_difference_stats->read_bytes, p_final_stats->read_bytes);
-    if(p_expected_difference_stats->write_bytes != SIZE_MAX)
+    }
+    if (p_expected_difference_stats->write_bytes != SIZE_MAX) {
         TEST_ASSERT_EQUAL(p_initial_stats->write_bytes + p_expected_difference_stats->write_bytes, p_final_stats->write_bytes);
-    if(p_expected_difference_stats->total_time != SIZE_MAX)
+    }
+    if (p_expected_difference_stats->total_time != SIZE_MAX) {
         TEST_ASSERT_EQUAL(p_initial_stats->total_time + p_expected_difference_stats->total_time, p_final_stats->total_time);
+    }
 
-    for(size_t i = 0; i < p_initial_stats->sector_erase_count_size; i++)
+    for (size_t i = 0; i < p_initial_stats->sector_erase_count_size; i++) {
-    {
+        if (p_expected_difference_stats->sector_erase_count[i] != SIZE_MAX) {
-        if(p_expected_difference_stats->sector_erase_count[i] != SIZE_MAX)
-        {
             size_t expected_value = p_initial_stats->sector_erase_count[i] + p_expected_difference_stats->sector_erase_count[i];
             size_t final_value = p_final_stats->sector_erase_count[i];
 
@@ -601,8 +627,9 @@ TEST(partition_api, test_partition_stats)
     size_t non_aligned_portions = (part_offset % ESP_PARTITION_EMULATED_SECTOR_SIZE) + (size % ESP_PARTITION_EMULATED_SECTOR_SIZE);
     size_t erase_ops = size / ESP_PARTITION_EMULATED_SECTOR_SIZE;
     erase_ops += non_aligned_portions / ESP_PARTITION_EMULATED_SECTOR_SIZE;
-    if((non_aligned_portions % ESP_PARTITION_EMULATED_SECTOR_SIZE) > 0)
+    if ((non_aligned_portions % ESP_PARTITION_EMULATED_SECTOR_SIZE) > 0) {
         erase_ops += 1;
+    }
 
     t_stats expected_difference_stats;
     init_stats(&expected_difference_stats);
@@ -613,8 +640,9 @@ TEST(partition_api, test_partition_stats)
     expected_difference_stats.read_bytes = size;
     expected_difference_stats.write_bytes = size;
     expected_difference_stats.total_time = SIZE_MAX;
-    for (size_t i = 0; i < expected_difference_stats.sector_erase_count_size; i++)
+    for (size_t i = 0; i < expected_difference_stats.sector_erase_count_size; i++) {
         expected_difference_stats.sector_erase_count[i] = SIZE_MAX;
+    }
 
     evaluate_stats(&initial_stats, &final_stats, &expected_difference_stats);
 

components/esp_partition/partition.c

@@ -9,16 +9,25 @@
 #include <string.h>
 #include <stdio.h>
 #include <sys/lock.h>
+
+/* interim to enable test_wl_host and test_fatfs_on_host compilation (both use IDF_TARGET_ESP32)
+ * should go back to #include "sys/queue.h" once the tests are switched to CMake
+ * see IDF-7000
+ */
+#if __has_include(<bsd/string.h>)
+#include <bsd/sys/queue.h>
+#else
+#include "sys/queue.h"
+#endif
+
 #include "sdkconfig.h"
 #include "esp_flash_partitions.h"
 #include "esp_attr.h"
 #include "esp_partition.h"
-
 #if !CONFIG_IDF_TARGET_LINUX
 #include "esp_flash.h"
 #include "esp_flash_encrypt.h"
 #endif
-
 #include "esp_log.h"
 #include "esp_rom_md5.h"
 #include "bootloader_util.h"
@@ -41,7 +50,6 @@
 // Enable built-in checks in queue.h in debug builds
 #define INVARIANTS
 #endif
-#include "sys/queue.h"
 
 typedef struct partition_list_item_ {
     esp_partition_t info;
@@ -229,7 +237,8 @@ void unload_partitions(void)
 {
     _lock_acquire(&s_partition_list_lock);
     partition_list_item_t *it;
-    SLIST_FOREACH(it, &s_partition_list, next) {
+    partition_list_item_t *tmp;
+    SLIST_FOREACH_SAFE(it, &s_partition_list, next, tmp) {
         SLIST_REMOVE(&s_partition_list, it, partition_list_item_, next);
         free(it);
     }
@@ -441,7 +450,8 @@ esp_err_t esp_partition_deregister_external(const esp_partition_t *partition)
     esp_err_t result = ESP_ERR_NOT_FOUND;
     _lock_acquire(&s_partition_list_lock);
     partition_list_item_t *it;
-    SLIST_FOREACH(it, &s_partition_list, next) {
+    partition_list_item_t *tmp;
+    SLIST_FOREACH_SAFE(it, &s_partition_list, next, tmp) {
         if (&it->info == partition) {
             if (!it->user_registered) {
                 result = ESP_ERR_INVALID_ARG;