probably fixes #152 SPIFFS_remove() doesn't free file descriptor

Author: pellepl

src/spiffs_hydrogen.c

@@ -78,9 +78,11 @@ s32_t SPIFFS_mount(spiffs *fs, spiffs_config *config, u8_t *work,
                  " fdsz:"_SPIPRIi " cachesz:"_SPIPRIi
                  "\n",
                  __func__,
-                 SPIFFS_CFG_PHYS_SZ(fs), SPIFFS_CFG_LOG_PAGE_SZ(fs), SPIFFS_CFG_LOG_BLOCK_SZ(fs),
-                 SPIFFS_CFG_PHYS_ADDR(fs),
+                 SPIFFS_CFG_PHYS_SZ(fs),
+                 SPIFFS_CFG_LOG_PAGE_SZ(fs),
+                 SPIFFS_CFG_LOG_BLOCK_SZ(fs),
                  SPIFFS_CFG_PHYS_ERASE_SZ(fs),
+                 SPIFFS_CFG_PHYS_ADDR(fs),
                  fd_space_size, cache_size);
   void *user_data;
   SPIFFS_LOCK(fs);

src/spiffs_nucleus.c

@@ -1052,13 +1052,15 @@ void spiffs_cb_object_event(
       obj_id_raw, spix, new_pix, new_size);
   for (i = 0; i < fs->fd_count; i++) {
     spiffs_fd *cur_fd = &fds[i];
-#if SPIFFS_TEMPORAL_FD_CACHE
-    if (cur_fd->score == 0 || (cur_fd->obj_id & ~SPIFFS_OBJ_ID_IX_FLAG) != obj_id) continue;
-#else
-    if (cur_fd->file_nbr == 0 || (cur_fd->obj_id & ~SPIFFS_OBJ_ID_IX_FLAG) != obj_id) continue;
+    if ((cur_fd->obj_id & ~SPIFFS_OBJ_ID_IX_FLAG) != obj_id) continue; // fd not related to updated file
+#if !SPIFFS_TEMPORAL_FD_CACHE
+    if (cur_fd->file_nbr == 0) continue; // fd closed
 #endif
-    if (spix == 0) {
+    if (spix == 0) { // object index header update
       if (ev != SPIFFS_EV_IX_DEL) {
+#if SPIFFS_TEMPORAL_FD_CACHE
+        if (cur_fd->score == 0) continue; // never used fd
+#endif
         SPIFFS_DBG("       callback: setting fd "_SPIPRIfd":"_SPIPRIid"(fdoffs:"_SPIPRIi" offs:"_SPIPRIi") objix_hdr_pix to "_SPIPRIpg", size:"_SPIPRIi"\n", SPIFFS_FH_OFFS(fs, cur_fd->file_nbr), cur_fd->obj_id, cur_fd->fdoffset, cur_fd->offset, new_pix, new_size);
         cur_fd->objix_hdr_pix = new_pix;
         if (new_size != 0) {
@@ -1086,10 +1088,11 @@ void spiffs_cb_object_event(
           spiffs_cache_fd_release(fs, cur_fd->cache_page);
         }
 #endif
+        SPIFFS_DBG("       callback: release fd "_SPIPRIfd":"_SPIPRIid" span:"_SPIPRIsp" objix_pix to "_SPIPRIpg"\n", SPIFFS_FH_OFFS(fs, cur_fd->file_nbr), cur_fd->obj_id, spix, new_pix);
         cur_fd->file_nbr = 0;
         cur_fd->obj_id = SPIFFS_OBJ_ID_DELETED;
       }
-    }
+    } // object index header update
     if (cur_fd->cursor_objix_spix == spix) {
       if (ev != SPIFFS_EV_IX_DEL) {
         SPIFFS_DBG("       callback: setting fd "_SPIPRIfd":"_SPIPRIid" span:"_SPIPRIsp" objix_pix to "_SPIPRIpg"\n", SPIFFS_FH_OFFS(fs, cur_fd->file_nbr), cur_fd->obj_id, spix, new_pix);
@@ -1098,7 +1101,7 @@ void spiffs_cb_object_event(
         cur_fd->cursor_objix_pix = 0;
       }
     }
-  }
+  } // fd update loop
 
 #if SPIFFS_IX_MAP
 
@@ -2245,7 +2248,7 @@ s32_t spiffs_fd_find_new(spiffs *fs, spiffs_fd **fd, const char *name) {
     }
   }
 
