From 9ae8d3820474a9525d707b1e19f7703721aec1c8 Mon Sep 17 00:00:00 2001
From: stijn <stijn@ignitron.net>
Date: Mon, 12 Sep 2022 14:45:39 +0200
Subject: [PATCH] extmod/vfs_posix_file: Implement finaliser for files.

Prevent handle leaks when file objects aren't closed explicitly and
fix some MICROPY_CPYTHON_COMPAT issues: this wasn't properly adhered
to because #ifdef was used so it was always on, and closing files
multiple times should be avoided unconditionally.
---
 extmod/vfs_posix_file.c       | 15 ++++++++-------
 tests/extmod/vfs_posix.py     | 29 +++++++++++++++++++++++++++++
 tests/extmod/vfs_posix.py.exp |  1 +
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/extmod/vfs_posix_file.c b/extmod/vfs_posix_file.c
index 4a37489686..ea19de7fd0 100644
--- a/extmod/vfs_posix_file.c
+++ b/extmod/vfs_posix_file.c
@@ -46,7 +46,7 @@ typedef struct _mp_obj_vfs_posix_file_t {
     int fd;
 } mp_obj_vfs_posix_file_t;
 
-#ifdef MICROPY_CPYTHON_COMPAT
+#if MICROPY_CPYTHON_COMPAT
 STATIC void check_fd_is_open(const mp_obj_vfs_posix_file_t *o) {
     if (o->fd < 0) {
         mp_raise_ValueError(MP_ERROR_TEXT("I/O operation on closed file"));
@@ -63,7 +63,7 @@ STATIC void vfs_posix_file_print(const mp_print_t *print, mp_obj_t self_in, mp_p
 }
 
 mp_obj_t mp_vfs_posix_file_open(const mp_obj_type_t *type, mp_obj_t file_in, mp_obj_t mode_in) {
-    mp_obj_vfs_posix_file_t *o = m_new_obj(mp_obj_vfs_posix_file_t);
+    mp_obj_vfs_posix_file_t *o = m_new_obj_with_finaliser(mp_obj_vfs_posix_file_t);
     const char *mode_s = mp_obj_str_get_str(mode_in);
 
     int mode_rw = 0, mode_x = 0;
@@ -185,12 +185,12 @@ STATIC mp_uint_t vfs_posix_file_ioctl(mp_obj_t o_in, mp_uint_t request, uintptr_
             return 0;
         }
         case MP_STREAM_CLOSE:
-            MP_THREAD_GIL_EXIT();
-            close(o->fd);
-            MP_THREAD_GIL_ENTER();
-            #ifdef MICROPY_CPYTHON_COMPAT
+            if (o->fd >= 0) {
+                MP_THREAD_GIL_EXIT();
+                close(o->fd);
+                MP_THREAD_GIL_ENTER();
+            }
             o->fd = -1;
-            #endif
             return 0;
         case MP_STREAM_GET_FILENO:
             return o->fd;
@@ -237,6 +237,7 @@ STATIC const mp_rom_map_elem_t vfs_posix_rawfile_locals_dict_table[] = {
     { MP_ROM_QSTR(MP_QSTR_tell), MP_ROM_PTR(&mp_stream_tell_obj) },
     { MP_ROM_QSTR(MP_QSTR_flush), MP_ROM_PTR(&mp_stream_flush_obj) },
     { MP_ROM_QSTR(MP_QSTR_close), MP_ROM_PTR(&mp_stream_close_obj) },
+    { MP_ROM_QSTR(MP_QSTR___del__), MP_ROM_PTR(&mp_stream_close_obj) },
     { MP_ROM_QSTR(MP_QSTR___enter__), MP_ROM_PTR(&mp_identity_obj) },
     { MP_ROM_QSTR(MP_QSTR___exit__), MP_ROM_PTR(&vfs_posix_file___exit___obj) },
 };
diff --git a/tests/extmod/vfs_posix.py b/tests/extmod/vfs_posix.py
index d193236696..3c23140065 100644
--- a/tests/extmod/vfs_posix.py
+++ b/tests/extmod/vfs_posix.py
@@ -1,6 +1,7 @@
 # Test for VfsPosix
 
 try:
+    import gc
     import uos
 
     uos.VfsPosix
@@ -51,6 +52,34 @@ f = open(temp_dir + "/test", "r")
 print(f.read())
 f.close()
 
+# file finaliser, also see vfs_fat_finaliser.py
+names = [temp_dir + "/x%d" % i for i in range(4)]
+basefd = temp_dir + "/nextfd1"
+nextfd = temp_dir + "/nextfd2"
+
+with open(basefd, "w") as f:
+    base_file_no = f.fileno()
+
+for i in range(1024):  # move GC head forwards by allocating a lot of single blocks
+    []
+
+
+def write_files_without_closing():
+    for n in names:
+        open(n, "w").write(n)
+    sorted(list(range(128)), key=lambda x: x)  # use up Python and C stack so f is really gone
+
+
+write_files_without_closing()
+gc.collect()
+
+with open(nextfd, "w") as f:
+    next_file_no = f.fileno()
+    print("next_file_no <= base_file_no", next_file_no <= base_file_no)
+
+for n in names + [basefd, nextfd]:
+    uos.remove(n)
+
 # rename
 uos.rename(temp_dir + "/test", temp_dir + "/test2")
 print(uos.listdir(temp_dir))
diff --git a/tests/extmod/vfs_posix.py.exp b/tests/extmod/vfs_posix.py.exp
index eb9ab43106..99922e621d 100644
--- a/tests/extmod/vfs_posix.py.exp
+++ b/tests/extmod/vfs_posix.py.exp
@@ -4,6 +4,7 @@ True
 <class 'list'>
 <io.TextIOWrapper 2>
 hello
+next_file_no <= base_file_no True
 ['test2']
 ['test2']
 <class 'tuple'>