Skip to content

Commit

Permalink
extmod/vfs: Add finaliser to ilistdir to close directory handle.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pi-anl authored and dpgeorge committed Sep 13, 2022
1 parent 582b3e4 commit 4e0964b
Show file tree
Hide file tree
Showing 10 changed files with 368 additions and 6 deletions.
16 changes: 15 additions & 1 deletion extmod/vfs_fat.c
Expand Up @@ -28,6 +28,10 @@
#include "py/mpconfig.h"
#if MICROPY_VFS_FAT

#if !MICROPY_ENABLE_FINALISER
#error "MICROPY_VFS_FAT requires MICROPY_ENABLE_FINALISER"
#endif

#if !MICROPY_VFS
#error "with MICROPY_VFS_FAT enabled, must also enable MICROPY_VFS"
#endif
Expand Down Expand Up @@ -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 {
mp_obj_base_t base;
mp_fun_1_t iternext;
mp_fun_1_t finaliser;
bool is_str;
FF_DIR dir;
} mp_vfs_fat_ilistdir_it_t;
Expand Down Expand Up @@ -162,6 +167,13 @@ STATIC mp_obj_t mp_vfs_fat_ilistdir_it_iternext(mp_obj_t self_in) {
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) {
mp_obj_fat_vfs_t *self = MP_OBJ_TO_PTR(args[0]);
bool is_str_type = true;
Expand All @@ -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
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->finaliser = mp_vfs_fat_ilistdir_it_del;
iter->is_str = is_str_type;
FRESULT res = f_opendir(&self->fatfs, &iter->dir, path);
if (res != FR_OK) {
Expand Down
25 changes: 23 additions & 2 deletions extmod/vfs_lfsx.c
Expand Up @@ -36,6 +36,10 @@
#include "extmod/vfs.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) {
mp_obj_t ret = mp_vfs_blockdev_ioctl(c->context, cmd, arg);
int ret_i = 0;
Expand Down Expand Up @@ -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) {
mp_obj_base_t base;
mp_fun_1_t iternext;
mp_fun_1_t finaliser;
bool is_str;
MP_OBJ_VFS_LFSx *vfs;
LFSx_API(dir_t) dir;
Expand All @@ -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) {
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;
for (;;) {
int ret = LFSx_API(dir_read)(&self->vfs->lfs, &self->dir, &info);
if (ret == 0) {
LFSx_API(dir_close)(&self->vfs->lfs, &self->dir);
self->vfs = NULL;
return MP_OBJ_STOP_ITERATION;
}
if (!(info.name[0] == '.' && (info.name[1] == '\0'
Expand All @@ -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);
}

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) {
MP_OBJ_VFS_LFSx *self = MP_OBJ_TO_PTR(args[0]);
bool is_str_type = true;
Expand All @@ -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);
}

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->finaliser = MP_VFS_LFSx(ilistdir_it_del);
iter->is_str = is_str_type;
iter->vfs = self;
int ret = LFSx_API(dir_open)(&self->lfs, &iter->dir, path);
if (ret < 0) {
mp_raise_OSError(-ret);
}
iter->vfs = self;
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));
Expand Down
19 changes: 18 additions & 1 deletion extmod/vfs_posix.c
Expand Up @@ -33,6 +33,10 @@

#if MICROPY_VFS_POSIX

#if !MICROPY_ENABLE_FINALISER
#error "MICROPY_VFS_POSIX requires MICROPY_ENABLE_FINALISER"
#endif

#include <stdio.h>
#include <string.h>
#include <sys/stat.h>
Expand Down Expand Up @@ -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 {
mp_obj_base_t base;
mp_fun_1_t iternext;
mp_fun_1_t finaliser;
bool is_str;
DIR *dir;
} vfs_posix_ilistdir_it_t;
Expand Down Expand Up @@ -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) {
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->finaliser = vfs_posix_ilistdir_it_del;
iter->is_str = mp_obj_get_type(path_in) == &mp_type_str;
const char *path = vfs_posix_get_path_str(self, path_in);
if (path[0] == '\0') {
Expand Down
75 changes: 75 additions & 0 deletions tests/extmod/vfs_fat_ilistdir_del.py
@@ -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)
30 changes: 30 additions & 0 deletions tests/extmod/vfs_fat_ilistdir_del.py.exp
@@ -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
75 changes: 75 additions & 0 deletions tests/extmod/vfs_lfs_ilistdir_del.py
@@ -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)
30 changes: 30 additions & 0 deletions tests/extmod/vfs_lfs_ilistdir_del.py.exp
@@ -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

0 comments on commit 4e0964b

Please sign in to comment.