Skip to content

NC | supplemental groups - getpwuid concurrency issue #9010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nadavMiz
Copy link
Contributor

@nadavMiz nadavMiz commented May 5, 2025

Describe the Problem

as noted in #8982 : getpwuid function is not thread safe. so need to replace with thread safe getpwuid_r

Explain the Changes

  1. create a static variable for password buffer size in ThreadScope scope. initialize it from buffer length we got in fs_napi CTOR
  2. replace getpwuid in both dynamic supplemental group usage for both linux and darwin.

Issues: Fixed #8985

GAP - need to test on macos environment

Testing Instructions:

  1. for general supplemental groups tests run:
    sudo NC_CORETEST=true node ./node_modules/mocha/bin/mocha src/test/unit_tests/test_nsfs_access.js
  • Doc added/updated
  • Tests added

@nadavMiz nadavMiz requested review from guymguym and romayalon May 5, 2025 11:13
@nadavMiz nadavMiz self-assigned this May 5, 2025
@nadavMiz nadavMiz force-pushed the supplemental_groups_concurrency branch from 7d93074 to 48e9f77 Compare May 7, 2025 08:45
@nadavMiz nadavMiz force-pushed the supplemental_groups_concurrency branch from 48e9f77 to bbb348c Compare May 7, 2025 08:47
@nadavMiz nadavMiz requested a review from guymguym May 7, 2025 10:55
@@ -41,6 +41,16 @@ class ThreadScope
change_user();
}

static void set_passwd_buf_size(int buf_size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static void set_passwd_buf_size(int buf_size)
static void set_passwd_buf_size(long buf_size)

@@ -2419,7 +2419,7 @@ fs_napi(Napi::Env env, Napi::Object exports)
if (passwd_bufsize == -1) {
passwd_bufsize = 16384;
}
PASSWD_BUF_SIZE = passwd_bufsize;
ThreadScope::set_passwd_buf_size(passwd_bufsize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should move lines 2418 - 2421 inside set_passwd_buf_size()
although I must say it's a bit weird to set this static size per thread

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supplemental groups thread safety issue + dynamic supplemental groups PANIC in a container
3 participants