From 7c2b4342b9da0df542c4b118571a04cdde98a5ec Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 19 Jul 2024 11:29:04 -0700 Subject: [PATCH 1/8] Release GVL around pw and gr function calls in Etc These can block as they parse files. I'm not sure if other function calls in Etc can block, so this doesn't add GVL releasing to them. Releasing the GVL for these methods results in thread-safety issues, since thread-safety for the methods relied previously on the GVL. Add a mutex and surround GVL-releasing calls with a mutex. --- ext/etc/etc.c | 127 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 109 insertions(+), 18 deletions(-) diff --git a/ext/etc/etc.c b/ext/etc/etc.c index fcbd1af..1aa04e8 100644 --- a/ext/etc/etc.c +++ b/ext/etc/etc.c @@ -10,6 +10,7 @@ #include "ruby.h" #include "ruby/encoding.h" #include "ruby/io.h" +#include "ruby/thread.h" #include #ifdef HAVE_UNISTD_H @@ -94,6 +95,30 @@ atomic_exchange(volatile rb_atomic_t *var, rb_atomic_t newval) } #endif +static VALUE mEtc_mutex; + +struct mEtc_mutex_args { + void * (*func)(void *); + void *arg; +}; + +static VALUE +mEtc_mutex_nogvl(VALUE arg) +{ + + struct mEtc_mutex_args *args = (struct mEtc_mutex_args *)arg; + return (VALUE)rb_thread_call_without_gvl(args->func, args->arg, RUBY_UBF_IO, 0); +} + +static void * +mEtc_mutex_synchronize(void * (*func)(void *), void *arg) +{ + struct mEtc_mutex_args args; + args.func = func; + args.arg = arg; + return (void *)rb_mutex_synchronize(mEtc_mutex, mEtc_mutex_nogvl, (VALUE)&args); +} + /* call-seq: * getlogin -> String * @@ -157,6 +182,37 @@ safe_setup_filesystem_str(const char *str) #endif #ifdef HAVE_GETPWENT +static void * +nogvl_getpwuid(void *uid) +{ + return getpwuid((uid_t)(VALUE)uid); +} + +static void * +nogvl_getpwnam(void *name) +{ + return getpwnam((const char *)name); +} + +static void * +nogvl_getpwent(void *_) +{ + return getpwent(); +} + +static void * +nogvl_setpwent(void *_) +{ + setpwent(); + return NULL; +} + +static void * +nogvl_endpwent(void *_) +{ + endpwent(); + return NULL; +} # ifdef __APPLE__ # define PW_TIME2VAL(t) INT2NUM((int)(t)) # else @@ -234,7 +290,7 @@ etc_getpwuid(int argc, VALUE *argv, VALUE obj) else { uid = getuid(); } - pwd = getpwuid(uid); + pwd = mEtc_mutex_synchronize(nogvl_getpwuid, (void *)(VALUE)uid); if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %d", (int)uid); return setup_passwd(pwd); #else @@ -264,7 +320,7 @@ etc_getpwnam(VALUE obj, VALUE nam) struct passwd *pwd; const char *p = StringValueCStr(nam); - pwd = getpwnam(p); + pwd = mEtc_mutex_synchronize(nogvl_getpwnam, (void *)p); if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %"PRIsVALUE, nam); return setup_passwd(pwd); #else @@ -277,7 +333,7 @@ static rb_atomic_t passwd_blocking; static VALUE passwd_ensure(VALUE _) { - endpwent(); + mEtc_mutex_synchronize(nogvl_endpwent, NULL); if (RUBY_ATOMIC_EXCHANGE(passwd_blocking, 0) != 1) { rb_raise(rb_eRuntimeError, "unexpected passwd_blocking"); } @@ -289,8 +345,8 @@ passwd_iterate(VALUE _) { struct passwd *pw; - setpwent(); - while ((pw = getpwent()) != 0) { + mEtc_mutex_synchronize(nogvl_setpwent, NULL); + while ((pw = mEtc_mutex_synchronize(nogvl_getpwent, NULL)) != 0) { rb_yield(setup_passwd(pw)); } return Qnil; @@ -335,7 +391,7 @@ etc_passwd(VALUE obj) if (rb_block_given_p()) { each_passwd(); } - else if ((pw = getpwent()) != 0) { + else if ((pw = mEtc_mutex_synchronize(nogvl_getpwent, NULL)) != 0) { return setup_passwd(pw); } #endif @@ -387,7 +443,7 @@ static VALUE etc_setpwent(VALUE obj) { #ifdef HAVE_GETPWENT - setpwent(); + mEtc_mutex_synchronize(nogvl_setpwent, NULL); #endif return Qnil; } @@ -402,7 +458,7 @@ static VALUE etc_endpwent(VALUE obj) { #ifdef HAVE_GETPWENT - endpwent(); + mEtc_mutex_synchronize(nogvl_endpwent, NULL); #endif return Qnil; } @@ -427,7 +483,7 @@ etc_getpwent(VALUE obj) #ifdef HAVE_GETPWENT struct passwd *pw; - if ((pw = getpwent()) != 0) { + if ((pw = mEtc_mutex_synchronize(nogvl_getpwent, NULL)) != 0) { return setup_passwd(pw); } #endif @@ -435,6 +491,38 @@ etc_getpwent(VALUE obj) } #ifdef HAVE_GETGRENT +static void * +nogvl_getgrgid(void *gid) +{ + return getgrgid((gid_t)(VALUE)gid); +} + +static void * +nogvl_getgrnam(void *name) +{ + return getgrnam((const char *)name); +} + +static void * +nogvl_getgrent(void *_) +{ + return getgrent(); +} + +static void * +nogvl_setgrent(void *_) +{ + setgrent(); + return NULL; +} + +static void * +nogvl_endgrent(void *_) +{ + endgrent(); + return NULL; +} + static VALUE setup_group(struct group *grp) { @@ -487,7 +575,7 @@ etc_getgrgid(int argc, VALUE *argv, VALUE obj) else { gid = getgid(); } - grp = getgrgid(gid); + grp = mEtc_mutex_synchronize(nogvl_getgrgid, (void *)(VALUE)gid); if (grp == 0) rb_raise(rb_eArgError, "can't find group for %d", (int)gid); return setup_group(grp); #else @@ -518,7 +606,7 @@ etc_getgrnam(VALUE obj, VALUE nam) struct group *grp; const char *p = StringValueCStr(nam); - grp = getgrnam(p); + grp = mEtc_mutex_synchronize(nogvl_getgrnam, (void *)p); if (grp == 0) rb_raise(rb_eArgError, "can't find group for %"PRIsVALUE, nam); return setup_group(grp); #else @@ -531,7 +619,7 @@ static rb_atomic_t group_blocking; static VALUE group_ensure(VALUE _) { - endgrent(); + mEtc_mutex_synchronize(nogvl_endgrent, NULL); if (RUBY_ATOMIC_EXCHANGE(group_blocking, 0) != 1) { rb_raise(rb_eRuntimeError, "unexpected group_blocking"); } @@ -543,8 +631,8 @@ group_iterate(VALUE _) { struct group *pw; - setgrent(); - while ((pw = getgrent()) != 0) { + mEtc_mutex_synchronize(nogvl_setgrent, NULL); + while ((pw = mEtc_mutex_synchronize(nogvl_getgrent, NULL)) != 0) { rb_yield(setup_group(pw)); } return Qnil; @@ -589,7 +677,7 @@ etc_group(VALUE obj) if (rb_block_given_p()) { each_group(); } - else if ((grp = getgrent()) != 0) { + else if ((grp = mEtc_mutex_synchronize(nogvl_getgrent, NULL)) != 0) { return setup_group(grp); } #endif @@ -639,7 +727,7 @@ static VALUE etc_setgrent(VALUE obj) { #ifdef HAVE_GETGRENT - setgrent(); + mEtc_mutex_synchronize(nogvl_setgrent, NULL); #endif return Qnil; } @@ -654,7 +742,7 @@ static VALUE etc_endgrent(VALUE obj) { #ifdef HAVE_GETGRENT - endgrent(); + mEtc_mutex_synchronize(nogvl_endgrent, NULL); #endif return Qnil; } @@ -678,7 +766,7 @@ etc_getgrent(VALUE obj) #ifdef HAVE_GETGRENT struct group *gr; - if ((gr = getgrent()) != 0) { + if ((gr = mEtc_mutex_synchronize(nogvl_getgrent, NULL)) != 0) { return setup_group(gr); } #endif @@ -1157,6 +1245,9 @@ Init_etc(void) rb_define_const(mEtc, "VERSION", rb_str_new_cstr(RUBY_ETC_VERSION)); init_constants(mEtc); + mEtc_mutex = rb_mutex_new(); + rb_gc_register_mark_object(mEtc_mutex); + rb_define_module_function(mEtc, "getlogin", etc_getlogin, 0); rb_define_module_function(mEtc, "getpwuid", etc_getpwuid, -1); From eea60efaa906f44dfe8425c4633869f395d4cd88 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Sat, 20 Jul 2024 07:58:16 -0700 Subject: [PATCH 2/8] Update ext/etc/etc.c Co-authored-by: Olle Jonsson --- ext/etc/etc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/etc/etc.c b/ext/etc/etc.c index 1aa04e8..274e3c4 100644 --- a/ext/etc/etc.c +++ b/ext/etc/etc.c @@ -105,7 +105,6 @@ struct mEtc_mutex_args { static VALUE mEtc_mutex_nogvl(VALUE arg) { - struct mEtc_mutex_args *args = (struct mEtc_mutex_args *)arg; return (VALUE)rb_thread_call_without_gvl(args->func, args->arg, RUBY_UBF_IO, 0); } From d649cf2aceb0e80409f3ab3c784a80017bc636a6 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Mon, 22 Jul 2024 10:43:01 -0700 Subject: [PATCH 3/8] Fix thread-safety issues There were significant thread-safety issues with releasing the GVL on platforms where the underlying functions are not thread-safe. However, I was only able to trigger the issues by manually adding rb_thread_schedule calls after the mutexes were released. This fixes the thread safety issues so that the mutex is held while creating the related Ruby objects to be returned. I've added thread-safety tests for get{pwuid,pwnam,grgid,grnam}. I've also added support for -DTEST_THREAD_SAFETY compile flag, which will call rb_thread_schedule after all mutex releases. The thread-safety tests pass with this compile flag set. --- ext/etc/etc.c | 143 ++++++++++++++++++++++++++++--------------- test/etc/test_etc.rb | 68 ++++++++++++++++++++ 2 files changed, 160 insertions(+), 51 deletions(-) diff --git a/ext/etc/etc.c b/ext/etc/etc.c index 274e3c4..86f83d8 100644 --- a/ext/etc/etc.c +++ b/ext/etc/etc.c @@ -48,6 +48,19 @@ static VALUE sGroup; #define HAVE_UNAME 1 #endif +#if defined(TEST_THREAD_SAFETY) +#define mutex_synchronize mutex_synchronize_thread_schedule +static VALUE +mutex_synchronize_thread_schedule(VALUE mutex, VALUE (*func)(VALUE arg), VALUE arg) +{ + VALUE v = rb_mutex_synchronize(mutex, func, arg); + rb_thread_schedule(); + return v; +} +#else +#define mutex_synchronize rb_mutex_synchronize +#endif + #ifdef STDC_HEADERS # include #else @@ -115,7 +128,7 @@ mEtc_mutex_synchronize(void * (*func)(void *), void *arg) struct mEtc_mutex_args args; args.func = func; args.arg = arg; - return (void *)rb_mutex_synchronize(mEtc_mutex, mEtc_mutex_nogvl, (VALUE)&args); + return (void *)mutex_synchronize(mEtc_mutex, mEtc_mutex_nogvl, (VALUE)&args); } /* call-seq: @@ -255,6 +268,34 @@ setup_passwd(struct passwd *pwd) 0 /*dummy*/ ); } + +static VALUE +setup_passwd_getpwuid(VALUE uid) +{ + struct passwd *pwd = rb_thread_call_without_gvl(nogvl_getpwuid, (void *)uid, RUBY_UBF_IO, 0); + if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %d", (int)uid); + return setup_passwd(pwd); +} + +static VALUE +setup_passwd_getpwnam(VALUE nam) +{ + const char *p = StringValueCStr(nam); + struct passwd *pwd = rb_thread_call_without_gvl(nogvl_getpwnam, (void *)p, RUBY_UBF_IO, 0); + RB_GC_GUARD(nam); + if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %"PRIsVALUE, nam); + return setup_passwd(pwd); +} + +static VALUE +setup_passwd_getpwent(VALUE _) +{ + struct passwd *pw; + if ((pw = rb_thread_call_without_gvl(nogvl_getpwent, NULL, RUBY_UBF_IO, 0)) != 0) + return setup_passwd(pw); + else + return Qnil; +} #endif /* call-seq: @@ -281,7 +322,6 @@ etc_getpwuid(int argc, VALUE *argv, VALUE obj) #if defined(HAVE_GETPWENT) VALUE id; rb_uid_t uid; - struct passwd *pwd; if (rb_scan_args(argc, argv, "01", &id) == 1) { uid = NUM2UIDT(id); @@ -289,9 +329,7 @@ etc_getpwuid(int argc, VALUE *argv, VALUE obj) else { uid = getuid(); } - pwd = mEtc_mutex_synchronize(nogvl_getpwuid, (void *)(VALUE)uid); - if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %d", (int)uid); - return setup_passwd(pwd); + return mutex_synchronize(mEtc_mutex, setup_passwd_getpwuid, (VALUE)uid); #else return Qnil; #endif @@ -316,12 +354,7 @@ static VALUE etc_getpwnam(VALUE obj, VALUE nam) { #ifdef HAVE_GETPWENT - struct passwd *pwd; - const char *p = StringValueCStr(nam); - - pwd = mEtc_mutex_synchronize(nogvl_getpwnam, (void *)p); - if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %"PRIsVALUE, nam); - return setup_passwd(pwd); + return mutex_synchronize(mEtc_mutex, setup_passwd_getpwnam, nam); #else return Qnil; #endif @@ -342,11 +375,11 @@ passwd_ensure(VALUE _) static VALUE passwd_iterate(VALUE _) { - struct passwd *pw; + VALUE pw; mEtc_mutex_synchronize(nogvl_setpwent, NULL); - while ((pw = mEtc_mutex_synchronize(nogvl_getpwent, NULL)) != 0) { - rb_yield(setup_passwd(pw)); + while ((pw = mutex_synchronize(mEtc_mutex, setup_passwd_getpwent, Qnil)) != Qnil) { + rb_yield(pw); } return Qnil; } @@ -385,16 +418,13 @@ static VALUE etc_passwd(VALUE obj) { #ifdef HAVE_GETPWENT - struct passwd *pw; - if (rb_block_given_p()) { each_passwd(); } - else if ((pw = mEtc_mutex_synchronize(nogvl_getpwent, NULL)) != 0) { - return setup_passwd(pw); - } -#endif + return mutex_synchronize(mEtc_mutex, setup_passwd_getpwent, Qnil); +#else return Qnil; +#endif } /* call-seq: @@ -480,13 +510,10 @@ static VALUE etc_getpwent(VALUE obj) { #ifdef HAVE_GETPWENT - struct passwd *pw; - - if ((pw = mEtc_mutex_synchronize(nogvl_getpwent, NULL)) != 0) { - return setup_passwd(pw); - } -#endif + return mutex_synchronize(mEtc_mutex, setup_passwd_getpwent, Qnil); +#else return Qnil; +#endif } #ifdef HAVE_GETGRENT @@ -542,6 +569,34 @@ setup_group(struct group *grp) GIDT2NUM(grp->gr_gid), mem); } + +static VALUE +setup_group_getgrgid(VALUE gid) +{ + struct group *grp = rb_thread_call_without_gvl(nogvl_getgrgid, (void *)gid, RUBY_UBF_IO, 0); + if (grp == 0) rb_raise(rb_eArgError, "can't find group for %d", (int)gid); + return setup_group(grp); +} + +static VALUE +setup_group_getgrnam(VALUE nam) +{ + const char *p = StringValueCStr(nam); + struct group *grp = rb_thread_call_without_gvl(nogvl_getgrnam, (void *)p, RUBY_UBF_IO, 0); + RB_GC_GUARD(nam); + if (grp == 0) rb_raise(rb_eArgError, "can't find group for %"PRIsVALUE, nam); + return setup_group(grp); +} + +static VALUE +setup_group_getgrent(VALUE _) +{ + struct group *grp; + if ((grp = rb_thread_call_without_gvl(nogvl_getgrent, NULL, RUBY_UBF_IO, 0)) != 0) + return setup_group(grp); + else + return Qnil; +} #endif /* call-seq: @@ -566,7 +621,6 @@ etc_getgrgid(int argc, VALUE *argv, VALUE obj) #ifdef HAVE_GETGRENT VALUE id; gid_t gid; - struct group *grp; if (rb_scan_args(argc, argv, "01", &id) == 1) { gid = NUM2GIDT(id); @@ -574,9 +628,7 @@ etc_getgrgid(int argc, VALUE *argv, VALUE obj) else { gid = getgid(); } - grp = mEtc_mutex_synchronize(nogvl_getgrgid, (void *)(VALUE)gid); - if (grp == 0) rb_raise(rb_eArgError, "can't find group for %d", (int)gid); - return setup_group(grp); + return mutex_synchronize(mEtc_mutex, setup_group_getgrgid, (VALUE)gid); #else return Qnil; #endif @@ -602,12 +654,7 @@ static VALUE etc_getgrnam(VALUE obj, VALUE nam) { #ifdef HAVE_GETGRENT - struct group *grp; - const char *p = StringValueCStr(nam); - - grp = mEtc_mutex_synchronize(nogvl_getgrnam, (void *)p); - if (grp == 0) rb_raise(rb_eArgError, "can't find group for %"PRIsVALUE, nam); - return setup_group(grp); + return mutex_synchronize(mEtc_mutex, setup_group_getgrnam, nam); #else return Qnil; #endif @@ -628,11 +675,11 @@ group_ensure(VALUE _) static VALUE group_iterate(VALUE _) { - struct group *pw; + VALUE grp; mEtc_mutex_synchronize(nogvl_setgrent, NULL); - while ((pw = mEtc_mutex_synchronize(nogvl_getgrent, NULL)) != 0) { - rb_yield(setup_group(pw)); + while ((grp = mutex_synchronize(mEtc_mutex, setup_group_getgrent, Qnil)) != Qnil) { + rb_yield(grp); } return Qnil; } @@ -671,16 +718,13 @@ static VALUE etc_group(VALUE obj) { #ifdef HAVE_GETGRENT - struct group *grp; - if (rb_block_given_p()) { each_group(); } - else if ((grp = mEtc_mutex_synchronize(nogvl_getgrent, NULL)) != 0) { - return setup_group(grp); - } -#endif + return mutex_synchronize(mEtc_mutex, setup_group_getgrent, Qnil); +#else return Qnil; +#endif } #ifdef HAVE_GETGRENT @@ -763,13 +807,10 @@ static VALUE etc_getgrent(VALUE obj) { #ifdef HAVE_GETGRENT - struct group *gr; - - if ((gr = mEtc_mutex_synchronize(nogvl_getgrent, NULL)) != 0) { - return setup_group(gr); - } -#endif + return mutex_synchronize(mEtc_mutex, setup_group_getgrent, Qnil); +#else return Qnil; +#endif } #define numberof(array) (sizeof(array) / sizeof(*(array))) diff --git a/test/etc/test_etc.rb b/test/etc/test_etc.rb index 2eddcf4..b08e1b9 100644 --- a/test/etc/test_etc.rb +++ b/test/etc/test_etc.rb @@ -3,6 +3,8 @@ require "etc" class TestEtc < Test::Unit::TestCase + MAX_THREADS = 20 + def test_getlogin s = Etc.getlogin return if s == nil @@ -46,6 +48,21 @@ def test_getpwuid end end unless RUBY_PLATFORM.include?("android") + def test_getpwuid_thread_safety + uids = [] + Etc.passwd {|s| uids << s.uid} + uids.uniq! + uids = uids[-MAX_THREADS..-1] + + uids.map do |uid| + Thread.new do + 1000.times do + assert_equal uid, Etc.getpwuid(uid).uid + end + end + end.map(&:join) + end + def test_getpwnam passwd = {} Etc.passwd do |s| @@ -56,6 +73,24 @@ def test_getpwnam end end unless RUBY_PLATFORM.include?("android") + def test_getpwnam_thread_safety + passwd = {} + Etc.passwd do |s| + (passwd[s.name] ||= []) << s.uid unless /\A\+/ =~ s.name + end + passwd = passwd.to_a.reject{|name, uids| uids.length > 1} + passwd = passwd[-MAX_THREADS..-1] + + passwd.map do |name, uids| + uid = uids.first + Thread.new do + 1000.times do + assert_equal uid, Etc.getpwnam(name).uid + end + end + end.map(&:join) + end + def test_passwd_with_low_level_api a = [] Etc.passwd {|s| a << s } @@ -96,6 +131,21 @@ def test_getgrgid end end + def test_getgrgid_thread_safety + gids = [] + Etc.group {|g| gids << g.gid} + gids.uniq! + gids = gids[-MAX_THREADS..-1] + + gids.map do |gid| + Thread.new do + 1000.times do + assert_equal gid, Etc.getgrgid(gid).gid + end + end + end.map(&:join) + end + def test_getgrnam groups = Hash.new {[]} Etc.group do |s| @@ -106,6 +156,24 @@ def test_getgrnam end end + def test_getgrnam_thread_safety + groups = {} + Etc.group do |s| + (groups[s.name] ||= []) << s.gid unless /\A\+/ =~ s.name + end + groups = groups.to_a.reject{|name, gids| gids.length > 1} + groups = groups[-MAX_THREADS..-1] + + groups.map do |name, gids| + gid = gids.first + Thread.new do + 1000.times do + assert_equal gid, Etc.getgrnam(name).gid + end + end + end.map(&:join) + end + def test_group_with_low_level_api a = [] Etc.group {|s| a << s } From b4d4f192af076376c3ad7bfe45a7b35897d20081 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 24 Jul 2024 21:12:28 -0700 Subject: [PATCH 4/8] Switch from rb_mutex_synchronize to rb_nativethread_lock_{,un}lock Need to wrap rb_nativethread_lock_lock in rb_thread_call_without_gvl to avoid deadlock. --- ext/etc/etc.c | 54 +++++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/ext/etc/etc.c b/ext/etc/etc.c index 86f83d8..5b7f1ab 100644 --- a/ext/etc/etc.c +++ b/ext/etc/etc.c @@ -11,6 +11,7 @@ #include "ruby/encoding.h" #include "ruby/io.h" #include "ruby/thread.h" +#include "ruby/thread_native.h" #include #ifdef HAVE_UNISTD_H @@ -48,19 +49,6 @@ static VALUE sGroup; #define HAVE_UNAME 1 #endif -#if defined(TEST_THREAD_SAFETY) -#define mutex_synchronize mutex_synchronize_thread_schedule -static VALUE -mutex_synchronize_thread_schedule(VALUE mutex, VALUE (*func)(VALUE arg), VALUE arg) -{ - VALUE v = rb_mutex_synchronize(mutex, func, arg); - rb_thread_schedule(); - return v; -} -#else -#define mutex_synchronize rb_mutex_synchronize -#endif - #ifdef STDC_HEADERS # include #else @@ -108,7 +96,19 @@ atomic_exchange(volatile rb_atomic_t *var, rb_atomic_t newval) } #endif -static VALUE mEtc_mutex; +static rb_nativethread_lock_t mEtc_lock; +static rb_nativethread_lock_t *mEtc_mutex; + +static VALUE +mutex_synchronize(VALUE (*func)(VALUE), VALUE arg) +{ + rb_thread_call_without_gvl((void *(*)(void *))rb_nativethread_lock_lock, mEtc_mutex, RUBY_UBF_IO, 0); + VALUE v = rb_ensure(func, arg, (VALUE(*)(VALUE))rb_nativethread_lock_unlock, (VALUE)mEtc_mutex); +#if defined(TEST_THREAD_SAFETY) + rb_thread_schedule(); +#endif + return v; +} struct mEtc_mutex_args { void * (*func)(void *); @@ -128,7 +128,7 @@ mEtc_mutex_synchronize(void * (*func)(void *), void *arg) struct mEtc_mutex_args args; args.func = func; args.arg = arg; - return (void *)mutex_synchronize(mEtc_mutex, mEtc_mutex_nogvl, (VALUE)&args); + return (void *)mutex_synchronize(mEtc_mutex_nogvl, (VALUE)&args); } /* call-seq: @@ -329,7 +329,7 @@ etc_getpwuid(int argc, VALUE *argv, VALUE obj) else { uid = getuid(); } - return mutex_synchronize(mEtc_mutex, setup_passwd_getpwuid, (VALUE)uid); + return mutex_synchronize(setup_passwd_getpwuid, (VALUE)uid); #else return Qnil; #endif @@ -354,7 +354,7 @@ static VALUE etc_getpwnam(VALUE obj, VALUE nam) { #ifdef HAVE_GETPWENT - return mutex_synchronize(mEtc_mutex, setup_passwd_getpwnam, nam); + return mutex_synchronize(setup_passwd_getpwnam, nam); #else return Qnil; #endif @@ -378,7 +378,7 @@ passwd_iterate(VALUE _) VALUE pw; mEtc_mutex_synchronize(nogvl_setpwent, NULL); - while ((pw = mutex_synchronize(mEtc_mutex, setup_passwd_getpwent, Qnil)) != Qnil) { + while ((pw = mutex_synchronize(setup_passwd_getpwent, Qnil)) != Qnil) { rb_yield(pw); } return Qnil; @@ -421,7 +421,7 @@ etc_passwd(VALUE obj) if (rb_block_given_p()) { each_passwd(); } - return mutex_synchronize(mEtc_mutex, setup_passwd_getpwent, Qnil); + return mutex_synchronize(setup_passwd_getpwent, Qnil); #else return Qnil; #endif @@ -510,7 +510,7 @@ static VALUE etc_getpwent(VALUE obj) { #ifdef HAVE_GETPWENT - return mutex_synchronize(mEtc_mutex, setup_passwd_getpwent, Qnil); + return mutex_synchronize(setup_passwd_getpwent, Qnil); #else return Qnil; #endif @@ -628,7 +628,7 @@ etc_getgrgid(int argc, VALUE *argv, VALUE obj) else { gid = getgid(); } - return mutex_synchronize(mEtc_mutex, setup_group_getgrgid, (VALUE)gid); + return mutex_synchronize(setup_group_getgrgid, (VALUE)gid); #else return Qnil; #endif @@ -654,7 +654,7 @@ static VALUE etc_getgrnam(VALUE obj, VALUE nam) { #ifdef HAVE_GETGRENT - return mutex_synchronize(mEtc_mutex, setup_group_getgrnam, nam); + return mutex_synchronize(setup_group_getgrnam, nam); #else return Qnil; #endif @@ -678,7 +678,7 @@ group_iterate(VALUE _) VALUE grp; mEtc_mutex_synchronize(nogvl_setgrent, NULL); - while ((grp = mutex_synchronize(mEtc_mutex, setup_group_getgrent, Qnil)) != Qnil) { + while ((grp = mutex_synchronize(setup_group_getgrent, Qnil)) != Qnil) { rb_yield(grp); } return Qnil; @@ -721,7 +721,7 @@ etc_group(VALUE obj) if (rb_block_given_p()) { each_group(); } - return mutex_synchronize(mEtc_mutex, setup_group_getgrent, Qnil); + return mutex_synchronize(setup_group_getgrent, Qnil); #else return Qnil; #endif @@ -807,7 +807,7 @@ static VALUE etc_getgrent(VALUE obj) { #ifdef HAVE_GETGRENT - return mutex_synchronize(mEtc_mutex, setup_group_getgrent, Qnil); + return mutex_synchronize(setup_group_getgrent, Qnil); #else return Qnil; #endif @@ -1285,8 +1285,8 @@ Init_etc(void) rb_define_const(mEtc, "VERSION", rb_str_new_cstr(RUBY_ETC_VERSION)); init_constants(mEtc); - mEtc_mutex = rb_mutex_new(); - rb_gc_register_mark_object(mEtc_mutex); + mEtc_mutex = &mEtc_lock; + rb_nativethread_lock_initialize(mEtc_mutex); rb_define_module_function(mEtc, "getlogin", etc_getlogin, 0); From a4ba4e624e966dd5b2a9a8a055df470efa5585a2 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 24 Jul 2024 21:17:15 -0700 Subject: [PATCH 5/8] Skip thread safety tests if there are no users/groups to test with --- test/etc/test_etc.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/etc/test_etc.rb b/test/etc/test_etc.rb index b08e1b9..94f9571 100644 --- a/test/etc/test_etc.rb +++ b/test/etc/test_etc.rb @@ -52,7 +52,7 @@ def test_getpwuid_thread_safety uids = [] Etc.passwd {|s| uids << s.uid} uids.uniq! - uids = uids[-MAX_THREADS..-1] + return unless uids = uids[-MAX_THREADS..-1] uids.map do |uid| Thread.new do @@ -79,7 +79,7 @@ def test_getpwnam_thread_safety (passwd[s.name] ||= []) << s.uid unless /\A\+/ =~ s.name end passwd = passwd.to_a.reject{|name, uids| uids.length > 1} - passwd = passwd[-MAX_THREADS..-1] + return unless passwd = passwd[-MAX_THREADS..-1] passwd.map do |name, uids| uid = uids.first @@ -135,7 +135,7 @@ def test_getgrgid_thread_safety gids = [] Etc.group {|g| gids << g.gid} gids.uniq! - gids = gids[-MAX_THREADS..-1] + return unless gids = gids[-MAX_THREADS..-1] gids.map do |gid| Thread.new do @@ -162,7 +162,7 @@ def test_getgrnam_thread_safety (groups[s.name] ||= []) << s.gid unless /\A\+/ =~ s.name end groups = groups.to_a.reject{|name, gids| gids.length > 1} - groups = groups[-MAX_THREADS..-1] + return unless groups = groups[-MAX_THREADS..-1] groups.map do |name, gids| gid = gids.first From 177ded814525bbc592904c7df18288ecc47bb327 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Thu, 25 Jul 2024 13:47:37 -0700 Subject: [PATCH 6/8] Use get{pwuid,pwnam,grgid,grnam}_r functions if available The approach of releasing the GVL and using a mutex around get{pwuid,pwnam,pwent,grgid,grnam,grent} is only safe if the only calls to those functions are inside Etc. However, core Ruby can call the functions in process.c and file.c, and concurrent calls to get{pwuid,pwnam,pwent,grgid,grnam,grent} could result in segfaults if the GVL is released. If get{pwuid,pwnam,grgid,grnam}_r functions are supported, use them. If they are not supported, continue to call get{pwuid,pwnam,grgid,grnam}, but do not release the GVL, as doing so is not safe. This still releases the GVL for {end,set}{pw,gr}ent function calls, since those return void and should not cause problems. The get{pwuid,pwnam,grgid,grnam}_r function call logic was taken from Ruby's process.c. Remove use of native mutexes as they are no longer needed. --- ext/etc/etc.c | 401 ++++++++++++++++++++++++++++++++------------- ext/etc/extconf.rb | 4 + 2 files changed, 292 insertions(+), 113 deletions(-) diff --git a/ext/etc/etc.c b/ext/etc/etc.c index 5b7f1ab..c0103d8 100644 --- a/ext/etc/etc.c +++ b/ext/etc/etc.c @@ -11,7 +11,6 @@ #include "ruby/encoding.h" #include "ruby/io.h" #include "ruby/thread.h" -#include "ruby/thread_native.h" #include #ifdef HAVE_UNISTD_H @@ -60,6 +59,16 @@ RUBY_EXTERN char *getlogin(void); #define RUBY_ETC_VERSION "1.4.3" +# if defined(HAVE_GETPWNAM_R) || defined(HAVE_GETPWUID_R) +# define GETPW_R_SIZE_DEFAULT 0x1000 +# define GETPW_R_SIZE_LIMIT 0x10000 +# if defined(_SC_GETPW_R_SIZE_MAX) +# define GETPW_R_SIZE_INIT sysconf(_SC_GETPW_R_SIZE_MAX) +# else +# define GETPW_R_SIZE_INIT GETPW_R_SIZE_DEFAULT +# endif +# endif + #ifdef HAVE_RB_DEPRECATE_CONSTANT void rb_deprecate_constant(VALUE mod, const char *name); #else @@ -96,40 +105,18 @@ atomic_exchange(volatile rb_atomic_t *var, rb_atomic_t newval) } #endif -static rb_nativethread_lock_t mEtc_lock; -static rb_nativethread_lock_t *mEtc_mutex; - -static VALUE -mutex_synchronize(VALUE (*func)(VALUE), VALUE arg) -{ - rb_thread_call_without_gvl((void *(*)(void *))rb_nativethread_lock_lock, mEtc_mutex, RUBY_UBF_IO, 0); - VALUE v = rb_ensure(func, arg, (VALUE(*)(VALUE))rb_nativethread_lock_unlock, (VALUE)mEtc_mutex); #if defined(TEST_THREAD_SAFETY) - rb_thread_schedule(); -#endif - return v; -} - -struct mEtc_mutex_args { - void * (*func)(void *); - void *arg; -}; - -static VALUE -mEtc_mutex_nogvl(VALUE arg) -{ - struct mEtc_mutex_args *args = (struct mEtc_mutex_args *)arg; - return (VALUE)rb_thread_call_without_gvl(args->func, args->arg, RUBY_UBF_IO, 0); -} - static void * -mEtc_mutex_synchronize(void * (*func)(void *), void *arg) +WITHOUT_GVL(void *(*func)(void *), void *arg) { - struct mEtc_mutex_args args; - args.func = func; - args.arg = arg; - return (void *)mutex_synchronize(mEtc_mutex_nogvl, (VALUE)&args); + void *p = rb_thread_call_without_gvl(func, arg, RUBY_UBF_IO, 0); + rb_thread_schedule(); + return p; } +#else +# define WITHOUT_GVL(func, arg) rb_thread_call_without_gvl((func), (arg), RUBY_UBF_IO, 0) +#endif +#define WITHOUT_GVL_INT(func, arg) (int)(VALUE)WITHOUT_GVL((func), (arg)) /* call-seq: * getlogin -> String @@ -194,24 +181,6 @@ safe_setup_filesystem_str(const char *str) #endif #ifdef HAVE_GETPWENT -static void * -nogvl_getpwuid(void *uid) -{ - return getpwuid((uid_t)(VALUE)uid); -} - -static void * -nogvl_getpwnam(void *name) -{ - return getpwnam((const char *)name); -} - -static void * -nogvl_getpwent(void *_) -{ - return getpwent(); -} - static void * nogvl_setpwent(void *_) { @@ -269,29 +238,51 @@ setup_passwd(struct passwd *pwd) ); } -static VALUE -setup_passwd_getpwuid(VALUE uid) +#if HAVE_GETPWUID_R +struct getpwuid_r_args { + uid_t uid; + char *buf; + size_t bufsize; + struct passwd *result; + struct passwd pwstore; +}; + +# define GETPWUID_R_ARGS(uid_, buf_, bufsize_) (struct getpwuid_r_args) \ + {.uid = uid_, .buf = buf_, .bufsize = bufsize_, .result = NULL} + +static void * +nogvl_getpwuid_r(void *args) { - struct passwd *pwd = rb_thread_call_without_gvl(nogvl_getpwuid, (void *)uid, RUBY_UBF_IO, 0); - if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %d", (int)uid); - return setup_passwd(pwd); + struct getpwuid_r_args *arg = args; + return (void *)(VALUE)getpwuid_r(arg->uid, &arg->pwstore, arg->buf, arg->bufsize, &arg->result); } +#endif -static VALUE -setup_passwd_getpwnam(VALUE nam) +# ifdef HAVE_GETPWNAM_R +struct getpwnam_r_args { + char *login; + char *buf; + size_t bufsize; + struct passwd *result; + struct passwd pwstore; +}; + +# define GETPWNAM_R_ARGS(login_, buf_, bufsize_) (struct getpwnam_r_args) \ + {.login = login_, .buf = buf_, .bufsize = bufsize_, .result = NULL} + +static void * +nogvl_getpwnam_r(void *args) { - const char *p = StringValueCStr(nam); - struct passwd *pwd = rb_thread_call_without_gvl(nogvl_getpwnam, (void *)p, RUBY_UBF_IO, 0); - RB_GC_GUARD(nam); - if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %"PRIsVALUE, nam); - return setup_passwd(pwd); + struct getpwnam_r_args *arg = args; + return (void *)(VALUE)getpwnam_r(arg->login, &arg->pwstore, arg->buf, arg->bufsize, &arg->result); } +# endif static VALUE -setup_passwd_getpwent(VALUE _) +setup_passwd_getpwent(void) { struct passwd *pw; - if ((pw = rb_thread_call_without_gvl(nogvl_getpwent, NULL, RUBY_UBF_IO, 0)) != 0) + if ((pw = getpwent()) != 0) return setup_passwd(pw); else return Qnil; @@ -329,7 +320,56 @@ etc_getpwuid(int argc, VALUE *argv, VALUE obj) else { uid = getuid(); } - return mutex_synchronize(setup_passwd_getpwuid, (VALUE)uid); + +# ifdef HAVE_GETPWUID_R + char *bufid; + long bufsizeid = GETPW_R_SIZE_INIT; /* maybe -1 */ + + if (bufsizeid < 0) + bufsizeid = GETPW_R_SIZE_DEFAULT; + + VALUE getpwid_tmp = rb_str_tmp_new(bufsizeid); + + bufid = RSTRING_PTR(getpwid_tmp); + bufsizeid = rb_str_capacity(getpwid_tmp); + rb_str_set_len(getpwid_tmp, bufsizeid); + struct getpwuid_r_args args = GETPWUID_R_ARGS(uid, bufid, bufsizeid); + + int eid; + errno = 0; + while ((eid = WITHOUT_GVL_INT(nogvl_getpwuid_r, &args)) != 0) { + + if (eid == ENOENT || eid== ESRCH || eid == EBADF || eid == EPERM) { + /* not found; non-errors */ + rb_str_resize(getpwid_tmp, 0); + return Qnil; + } + + if (eid != ERANGE || args.bufsize >= GETPW_R_SIZE_LIMIT) { + rb_str_resize(getpwid_tmp, 0); + rb_syserr_fail(eid, "getpwuid_r"); + } + + rb_str_modify_expand(getpwid_tmp, args.bufsize); + args.buf = RSTRING_PTR(getpwid_tmp); + args.bufsize = (size_t)rb_str_capacity(getpwid_tmp); + } + + if (args.result == NULL) { + /* no record in the password database for the uid */ + rb_str_resize(getpwid_tmp, 0); + return Qnil; + } + + /* found it */ + VALUE result = setup_passwd(args.result); + rb_str_resize(getpwid_tmp, 0); + return result; +# else + struct passwd *pwd = getpwuid((uid_t)uid); + if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %d", (int)uid); + return setup_passwd(pwd); +# endif #else return Qnil; #endif @@ -354,7 +394,59 @@ static VALUE etc_getpwnam(VALUE obj, VALUE nam) { #ifdef HAVE_GETPWENT - return mutex_synchronize(setup_passwd_getpwnam, nam); +# ifdef HAVE_GETPWNAM_R + char *login = RSTRING_PTR(nam); + char *bufnm; + long bufsizenm = GETPW_R_SIZE_INIT; /* maybe -1 */ + + if (bufsizenm < 0) + bufsizenm = GETPW_R_SIZE_DEFAULT; + + VALUE getpwnm_tmp = rb_str_tmp_new(bufsizenm); + + bufnm = RSTRING_PTR(getpwnm_tmp); + bufsizenm = rb_str_capacity(getpwnm_tmp); + rb_str_set_len(getpwnm_tmp, bufsizenm); + struct getpwnam_r_args args = GETPWNAM_R_ARGS(login, bufnm, bufsizenm); + + int enm; + errno = 0; + while ((enm = WITHOUT_GVL_INT(nogvl_getpwnam_r, &args)) != 0) { + + if (enm == ENOENT || enm== ESRCH || enm == EBADF || enm == EPERM) { + /* not found; non-errors */ + rb_str_resize(getpwnm_tmp, 0); + return Qnil; + } + + if (enm != ERANGE || bufsizenm >= GETPW_R_SIZE_LIMIT) { + rb_str_resize(getpwnm_tmp, 0); + rb_syserr_fail(enm, "getpwnam_r"); + } + + rb_str_modify_expand(getpwnm_tmp, bufsizenm); + args.buf = RSTRING_PTR(getpwnm_tmp); + args.bufsize = (size_t)rb_str_capacity(getpwnm_tmp); + } + + if (args.result == NULL) { + /* no record in the password database for the login name */ + rb_str_resize(getpwnm_tmp, 0); + return Qnil; + } + + /* found it */ + VALUE result = setup_passwd(args.result); + rb_str_resize(getpwnm_tmp, 0); + RB_GC_GUARD(nam); + return result; +# else + const char *p = StringValueCStr(nam); + struct passwd *pwd = getpwnam(p); + if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %"PRIsVALUE, nam); + RB_GC_GUARD(nam); + return setup_passwd(pwd); +# endif #else return Qnil; #endif @@ -365,7 +457,7 @@ static rb_atomic_t passwd_blocking; static VALUE passwd_ensure(VALUE _) { - mEtc_mutex_synchronize(nogvl_endpwent, NULL); + WITHOUT_GVL(nogvl_endpwent, NULL); if (RUBY_ATOMIC_EXCHANGE(passwd_blocking, 0) != 1) { rb_raise(rb_eRuntimeError, "unexpected passwd_blocking"); } @@ -377,8 +469,8 @@ passwd_iterate(VALUE _) { VALUE pw; - mEtc_mutex_synchronize(nogvl_setpwent, NULL); - while ((pw = mutex_synchronize(setup_passwd_getpwent, Qnil)) != Qnil) { + WITHOUT_GVL(nogvl_setpwent, NULL); + while ((pw = setup_passwd_getpwent()) != Qnil) { rb_yield(pw); } return Qnil; @@ -421,7 +513,7 @@ etc_passwd(VALUE obj) if (rb_block_given_p()) { each_passwd(); } - return mutex_synchronize(setup_passwd_getpwent, Qnil); + return setup_passwd_getpwent(); #else return Qnil; #endif @@ -472,7 +564,7 @@ static VALUE etc_setpwent(VALUE obj) { #ifdef HAVE_GETPWENT - mEtc_mutex_synchronize(nogvl_setpwent, NULL); + WITHOUT_GVL(nogvl_setpwent, NULL); #endif return Qnil; } @@ -487,7 +579,7 @@ static VALUE etc_endpwent(VALUE obj) { #ifdef HAVE_GETPWENT - mEtc_mutex_synchronize(nogvl_endpwent, NULL); + WITHOUT_GVL(nogvl_endpwent, NULL); #endif return Qnil; } @@ -510,30 +602,61 @@ static VALUE etc_getpwent(VALUE obj) { #ifdef HAVE_GETPWENT - return mutex_synchronize(setup_passwd_getpwent, Qnil); + return setup_passwd_getpwent(); #else return Qnil; #endif } #ifdef HAVE_GETGRENT -static void * -nogvl_getgrgid(void *gid) -{ - return getgrgid((gid_t)(VALUE)gid); -} + +# if (defined(HAVE_GETGRNAM_R) || defined(HAVE_GETGRGID_R)) && defined(_SC_GETGR_R_SIZE_MAX) +# define GETGR_R_SIZE_INIT sysconf(_SC_GETGR_R_SIZE_MAX) +# define GETGR_R_SIZE_DEFAULT 0x1000 +# define GETGR_R_SIZE_LIMIT 0x10000 +# else +# define GETGR_R_SIZE_INIT -1 +# endif + +#if HAVE_GETGRGID_R +struct getgrgid_r_args { + gid_t gid; + char *buf; + size_t bufsize; + struct group *result; + struct group grp; +}; + +# define GETGRGID_R_ARGS(gid_, buf_, bufsize_) (struct getgrgid_r_args) \ + {.gid = gid_, .buf = buf_, .bufsize = bufsize_, .result = NULL} static void * -nogvl_getgrnam(void *name) +nogvl_getgrgid_r(void *args) { - return getgrnam((const char *)name); + struct getgrgid_r_args *arg = args; + return (void *)(VALUE)getgrgid_r(arg->gid, &arg->grp, arg->buf, arg->bufsize, &arg->result); } +#endif + +# ifdef HAVE_GETGRNAM_R +struct getgrnam_r_args { + const char *login; + char *buf; + size_t bufsize; + struct group *result; + struct group grp; +}; + +# define GETGRNAM_R_ARGS(login_, buf_, bufsize_) (struct getgrnam_r_args) \ + {.login = login_, .buf = buf_, .bufsize = bufsize_, .result = NULL} static void * -nogvl_getgrent(void *_) +nogvl_getgrnam_r(void *args) { - return getgrent(); + struct getgrnam_r_args *arg = args; + return (void *)(VALUE)getgrnam_r(arg->login, &arg->grp, arg->buf, arg->bufsize, &arg->result); } +# endif static void * nogvl_setgrent(void *_) @@ -571,28 +694,10 @@ setup_group(struct group *grp) } static VALUE -setup_group_getgrgid(VALUE gid) -{ - struct group *grp = rb_thread_call_without_gvl(nogvl_getgrgid, (void *)gid, RUBY_UBF_IO, 0); - if (grp == 0) rb_raise(rb_eArgError, "can't find group for %d", (int)gid); - return setup_group(grp); -} - -static VALUE -setup_group_getgrnam(VALUE nam) -{ - const char *p = StringValueCStr(nam); - struct group *grp = rb_thread_call_without_gvl(nogvl_getgrnam, (void *)p, RUBY_UBF_IO, 0); - RB_GC_GUARD(nam); - if (grp == 0) rb_raise(rb_eArgError, "can't find group for %"PRIsVALUE, nam); - return setup_group(grp); -} - -static VALUE -setup_group_getgrent(VALUE _) +setup_group_getgrent(void) { struct group *grp; - if ((grp = rb_thread_call_without_gvl(nogvl_getgrent, NULL, RUBY_UBF_IO, 0)) != 0) + if ((grp = getgrent()) != 0) return setup_group(grp); else return Qnil; @@ -628,7 +733,42 @@ etc_getgrgid(int argc, VALUE *argv, VALUE obj) else { gid = getgid(); } - return mutex_synchronize(setup_group_getgrgid, (VALUE)gid); + struct group *grp; +# ifdef HAVE_GETGRGID_R + char *getgr_buf; + long getgr_buf_len; + int e; + getgr_buf_len = GETGR_R_SIZE_INIT; + if (getgr_buf_len < 0) getgr_buf_len = GETGR_R_SIZE_DEFAULT; + VALUE getgr_tmp = rb_str_tmp_new(getgr_buf_len); + getgr_buf = RSTRING_PTR(getgr_tmp); + getgr_buf_len = rb_str_capacity(getgr_tmp); + rb_str_set_len(getgr_tmp, getgr_buf_len); + errno = 0; + struct getgrgid_r_args args = GETGRGID_R_ARGS(gid, getgr_buf, getgr_buf_len); + + while ((e = WITHOUT_GVL_INT(nogvl_getgrgid_r, &args)) != 0) { + if (e != ERANGE || args.bufsize >= GETGR_R_SIZE_LIMIT) { + rb_str_resize(getgr_tmp, 0); + rb_syserr_fail(e, "getgrnam_r"); + } + rb_str_modify_expand(getgr_tmp, args.bufsize); + args.buf = RSTRING_PTR(getgr_tmp); + args.bufsize = (size_t)rb_str_capacity(getgr_tmp); + } + grp = args.result; + if (grp == NULL) { + rb_str_resize(getgr_tmp, 0); + rb_raise(rb_eArgError, "can't find group for %d", (int)gid); + } + VALUE group = setup_group(args.result); + rb_str_resize(getgr_tmp, 0); + return group; +# else + grp = getgrgid(gid); + if (grp == NULL) rb_raise(rb_eArgError, "can't find group for %d", (int)gid); + return setup_group(grp); +# endif #else return Qnil; #endif @@ -654,7 +794,45 @@ static VALUE etc_getgrnam(VALUE obj, VALUE nam) { #ifdef HAVE_GETGRENT - return mutex_synchronize(setup_group_getgrnam, nam); + const char *grpname = StringValueCStr(nam); + struct group *grp; +# ifdef HAVE_GETGRNAM_R + char *getgr_buf; + long getgr_buf_len; + int e; + getgr_buf_len = GETGR_R_SIZE_INIT; + if (getgr_buf_len < 0) getgr_buf_len = GETGR_R_SIZE_DEFAULT; + VALUE getgr_tmp = rb_str_tmp_new(getgr_buf_len); + getgr_buf = RSTRING_PTR(getgr_tmp); + getgr_buf_len = rb_str_capacity(getgr_tmp); + rb_str_set_len(getgr_tmp, getgr_buf_len); + errno = 0; + struct getgrnam_r_args args = GETGRNAM_R_ARGS(grpname, getgr_buf, getgr_buf_len); + + while ((e = WITHOUT_GVL_INT(nogvl_getgrnam_r, &args)) != 0) { + if (e != ERANGE || args.bufsize >= GETGR_R_SIZE_LIMIT) { + rb_str_resize(getgr_tmp, 0); + rb_syserr_fail(e, "getgrnam_r"); + } + rb_str_modify_expand(getgr_tmp, args.bufsize); + args.buf = RSTRING_PTR(getgr_tmp); + args.bufsize = (size_t)rb_str_capacity(getgr_tmp); + } + grp = args.result; + if (grp == NULL) { + rb_str_resize(getgr_tmp, 0); + rb_raise(rb_eArgError, "can't find group for %"PRIsVALUE, nam); + } + VALUE group = setup_group(args.result); + rb_str_resize(getgr_tmp, 0); + RB_GC_GUARD(nam); + return group; +# else + grp = getgrnam(grpnam); + if (grp == NULL) rb_raise(rb_eArgError, "can't find group for %"PRIsVALUE, nam); + RB_GC_GUARD(nam); + return setup_group(grp); +# endif #else return Qnil; #endif @@ -665,7 +843,7 @@ static rb_atomic_t group_blocking; static VALUE group_ensure(VALUE _) { - mEtc_mutex_synchronize(nogvl_endgrent, NULL); + WITHOUT_GVL(nogvl_endgrent, NULL); if (RUBY_ATOMIC_EXCHANGE(group_blocking, 0) != 1) { rb_raise(rb_eRuntimeError, "unexpected group_blocking"); } @@ -677,8 +855,8 @@ group_iterate(VALUE _) { VALUE grp; - mEtc_mutex_synchronize(nogvl_setgrent, NULL); - while ((grp = mutex_synchronize(setup_group_getgrent, Qnil)) != Qnil) { + WITHOUT_GVL(nogvl_setgrent, NULL); + while ((grp = setup_group_getgrent()) != Qnil) { rb_yield(grp); } return Qnil; @@ -721,7 +899,7 @@ etc_group(VALUE obj) if (rb_block_given_p()) { each_group(); } - return mutex_synchronize(setup_group_getgrent, Qnil); + return setup_group_getgrent(); #else return Qnil; #endif @@ -770,7 +948,7 @@ static VALUE etc_setgrent(VALUE obj) { #ifdef HAVE_GETGRENT - mEtc_mutex_synchronize(nogvl_setgrent, NULL); + WITHOUT_GVL(nogvl_setgrent, NULL); #endif return Qnil; } @@ -785,7 +963,7 @@ static VALUE etc_endgrent(VALUE obj) { #ifdef HAVE_GETGRENT - mEtc_mutex_synchronize(nogvl_endgrent, NULL); + WITHOUT_GVL(nogvl_endgrent, NULL); #endif return Qnil; } @@ -807,7 +985,7 @@ static VALUE etc_getgrent(VALUE obj) { #ifdef HAVE_GETGRENT - return mutex_synchronize(setup_group_getgrent, Qnil); + return setup_group_getgrent(); #else return Qnil; #endif @@ -1285,9 +1463,6 @@ Init_etc(void) rb_define_const(mEtc, "VERSION", rb_str_new_cstr(RUBY_ETC_VERSION)); init_constants(mEtc); - mEtc_mutex = &mEtc_lock; - rb_nativethread_lock_initialize(mEtc_mutex); - rb_define_module_function(mEtc, "getlogin", etc_getlogin, 0); rb_define_module_function(mEtc, "getpwuid", etc_getpwuid, -1); diff --git a/ext/etc/extconf.rb b/ext/etc/extconf.rb index 2e28d58..23491bb 100644 --- a/ext/etc/extconf.rb +++ b/ext/etc/extconf.rb @@ -12,6 +12,10 @@ have_func("getlogin") have_func("getpwent") have_func("getgrent") +have_func("getpwuid_r") +have_func("getpwnam_r") +have_func("getgrgid_r") +have_func("getgrnam_r") if (sysconfdir = RbConfig::CONFIG["sysconfdir"] and !RbConfig.expand(sysconfdir.dup, "prefix"=>"", "DESTDIR"=>"").empty?) $defs.push("-DSYSCONFDIR=#{Shellwords.escape(sysconfdir.dump)}") From 79ea138c367c095e578b1c426d825e4f14118168 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 20 Nov 2024 15:06:39 -0800 Subject: [PATCH 7/8] Fix getpwnam_r size check Co-authored-by: Nobuyoshi Nakada --- ext/etc/etc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/etc/etc.c b/ext/etc/etc.c index c0103d8..e0acdb1 100644 --- a/ext/etc/etc.c +++ b/ext/etc/etc.c @@ -419,7 +419,7 @@ etc_getpwnam(VALUE obj, VALUE nam) return Qnil; } - if (enm != ERANGE || bufsizenm >= GETPW_R_SIZE_LIMIT) { + if (enm != ERANGE || args.bufsize >= GETPW_R_SIZE_LIMIT) { rb_str_resize(getpwnm_tmp, 0); rb_syserr_fail(enm, "getpwnam_r"); } From 7f26e35f7e8ed27d53c841e65a972226dd20a2e6 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 20 Nov 2024 15:28:31 -0800 Subject: [PATCH 8/8] Avoid declaration after statement to be compatible with C90, for Ruby 2.6 support --- ext/etc/etc.c | 62 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/ext/etc/etc.c b/ext/etc/etc.c index e0acdb1..d16aa57 100644 --- a/ext/etc/etc.c +++ b/ext/etc/etc.c @@ -313,6 +313,16 @@ etc_getpwuid(int argc, VALUE *argv, VALUE obj) #if defined(HAVE_GETPWENT) VALUE id; rb_uid_t uid; +# ifdef HAVE_GETPWUID_R + char *bufid; + long bufsizeid = GETPW_R_SIZE_INIT; /* maybe -1 */ + VALUE getpwid_tmp; + struct getpwuid_r_args args; + int eid; + VALUE result; +# else + struct passwd *pwd = getpwuid((uid_t)uid); +# endif if (rb_scan_args(argc, argv, "01", &id) == 1) { uid = NUM2UIDT(id); @@ -322,20 +332,16 @@ etc_getpwuid(int argc, VALUE *argv, VALUE obj) } # ifdef HAVE_GETPWUID_R - char *bufid; - long bufsizeid = GETPW_R_SIZE_INIT; /* maybe -1 */ - if (bufsizeid < 0) bufsizeid = GETPW_R_SIZE_DEFAULT; - VALUE getpwid_tmp = rb_str_tmp_new(bufsizeid); + getpwid_tmp = rb_str_tmp_new(bufsizeid); bufid = RSTRING_PTR(getpwid_tmp); bufsizeid = rb_str_capacity(getpwid_tmp); rb_str_set_len(getpwid_tmp, bufsizeid); - struct getpwuid_r_args args = GETPWUID_R_ARGS(uid, bufid, bufsizeid); + args = GETPWUID_R_ARGS(uid, bufid, bufsizeid); - int eid; errno = 0; while ((eid = WITHOUT_GVL_INT(nogvl_getpwuid_r, &args)) != 0) { @@ -362,11 +368,10 @@ etc_getpwuid(int argc, VALUE *argv, VALUE obj) } /* found it */ - VALUE result = setup_passwd(args.result); + result = setup_passwd(args.result); rb_str_resize(getpwid_tmp, 0); return result; # else - struct passwd *pwd = getpwuid((uid_t)uid); if (pwd == 0) rb_raise(rb_eArgError, "can't find user for %d", (int)uid); return setup_passwd(pwd); # endif @@ -398,18 +403,20 @@ etc_getpwnam(VALUE obj, VALUE nam) char *login = RSTRING_PTR(nam); char *bufnm; long bufsizenm = GETPW_R_SIZE_INIT; /* maybe -1 */ + VALUE getpwnm_tmp; + struct getpwnam_r_args args; + int enm; + VALUE result; if (bufsizenm < 0) bufsizenm = GETPW_R_SIZE_DEFAULT; - VALUE getpwnm_tmp = rb_str_tmp_new(bufsizenm); - + getpwnm_tmp = rb_str_tmp_new(bufsizenm); bufnm = RSTRING_PTR(getpwnm_tmp); bufsizenm = rb_str_capacity(getpwnm_tmp); rb_str_set_len(getpwnm_tmp, bufsizenm); - struct getpwnam_r_args args = GETPWNAM_R_ARGS(login, bufnm, bufsizenm); + args = GETPWNAM_R_ARGS(login, bufnm, bufsizenm); - int enm; errno = 0; while ((enm = WITHOUT_GVL_INT(nogvl_getpwnam_r, &args)) != 0) { @@ -436,7 +443,7 @@ etc_getpwnam(VALUE obj, VALUE nam) } /* found it */ - VALUE result = setup_passwd(args.result); + result = setup_passwd(args.result); rb_str_resize(getpwnm_tmp, 0); RB_GC_GUARD(nam); return result; @@ -726,6 +733,15 @@ etc_getgrgid(int argc, VALUE *argv, VALUE obj) #ifdef HAVE_GETGRENT VALUE id; gid_t gid; + struct group *grp; +# ifdef HAVE_GETGRGID_R + char *getgr_buf; + long getgr_buf_len; + int e; + VALUE getgr_tmp; + struct getgrgid_r_args args; + VALUE group; +# endif if (rb_scan_args(argc, argv, "01", &id) == 1) { gid = NUM2GIDT(id); @@ -733,19 +749,15 @@ etc_getgrgid(int argc, VALUE *argv, VALUE obj) else { gid = getgid(); } - struct group *grp; # ifdef HAVE_GETGRGID_R - char *getgr_buf; - long getgr_buf_len; - int e; getgr_buf_len = GETGR_R_SIZE_INIT; if (getgr_buf_len < 0) getgr_buf_len = GETGR_R_SIZE_DEFAULT; - VALUE getgr_tmp = rb_str_tmp_new(getgr_buf_len); + getgr_tmp = rb_str_tmp_new(getgr_buf_len); getgr_buf = RSTRING_PTR(getgr_tmp); getgr_buf_len = rb_str_capacity(getgr_tmp); rb_str_set_len(getgr_tmp, getgr_buf_len); errno = 0; - struct getgrgid_r_args args = GETGRGID_R_ARGS(gid, getgr_buf, getgr_buf_len); + args = GETGRGID_R_ARGS(gid, getgr_buf, getgr_buf_len); while ((e = WITHOUT_GVL_INT(nogvl_getgrgid_r, &args)) != 0) { if (e != ERANGE || args.bufsize >= GETGR_R_SIZE_LIMIT) { @@ -761,7 +773,7 @@ etc_getgrgid(int argc, VALUE *argv, VALUE obj) rb_str_resize(getgr_tmp, 0); rb_raise(rb_eArgError, "can't find group for %d", (int)gid); } - VALUE group = setup_group(args.result); + group = setup_group(args.result); rb_str_resize(getgr_tmp, 0); return group; # else @@ -800,14 +812,18 @@ etc_getgrnam(VALUE obj, VALUE nam) char *getgr_buf; long getgr_buf_len; int e; + VALUE getgr_tmp; + struct getgrnam_r_args args; + VALUE group; + getgr_buf_len = GETGR_R_SIZE_INIT; if (getgr_buf_len < 0) getgr_buf_len = GETGR_R_SIZE_DEFAULT; - VALUE getgr_tmp = rb_str_tmp_new(getgr_buf_len); + getgr_tmp = rb_str_tmp_new(getgr_buf_len); getgr_buf = RSTRING_PTR(getgr_tmp); getgr_buf_len = rb_str_capacity(getgr_tmp); rb_str_set_len(getgr_tmp, getgr_buf_len); errno = 0; - struct getgrnam_r_args args = GETGRNAM_R_ARGS(grpname, getgr_buf, getgr_buf_len); + args = GETGRNAM_R_ARGS(grpname, getgr_buf, getgr_buf_len); while ((e = WITHOUT_GVL_INT(nogvl_getgrnam_r, &args)) != 0) { if (e != ERANGE || args.bufsize >= GETGR_R_SIZE_LIMIT) { @@ -823,7 +839,7 @@ etc_getgrnam(VALUE obj, VALUE nam) rb_str_resize(getgr_tmp, 0); rb_raise(rb_eArgError, "can't find group for %"PRIsVALUE, nam); } - VALUE group = setup_group(args.result); + group = setup_group(args.result); rb_str_resize(getgr_tmp, 0); RB_GC_GUARD(nam); return group;