Skip to content

NSFS | FS Napi | getpwnam concurrency issue #8982

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

Merged
merged 1 commit into from
Apr 24, 2025

Conversation

romayalon
Copy link
Contributor

@romayalon romayalon commented Apr 22, 2025

Describe the Problem

While debugging https://issues.redhat.com/browse/DFBUGS-2227, it was found that getpwnam() sometimes return incorrect uid/gid per the username provided.
After checking the getpwnam https://man7.org/linux/man-pages/man3/getpwnam.3.html man page, I saw that in fs_napi we use an unsafe multi threaded operation getpwnam() instead of the safe operation getpwnam_r().
image

Explain the Changes

  1. Changed fs_napi GetPwName() to use getpwnam_r() instead of getpwnam().
  2. Added concurrency getPwName() tests.

Issues: Fixed #xxx / Gap #xxx

  1. https://issues.redhat.com/browse/DFBUGS-2227

Testing Instructions:

  1. Auto tests - sudo jest --testRegex=jest_tests/test_fs_napi_concurrency.test.js
  2. Manual tests for reproducing the original bug -
  3. install warp
  4. create at least 2 file system users, create 2 noobaa accounts, configured with distinguished names and create bucket for each account.
    you can use the following script -
storage_root_path="/nsfs_storage" ;
mkdir $storage_root_path/ -m 777;
for i in {1..2}; do 
groupadd -g 700$i user-$i ; 
useradd -c user-$i -m user-$i -p $(openssl passwd -1 pass-$i) -u 700$i -g 700$i ; 
mkdir $storage_root_path/user-$i-buckets/ -m 770; 
chown user-$i:user-$i $storage_root_path/user-$i-buckets/ ;  
noobaa-cli account add --name user-$i --new_buckets_path=$storage_root_path/user-$i-buckets/ --user=user-$i ;
for j in {1..1} ; do 
    mkdir $storage_root_path/user-$i-buckets/user-$i-bucket-$j/ -m 770 ;
    chown user-$i:user-$i $storage_root_path/user-$i-buckets/user-$i-bucket-$j/ ; 
    noobaa-cli bucket add --name=user.$i.bucket.$j --path=$storage_root_path/user-$i-buckets/user-$i-bucket-$j/ --owner=user-$i ; 
    done
done
  1. Open 2 tabs, and run from each the following warp command with different accounts -
  2. tab 1 -
warp put --host=localhost:6443 --access-key=<access_key of account 1> --secret-key=<secret_key of account 1>--obj.size=1k --duration=1m --disable-multipart  --bucket=<bucket of account 1> --tls --insecure --noclear --concurrent 1
  1. Tab 2 -
warp put --host=localhost:6443 --access-key=<access_key of account 2> --secret-key=<secret_key of account 2>--obj.size=1k --duration=1m --disable-multipart  --bucket=<bucket of account 2> --tls --insecure --noclear --concurrent 1

If the bug reproduced you will see AccessDenied error and the following logs -

Apr 22 13:53:41 ba2710905f48 node[5245]: Apr-22 13:53:41.544 [nsfs/5245]  [WARN] core.sdk.namespace_fs:: _load_bucket failed, on bucket
_path /nsfs_storage/user-1-buckets/user-1-bucket-1 got error [Error: Permission denied] { code: 'EACCES', context: 'Stat _path=/nsfs_storage/user-1-buckets/user-1-bucket-1 ' }
Apr 22 13:53:41 ba2710905f48 node[5245]: Apr-22 13:53:41.549 [nsfs/5245] [ERROR] core.endpoint.s3.s3_rest:: S3 ERROR <?xml version="1.0" encoding="UTF-8"?><Error><Code>AccessDenied</Code><Message>Access Denied</Message><Resource>/user.1.bucket.1/8gKQygb%28/25.eDq7H8kg5i4n1Hag.rnd</Resource><RequestId>m9skfje4-c1hdxw-dte</RequestId></Error> PUT /user.1.bucket.1/8gKQygb%28/25.eDq7H8kg5i4n1Hag.rnd {"host":"localhost:6443","user-agent":"MinIO (linux; amd64) minio-go/v7.0.90 warp/1.1.2","content-length":"1000","authorization":"AWS4-HMAC-SHA256 Credential=<access_key>/20250422/us-east-1/s3/aws4_request, SignedHeaders=content-type;host;x-amz-content-sha256;x-amz-date, Signature=3205a9a0616d7e1dcfdf369e595e7fea686882f85f4b8d1c142086cf3e3f336f","content-type":"application/octet-stream","x-amz-content-sha256":"UNSIGNED-PAYLOAD","x-amz-date":"20250422T135340Z"} Error: Permission denied - context: Stat _path=/nsfs_storage/user-1-buckets/user-1-bucket-1

and especially important, you will see a log message saying getpwnam returned wrong uid/gid -

Apr 22 13:53:41 ba2710905f48 node[5245]: 2025-04-22 13:53:41.507139 [PID-5245/TID-5245] [L1] FS::GetPwName::OnOK: _user=user-1 _getpwna
m_res->pw_uid=7002 _getpwnam_res->pw_gid=7002

while in passwd file -

user-1:x:7001:7001:user-1:/home/user-1:/bin/bash
user-2:x:7002:7002:user-2:/home/user-2:/bin/bash
  • Doc added/updated
  • Tests added

@romayalon romayalon changed the title NSFS | FS Napi getpwnam concurrency issue NSFS | FS Napi | getpwnam concurrency issue Apr 22, 2025
@romayalon romayalon force-pushed the romy-getpwnam-concurrency-issue branch from 728e86e to fb1433f Compare April 23, 2025 08:56
@romayalon romayalon requested a review from guymguym April 23, 2025 08:58
Copy link
Contributor

@nadavMiz nadavMiz left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments

@romayalon romayalon force-pushed the romy-getpwnam-concurrency-issue branch 3 times, most recently from dc26aeb to fd102d4 Compare April 24, 2025 06:20
Copy link
Member

@guymguym guymguym left a comment

Choose a reason for hiding this comment

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

I suggested a minor change which is to allocate the memory in Work (which runs in worker thread) instead in ctor (which runs in main thread). Looks good.

@romayalon romayalon force-pushed the romy-getpwnam-concurrency-issue branch from fd102d4 to f225557 Compare April 24, 2025 08:39
@romayalon romayalon merged commit 90f5dc7 into noobaa:master Apr 24, 2025
11 checks passed
@romayalon romayalon mentioned this pull request Apr 24, 2025
2 tasks
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.

3 participants