-  // find the free fd with least score
+  // find the free fd with least score or name match
   for (i = 0; i < fs->fd_count; i++) {
     spiffs_fd *cur_fd = &fds[i];
     if (cur_fd->file_nbr == 0) {

src/test/test_bugreports.c

@@ -1176,6 +1176,30 @@ TEST(seek_bug_148) {
 } TEST_END
 
 
+TEST(remove_release_fd_152) {
+  int res;
+  fs_reset_specific(0, 0, 64*1024, 4096, 4096, 256);
+  u8_t buf[1024];
+  memrand(buf, sizeof(buf));
+  TEST_CHECK_EQ(count_taken_fds(FS), 0);
+  spiffs_file fd1 = SPIFFS_open(FS, "removemeandloseafd", SPIFFS_O_CREAT | SPIFFS_O_RDWR, 0);
+  TEST_CHECK_GT(fd1, 0);
+  TEST_CHECK_EQ(count_taken_fds(FS), 1);
+  TEST_CHECK_EQ(SPIFFS_write(FS, fd1, &buf, sizeof(buf)), sizeof(buf));
+  TEST_CHECK_EQ(SPIFFS_close(FS, fd1), SPIFFS_OK);
+  TEST_CHECK_EQ(count_taken_fds(FS), 0);
+  spiffs_file fd2 = SPIFFS_open(FS, "removemeandloseafd", SPIFFS_O_RDWR, 0);
+  TEST_CHECK_GT(fd2, 0);
+  TEST_CHECK_EQ(count_taken_fds(FS), 1);
+  spiffs_file fd3 = SPIFFS_open(FS, "removemeandloseafd", SPIFFS_O_RDWR, 0);
+  TEST_CHECK_GT(fd3, 0);
+  TEST_CHECK_EQ(count_taken_fds(FS), 2);
+  TEST_CHECK_EQ(SPIFFS_remove(FS, "removemeandloseafd"), SPIFFS_OK);
+  TEST_CHECK_EQ(count_taken_fds(FS), 0);
+  return TEST_RES_OK;
+} TEST_END
+
+
 SUITE_TESTS(bug_tests)
   ADD_TEST(nodemcu_full_fs_1)
   ADD_TEST(nodemcu_full_fs_2)
@@ -1196,6 +1220,7 @@ SUITE_TESTS(bug_tests)
   ADD_TEST(fuzzer_found_2)
   ADD_TEST(fuzzer_found_3)
   ADD_TEST(fuzzer_found_4)
+  ADD_TEST(remove_release_fd_152)
   ADD_TEST_NON_DEFAULT(fuzzer_found_single_1)
   ADD_TEST_NON_DEFAULT(log_afl_test)
   ADD_TEST_NON_DEFAULT(afl_test)

src/test/test_spiffs.c

@@ -1091,5 +1091,13 @@ int run_file_config(int cfg_count, tfile_conf* cfgs, int max_runs, int max_concu
   return 0;
 }
 
-
-
+int count_taken_fds(spiffs *fs) {
+  int i;
+  spiffs_fd *fds = (spiffs_fd *)fs->fd_space;
+  int taken = 0;
+  for (i = 0; i < fs->fd_count; i++) {
+    spiffs_fd *cur_fd = &fds[i];
+    if (cur_fd->file_nbr) taken++;
+  }
+  return taken;
+}

src/test/test_spiffs.h

@@ -90,6 +90,7 @@ void invoke_error_after_read_bytes(u32_t b, char once_only);
 void invoke_error_after_write_bytes(u32_t b, char once_only);
 void fs_set_validate_flashing(int i);
 int get_error_count();
+int count_taken_fds(spiffs *fs);
 
 void memrand(u8_t *b, int len);
 int test_create_file(char *name);