From f2919bd11c570fc5f5440d1f101be38f61e3d16b Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Thu, 12 Sep 2024 10:04:10 -0700 Subject: [PATCH] Add error checking to readdir, telldir, and closedir calls in dir.c Raise SystemCallError exception when these functions return an error. This changes behavior for the following case (found by the tests): ```ruby dir1 = Dir.new('..') dir2 = Dir.for_fd(dir1.fileno) dir1.close dir2.close ``` The above code is basically broken, as `dir1.close` closed the file descriptor. The subsequent `dir2.close` call is undefined behavior. When run in isolation, it raises Errno::EBADF after the change, but if another thread opens a file descriptor between the `dir1.close` and `dir2.close` calls, the `dir2.close` call could close the file descriptor opened by the other thread. Raising an exception is much better in this case as it makes it obvious there is a bug in the code. For the readdir check, since the GVL has already been released, reacquire it rb_thread_call_with_gvl if an exception needs to be raised. Due to the number of closedir calls, this adds static close_dir_data and check_closedir functions. The close_dir_data takes a struct dir_data * and handles setting the dir entry to NULL regardless of failure. Fixes [Bug #20586] Co-authored-by: Nobuyoshi Nakada --- dir.c | 54 +++++++++++++++++++++++++++++++++---------- test/ruby/test_dir.rb | 4 +++- win32/dir.h | 2 +- win32/win32.c | 3 ++- 4 files changed, 48 insertions(+), 15 deletions(-) diff --git a/dir.c b/dir.c index 84687227a2..24eec8dc15 100644 --- a/dir.c +++ b/dir.c @@ -571,6 +571,25 @@ opendir_without_gvl(const char *path) return opendir(path); } +static void +close_dir_data(struct dir_data *dp) +{ + if (dp->dir) { + if (closedir(dp->dir) < 0) { + dp->dir = NULL; + rb_sys_fail("closedir"); + } + dp->dir = NULL; + } +} + +static void +check_closedir(DIR *dirp) +{ + if (closedir(dirp) < 0) + rb_sys_fail("closedir"); +} + static VALUE dir_initialize(rb_execution_context_t *ec, VALUE dir, VALUE dirname, VALUE enc) { @@ -585,8 +604,7 @@ dir_initialize(rb_execution_context_t *ec, VALUE dir, VALUE dirname, VALUE enc) dirname = rb_str_dup_frozen(dirname); TypedData_Get_Struct(dir, struct dir_data, &dir_data_type, dp); - if (dp->dir) closedir(dp->dir); - dp->dir = NULL; + close_dir_data(dp); RB_OBJ_WRITE(dir, &dp->path, Qnil); dp->enc = fsenc; path = RSTRING_PTR(dirname); @@ -810,10 +828,22 @@ fundamental_encoding_p(rb_encoding *enc) # define READDIR(dir, enc) rb_w32_readdir((dir), (enc)) # define READDIR_NOGVL READDIR #else +NORETURN(static void *sys_failure(void *function)); +static void * +sys_failure(void *function) +{ + rb_sys_fail(function); +} + static void * nogvl_readdir(void *dir) { - return readdir(dir); + rb_errno_set(0); + if ((dir = readdir(dir)) == NULL) { + if (rb_errno()) + rb_thread_call_with_gvl(sys_failure, (void *)"readdir"); + } + return dir; } # define READDIR(dir, enc) IO_WITHOUT_GVL(nogvl_readdir, (void *)(dir)) @@ -962,7 +992,8 @@ dir_tell(VALUE dir) long pos; GetDIR(dir, dirp); - pos = telldir(dirp->dir); + if((pos = telldir(dirp->dir)) < 0) + rb_sys_fail("telldir"); return rb_int2inum(pos); } #else @@ -1081,8 +1112,7 @@ dir_close(VALUE dir) dirp = dir_get(dir); if (!dirp->dir) return Qnil; - closedir(dirp->dir); - dirp->dir = NULL; + close_dir_data(dirp); return Qnil; } @@ -2585,7 +2615,7 @@ static void glob_dir_finish(ruby_glob_entries_t *ent, int flags) { if (flags & FNM_GLOB_NOSORT) { - closedir(ent->nosort.dirp); + check_closedir(ent->nosort.dirp); ent->nosort.dirp = NULL; } else if (ent->sort.entries) { @@ -2616,7 +2646,7 @@ glob_opendir(ruby_glob_entries_t *ent, DIR *dirp, int flags, rb_encoding *enc) #ifdef _WIN32 if ((capacity = dirp->nfiles) > 0) { if (!(newp = GLOB_ALLOC_N(rb_dirent_t, capacity))) { - closedir(dirp); + check_closedir(dirp); return NULL; } ent->sort.entries = newp; @@ -2636,7 +2666,7 @@ glob_opendir(ruby_glob_entries_t *ent, DIR *dirp, int flags, rb_encoding *enc) ent->sort.entries[count++] = rdp; ent->sort.count = count; } - closedir(dirp); + check_closedir(dirp); if (count < capacity) { if (!(newp = GLOB_REALLOC_N(ent->sort.entries, count))) { glob_dir_finish(ent, 0); @@ -2651,7 +2681,7 @@ glob_opendir(ruby_glob_entries_t *ent, DIR *dirp, int flags, rb_encoding *enc) nomem: glob_dir_finish(ent, 0); - closedir(dirp); + check_closedir(dirp); return NULL; } @@ -2814,7 +2844,7 @@ glob_helper( # if NORMALIZE_UTF8PATH if (!(norm_p || magical || recursive)) { - closedir(dirp); + check_closedir(dirp); goto literally; } # endif @@ -3712,7 +3742,7 @@ nogvl_dir_empty_p(void *ptr) break; } } - closedir(dir); + check_closedir(dir); return (void *)result; } diff --git a/test/ruby/test_dir.rb b/test/ruby/test_dir.rb index b86cf330bc..78371a096b 100644 --- a/test/ruby/test_dir.rb +++ b/test/ruby/test_dir.rb @@ -710,7 +710,9 @@ class TestDir < Test::Unit::TestCase assert_equal(new_dir.chdir{Dir.pwd}, for_fd_dir.chdir{Dir.pwd}) ensure new_dir&.close - for_fd_dir&.close + if for_fd_dir + assert_raise(Errno::EBADF) { for_fd_dir.close } + end end else assert_raise(NotImplementedError) { Dir.for_fd(0) } diff --git a/win32/dir.h b/win32/dir.h index 0292c27c9c..da1c4b76af 100644 --- a/win32/dir.h +++ b/win32/dir.h @@ -36,7 +36,7 @@ struct direct* rb_w32_ureaddir(DIR *); long rb_w32_telldir(DIR *); void rb_w32_seekdir(DIR *, long); void rb_w32_rewinddir(DIR *); -void rb_w32_closedir(DIR *); +int rb_w32_closedir(DIR *); char *rb_w32_ugetcwd(char *, int); #define opendir(s) rb_w32_uopendir((s)) diff --git a/win32/win32.c b/win32/win32.c index 23ab773eec..4c51b381f2 100644 --- a/win32/win32.c +++ b/win32/win32.c @@ -2461,7 +2461,7 @@ rb_w32_rewinddir(DIR *dirp) // /* License: Artistic or GPL */ -void +int rb_w32_closedir(DIR *dirp) { if (dirp) { @@ -2471,6 +2471,7 @@ rb_w32_closedir(DIR *dirp) free(dirp->bits); free(dirp); } + return 0; } #if RUBY_MSVCRT_VERSION >= 140