extmod/vfs: Add finaliser to ilistdir to close directory handle.

When iterating over filesystem/folders with os.iterdir(), an open file
(directory) handle is used internally.  Currently this file handle is only
closed once the iterator is completely drained, eg. once all entries have
been looped over / converted into list etc.

If a program opens an iterdir but does not loop over it, or starts to loop
over the iterator but breaks out of the loop, then the handle never gets
closed.  In this state, when the iter object is cleaned up by the garbage
collector this open handle can cause corruption of the filesystem.

Fixes issues #6568 and #8506.
This commit is contained in:
Andrew Leech 2022-09-09 09:48:01 +10:00 committed by Damien George
parent 582b3e4e78
commit 4e0964b59f
10 changed files with 368 additions and 6 deletions

View File

@ -28,6 +28,10 @@
#include "py/mpconfig.h" #include "py/mpconfig.h"
#if MICROPY_VFS_FAT #if MICROPY_VFS_FAT
#if !MICROPY_ENABLE_FINALISER
#error "MICROPY_VFS_FAT requires MICROPY_ENABLE_FINALISER"
#endif
#if !MICROPY_VFS #if !MICROPY_VFS
#error "with MICROPY_VFS_FAT enabled, must also enable MICROPY_VFS" #error "with MICROPY_VFS_FAT enabled, must also enable MICROPY_VFS"
#endif #endif
@ -118,6 +122,7 @@ STATIC MP_DEFINE_CONST_STATICMETHOD_OBJ(fat_vfs_mkfs_obj, MP_ROM_PTR(&fat_vfs_mk
typedef struct _mp_vfs_fat_ilistdir_it_t { typedef struct _mp_vfs_fat_ilistdir_it_t {
mp_obj_base_t base; mp_obj_base_t base;
mp_fun_1_t iternext; mp_fun_1_t iternext;
mp_fun_1_t finaliser;
bool is_str; bool is_str;
FF_DIR dir; FF_DIR dir;
} mp_vfs_fat_ilistdir_it_t; } mp_vfs_fat_ilistdir_it_t;
@ -162,6 +167,13 @@ STATIC mp_obj_t mp_vfs_fat_ilistdir_it_iternext(mp_obj_t self_in) {
return MP_OBJ_STOP_ITERATION; return MP_OBJ_STOP_ITERATION;
} }
STATIC mp_obj_t mp_vfs_fat_ilistdir_it_del(mp_obj_t self_in) {
mp_vfs_fat_ilistdir_it_t *self = MP_OBJ_TO_PTR(self_in);
// ignore result / error because we may be closing a second time.
f_closedir(&self->dir);
return mp_const_none;
}
STATIC mp_obj_t fat_vfs_ilistdir_func(size_t n_args, const mp_obj_t *args) { STATIC mp_obj_t fat_vfs_ilistdir_func(size_t n_args, const mp_obj_t *args) {
mp_obj_fat_vfs_t *self = MP_OBJ_TO_PTR(args[0]); mp_obj_fat_vfs_t *self = MP_OBJ_TO_PTR(args[0]);
bool is_str_type = true; bool is_str_type = true;
@ -176,8 +188,10 @@ STATIC mp_obj_t fat_vfs_ilistdir_func(size_t n_args, const mp_obj_t *args) {
} }
// Create a new iterator object to list the dir // Create a new iterator object to list the dir
mp_vfs_fat_ilistdir_it_t *iter = mp_obj_malloc(mp_vfs_fat_ilistdir_it_t, &mp_type_polymorph_iter); mp_vfs_fat_ilistdir_it_t *iter = m_new_obj_with_finaliser(mp_vfs_fat_ilistdir_it_t);
iter->base.type = &mp_type_polymorph_iter_with_finaliser;
iter->iternext = mp_vfs_fat_ilistdir_it_iternext; iter->iternext = mp_vfs_fat_ilistdir_it_iternext;
iter->finaliser = mp_vfs_fat_ilistdir_it_del;
iter->is_str = is_str_type; iter->is_str = is_str_type;
FRESULT res = f_opendir(&self->fatfs, &iter->dir, path); FRESULT res = f_opendir(&self->fatfs, &iter->dir, path);
if (res != FR_OK) { if (res != FR_OK) {

View File

@ -36,6 +36,10 @@
#include "extmod/vfs.h" #include "extmod/vfs.h"
#include "shared/timeutils/timeutils.h" #include "shared/timeutils/timeutils.h"
#if !MICROPY_ENABLE_FINALISER
#error "MICROPY_VFS_LFS requires MICROPY_ENABLE_FINALISER"
#endif
STATIC int MP_VFS_LFSx(dev_ioctl)(const struct LFSx_API (config) * c, int cmd, int arg, bool must_return_int) { STATIC int MP_VFS_LFSx(dev_ioctl)(const struct LFSx_API (config) * c, int cmd, int arg, bool must_return_int) {
mp_obj_t ret = mp_vfs_blockdev_ioctl(c->context, cmd, arg); mp_obj_t ret = mp_vfs_blockdev_ioctl(c->context, cmd, arg);
int ret_i = 0; int ret_i = 0;
@ -155,6 +159,7 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_3(MP_VFS_LFSx(open_obj), MP_VFS_LFSx(file_open));
typedef struct MP_VFS_LFSx (_ilistdir_it_t) { typedef struct MP_VFS_LFSx (_ilistdir_it_t) {
mp_obj_base_t base; mp_obj_base_t base;
mp_fun_1_t iternext; mp_fun_1_t iternext;
mp_fun_1_t finaliser;
bool is_str; bool is_str;
MP_OBJ_VFS_LFSx *vfs; MP_OBJ_VFS_LFSx *vfs;
LFSx_API(dir_t) dir; LFSx_API(dir_t) dir;
@ -163,11 +168,16 @@ typedef struct MP_VFS_LFSx (_ilistdir_it_t) {
STATIC mp_obj_t MP_VFS_LFSx(ilistdir_it_iternext)(mp_obj_t self_in) { STATIC mp_obj_t MP_VFS_LFSx(ilistdir_it_iternext)(mp_obj_t self_in) {
MP_VFS_LFSx(ilistdir_it_t) * self = MP_OBJ_TO_PTR(self_in); MP_VFS_LFSx(ilistdir_it_t) * self = MP_OBJ_TO_PTR(self_in);
if (self->vfs == NULL) {
return MP_OBJ_STOP_ITERATION;
}
struct LFSx_API (info) info; struct LFSx_API (info) info;
for (;;) { for (;;) {
int ret = LFSx_API(dir_read)(&self->vfs->lfs, &self->dir, &info); int ret = LFSx_API(dir_read)(&self->vfs->lfs, &self->dir, &info);
if (ret == 0) { if (ret == 0) {
LFSx_API(dir_close)(&self->vfs->lfs, &self->dir); LFSx_API(dir_close)(&self->vfs->lfs, &self->dir);
self->vfs = NULL;
return MP_OBJ_STOP_ITERATION; return MP_OBJ_STOP_ITERATION;
} }
if (!(info.name[0] == '.' && (info.name[1] == '\0' if (!(info.name[0] == '.' && (info.name[1] == '\0'
@ -190,6 +200,14 @@ STATIC mp_obj_t MP_VFS_LFSx(ilistdir_it_iternext)(mp_obj_t self_in) {
return MP_OBJ_FROM_PTR(t); return MP_OBJ_FROM_PTR(t);
} }
STATIC mp_obj_t MP_VFS_LFSx(ilistdir_it_del)(mp_obj_t self_in) {
MP_VFS_LFSx(ilistdir_it_t) * self = MP_OBJ_TO_PTR(self_in);
if (self->vfs != NULL) {
LFSx_API(dir_close)(&self->vfs->lfs, &self->dir);
}
return mp_const_none;
}
STATIC mp_obj_t MP_VFS_LFSx(ilistdir_func)(size_t n_args, const mp_obj_t *args) { STATIC mp_obj_t MP_VFS_LFSx(ilistdir_func)(size_t n_args, const mp_obj_t *args) {
MP_OBJ_VFS_LFSx *self = MP_OBJ_TO_PTR(args[0]); MP_OBJ_VFS_LFSx *self = MP_OBJ_TO_PTR(args[0]);
bool is_str_type = true; bool is_str_type = true;
@ -203,14 +221,17 @@ STATIC mp_obj_t MP_VFS_LFSx(ilistdir_func)(size_t n_args, const mp_obj_t *args)
path = vstr_null_terminated_str(&self->cur_dir); path = vstr_null_terminated_str(&self->cur_dir);
} }
MP_VFS_LFSx(ilistdir_it_t) * iter = mp_obj_malloc(MP_VFS_LFSx(ilistdir_it_t), &mp_type_polymorph_iter); MP_VFS_LFSx(ilistdir_it_t) * iter = m_new_obj_with_finaliser(MP_VFS_LFSx(ilistdir_it_t));
iter->base.type = &mp_type_polymorph_iter_with_finaliser;
iter->iternext = MP_VFS_LFSx(ilistdir_it_iternext); iter->iternext = MP_VFS_LFSx(ilistdir_it_iternext);
iter->finaliser = MP_VFS_LFSx(ilistdir_it_del);
iter->is_str = is_str_type; iter->is_str = is_str_type;
iter->vfs = self;
int ret = LFSx_API(dir_open)(&self->lfs, &iter->dir, path); int ret = LFSx_API(dir_open)(&self->lfs, &iter->dir, path);
if (ret < 0) { if (ret < 0) {
mp_raise_OSError(-ret); mp_raise_OSError(-ret);
} }
iter->vfs = self;
return MP_OBJ_FROM_PTR(iter); return MP_OBJ_FROM_PTR(iter);
} }
STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(MP_VFS_LFSx(ilistdir_obj), 1, 2, MP_VFS_LFSx(ilistdir_func)); STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(MP_VFS_LFSx(ilistdir_obj), 1, 2, MP_VFS_LFSx(ilistdir_func));

View File

@ -33,6 +33,10 @@
#if MICROPY_VFS_POSIX #if MICROPY_VFS_POSIX
#if !MICROPY_ENABLE_FINALISER
#error "MICROPY_VFS_POSIX requires MICROPY_ENABLE_FINALISER"
#endif
#include <stdio.h> #include <stdio.h>
#include <string.h> #include <string.h>
#include <sys/stat.h> #include <sys/stat.h>
@ -162,6 +166,7 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_1(vfs_posix_getcwd_obj, vfs_posix_getcwd);
typedef struct _vfs_posix_ilistdir_it_t { typedef struct _vfs_posix_ilistdir_it_t {
mp_obj_base_t base; mp_obj_base_t base;
mp_fun_1_t iternext; mp_fun_1_t iternext;
mp_fun_1_t finaliser;
bool is_str; bool is_str;
DIR *dir; DIR *dir;
} vfs_posix_ilistdir_it_t; } vfs_posix_ilistdir_it_t;
@ -226,10 +231,22 @@ STATIC mp_obj_t vfs_posix_ilistdir_it_iternext(mp_obj_t self_in) {
} }
} }
STATIC mp_obj_t vfs_posix_ilistdir_it_del(mp_obj_t self_in) {
vfs_posix_ilistdir_it_t *self = MP_OBJ_TO_PTR(self_in);
if (self->dir != NULL) {
MP_THREAD_GIL_EXIT();
closedir(self->dir);
MP_THREAD_GIL_ENTER();
}
return mp_const_none;
}
STATIC mp_obj_t vfs_posix_ilistdir(mp_obj_t self_in, mp_obj_t path_in) { STATIC mp_obj_t vfs_posix_ilistdir(mp_obj_t self_in, mp_obj_t path_in) {
mp_obj_vfs_posix_t *self = MP_OBJ_TO_PTR(self_in); mp_obj_vfs_posix_t *self = MP_OBJ_TO_PTR(self_in);
vfs_posix_ilistdir_it_t *iter = mp_obj_malloc(vfs_posix_ilistdir_it_t, &mp_type_polymorph_iter); vfs_posix_ilistdir_it_t *iter = m_new_obj_with_finaliser(vfs_posix_ilistdir_it_t);
iter->base.type = &mp_type_polymorph_iter_with_finaliser;
iter->iternext = vfs_posix_ilistdir_it_iternext; iter->iternext = vfs_posix_ilistdir_it_iternext;
iter->finaliser = vfs_posix_ilistdir_it_del;
iter->is_str = mp_obj_get_type(path_in) == &mp_type_str; iter->is_str = mp_obj_get_type(path_in) == &mp_type_str;
const char *path = vfs_posix_get_path_str(self, path_in); const char *path = vfs_posix_get_path_str(self, path_in);
if (path[0] == '\0') { if (path[0] == '\0') {

View File

@ -0,0 +1,75 @@
# Test ilistdir __del__ for VfsFat using a RAM device.
import gc
try:
import uos
uos.VfsFat
except (ImportError, AttributeError):
print("SKIP")
raise SystemExit
class RAMBlockDevice:
ERASE_BLOCK_SIZE = 4096
def __init__(self, blocks):
self.data = bytearray(blocks * self.ERASE_BLOCK_SIZE)
def readblocks(self, block, buf, off=0):
addr = block * self.ERASE_BLOCK_SIZE + off
for i in range(len(buf)):
buf[i] = self.data[addr + i]
def writeblocks(self, block, buf, off=0):
addr = block * self.ERASE_BLOCK_SIZE + off
for i in range(len(buf)):
self.data[addr + i] = buf[i]
def ioctl(self, op, arg):
if op == 4: # block count
return len(self.data) // self.ERASE_BLOCK_SIZE
if op == 5: # block size
return self.ERASE_BLOCK_SIZE
if op == 6: # erase block
return 0
def test(bdev, vfs_class):
vfs_class.mkfs(bdev)
vfs = vfs_class(bdev)
vfs.mkdir("/test_d1")
vfs.mkdir("/test_d2")
vfs.mkdir("/test_d3")
for i in range(10):
print(i)
# We want to partially iterate the ilistdir iterator to leave it in an
# open state, which will then test the finaliser when it's garbage collected.
idir = vfs.ilistdir("/")
print(any(idir))
# Alternate way of partially iterating the ilistdir object, modifying the
# filesystem while it's open.
for dname, *_ in vfs.ilistdir("/"):
vfs.rmdir(dname)
break
vfs.mkdir(dname)
# Also create a fully drained iterator and ensure trying to re-use it
# throws the correct exception.
idir_emptied = vfs.ilistdir("/")
l = list(idir_emptied)
print(len(l))
try:
next(idir_emptied)
except StopIteration:
pass
gc.collect()
vfs.open("/test", "w").close()
bdev = RAMBlockDevice(30)
test(bdev, uos.VfsFat)

View File

@ -0,0 +1,30 @@
0
True
3
1
True
4
2
True
4
3
True
4
4
True
4
5
True
4
6
True
4
7
True
4
8
True
4
9
True
4

View File

@ -0,0 +1,75 @@
# Test ilistdir __del__ for VfsLittle using a RAM device.
import gc
try:
import uos
uos.VfsLfs2
except (ImportError, AttributeError):
print("SKIP")
raise SystemExit
class RAMBlockDevice:
ERASE_BLOCK_SIZE = 1024
def __init__(self, blocks):
self.data = bytearray(blocks * self.ERASE_BLOCK_SIZE)
def readblocks(self, block, buf, off):
addr = block * self.ERASE_BLOCK_SIZE + off
for i in range(len(buf)):
buf[i] = self.data[addr + i]
def writeblocks(self, block, buf, off):
addr = block * self.ERASE_BLOCK_SIZE + off
for i in range(len(buf)):
self.data[addr + i] = buf[i]
def ioctl(self, op, arg):
if op == 4: # block count
return len(self.data) // self.ERASE_BLOCK_SIZE
if op == 5: # block size
return self.ERASE_BLOCK_SIZE
if op == 6: # erase block
return 0
def test(bdev, vfs_class):
vfs_class.mkfs(bdev)
vfs = vfs_class(bdev)
vfs.mkdir("/test_d1")
vfs.mkdir("/test_d2")
vfs.mkdir("/test_d3")
for i in range(10):
print(i)
# We want to partially iterate the ilistdir iterator to leave it in an
# open state, which will then test the finaliser when it's garbage collected.
idir = vfs.ilistdir("/")
print(any(idir))
# Alternate way of partially iterating the ilistdir object, modifying the
# filesystem while it's open.
for dname, *_ in vfs.ilistdir("/"):
vfs.rmdir(dname)
break
vfs.mkdir(dname)
# Also create a fully drained iterator and ensure trying to re-use it
# throws the correct exception.
idir_emptied = vfs.ilistdir("/")
l = list(idir_emptied)
print(len(l))
try:
next(idir_emptied)
except StopIteration:
pass
gc.collect()
vfs.open("/test", "w").close()
bdev = RAMBlockDevice(30)
test(bdev, uos.VfsLfs2)

View File

@ -0,0 +1,30 @@
0
True
3
1
True
4
2
True
4
3
True
4
4
True
4
5
True
4
6
True
4
7
True
4
8
True
4
9
True
4

View File

@ -0,0 +1,70 @@
# Test ilistdir __del__ for VfsPosix.
import gc
try:
import os
os.VfsPosix
except (ImportError, AttributeError):
print("SKIP")
raise SystemExit
def test(testdir):
vfs = os.VfsPosix(testdir)
vfs.mkdir("/test_d1")
vfs.mkdir("/test_d2")
vfs.mkdir("/test_d3")
for i in range(10):
print(i)
# We want to partially iterate the ilistdir iterator to leave it in an
# open state, which will then test the finaliser when it's garbage collected.
idir = vfs.ilistdir("/")
print(any(idir))
# Alternate way of partially iterating the ilistdir object, modifying the
# filesystem while it's open.
for dname, *_ in vfs.ilistdir("/"):
vfs.rmdir(dname)
break
vfs.mkdir(dname)
# Also create a fully drained iterator and ensure trying to re-use it
# throws the correct exception.
idir_emptied = vfs.ilistdir("/")
l = list(idir_emptied)
print(len(l))
try:
next(idir_emptied)
except StopIteration:
pass
gc.collect()
# Create and delete a file, try to flush out any filesystem
# corruption that may be caused over the loops.
vfs.open("/test", "w").close()
vfs.remove("/test")
# We need an empty directory for testing.
# Skip the test if it already exists.
temp_dir = "vfs_posix_ilistdir_del_test_dir"
try:
os.stat(temp_dir)
print("SKIP")
raise SystemExit
except OSError:
pass
os.mkdir(temp_dir)
test(temp_dir)
# Remove tempdir.
for td in os.listdir(temp_dir):
os.rmdir("/".join((temp_dir, td)))
os.rmdir(temp_dir)

View File

@ -0,0 +1,30 @@
0
True
3
1
True
3
2
True
3
3
True
3
4
True
3
5
True
3
6
True
3
7
True
3
8
True
3
9
True
3

View File

@ -632,7 +632,7 @@ function ci_unix_qemu_mips_run_tests {
# - (i)listdir does not work, it always returns the empty list (it's an issue with the underlying C call) # - (i)listdir does not work, it always returns the empty list (it's an issue with the underlying C call)
# - ffi tests do not work # - ffi tests do not work
file ./ports/unix/build-coverage/micropython file ./ports/unix/build-coverage/micropython
(cd tests && MICROPY_MICROPYTHON=../ports/unix/build-coverage/micropython ./run-tests.py --exclude 'vfs_posix.py' --exclude 'ffi_(callback|float|float2).py') (cd tests && MICROPY_MICROPYTHON=../ports/unix/build-coverage/micropython ./run-tests.py --exclude 'vfs_posix.*\.py' --exclude 'ffi_(callback|float|float2).py')
} }
function ci_unix_qemu_arm_setup { function ci_unix_qemu_arm_setup {
@ -652,7 +652,7 @@ function ci_unix_qemu_arm_run_tests {
# - (i)listdir does not work, it always returns the empty list (it's an issue with the underlying C call) # - (i)listdir does not work, it always returns the empty list (it's an issue with the underlying C call)
export QEMU_LD_PREFIX=/usr/arm-linux-gnueabi export QEMU_LD_PREFIX=/usr/arm-linux-gnueabi
file ./ports/unix/build-coverage/micropython file ./ports/unix/build-coverage/micropython
(cd tests && MICROPY_MICROPYTHON=../ports/unix/build-coverage/micropython ./run-tests.py --exclude 'vfs_posix.py') (cd tests && MICROPY_MICROPYTHON=../ports/unix/build-coverage/micropython ./run-tests.py --exclude 'vfs_posix.*\.py')
} }
######################################################################################## ########################################################################################