Skip to content
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

OOM looking up for users/groups with libnss_systemd.so #10

Open
tobald opened this issue Nov 1, 2023 · 12 comments
Open

OOM looking up for users/groups with libnss_systemd.so #10

tobald opened this issue Nov 1, 2023 · 12 comments

Comments

@tobald
Copy link
Contributor

tobald commented Nov 1, 2023

Hash comments are not allowed in libnss definition files (passwd/group/shadow). Alternc-nss' current implementation wrongly adds hash comments in those files. We spotted the issue after migrating a server (16Go memory) to debian bullseye ,we then got multiple applications reaching memory limits while several gigs of free memory were available, for instance :

$ addgroup test_xxx
Out of memory!

Attached is a system call log which allowed us to pinpoint the issue and a naïve patch.

Note that other bullseye machines with more memory (+32Go) do not reach to OOM error while the bug is present, I suspect the libnss-extrausers definitions are ignored in that case.

adduser-strace.log
0001-prevent-user-group-lookups-errors-with-libnss_system.patch.log

@camlafit
Copy link
Contributor

camlafit commented Nov 2, 2023

Hello

Very to have found a solution. We have noticed this behavior without time to investigate.

Looking about nss information, a workaround could be to add a space before #
Could you tried it ?

As we can't know if extrauser is used to another usecase, we need to identify specific AlternC block code.

@camlafit
Copy link
Contributor

camlafit commented Nov 2, 2023

I've checked about spacing workaround following musl or libc behavior could be different.

In both case pwck doesn't accept these lines.
I've opened an branch to solve this issus and backported your patch as starting point

@camlafit
Copy link
Contributor

camlafit commented Nov 2, 2023

Hello

I've a PR #11 with a less naive approach.
Before to write any AlternC related value with remove old previous line then we can preserve any other row and append only AlternC specific content.

@tobald
Copy link
Contributor Author

tobald commented Nov 3, 2023

thanks for the quick response, there is one issue left : the last line of passwd/group/shadow is invalid because it is not terminated/misses an end-of-line.

@tobald
Copy link
Contributor Author

tobald commented Nov 3, 2023

line 98, write_content is dubious, won't work for passwd or shadow :

    $content_bck = implode("\n", $this->group);

@camlafit
Copy link
Contributor

camlafit commented Nov 3, 2023

there is one issue left : the last line of passwd/group/shadow is invalid because it is not terminated/misses an end-of-line.

I've tried after purge backup and extrauser files and pwck don't raise any error.
Do you have again this problem ? We can add a security with terminate return carrier

@tobald
Copy link
Contributor Author

tobald commented Nov 3, 2023

Do you have again this problem ?

Thanks, without line termination borgbackup raises MemoryError.

@camlafit
Copy link
Contributor

camlafit commented Nov 3, 2023

Do you have again this problem ?

Thanks, without line termination borgbackup raises MemoryError.

Looks strange. I don't see reason why any backup could be raise any error about termination file. In any case return carrier is now forced :)

@tobald
Copy link
Contributor Author

tobald commented Nov 3, 2023

borgbackup looks for permissions, the posix standard definition of a line https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206

@camlafit
Copy link
Contributor

camlafit commented Nov 3, 2023

A new day to learn new thing :)
I thinks it's solved by 993ecfb

@tobald
Copy link
Contributor Author

tobald commented Nov 4, 2023

I fear your write_content() could lead to corruption, if manual changes occurs duplicate entries would then be inserted :

       if (file_exists($file_bck)) {
           $content_lines_bck = file($file_bck, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES);
           $content_lines = array_diff($content_lines, $content_lines_bck);
       }
       $content_lines = array_merge($content_lines, $content_new);

I think we should keep it simple and short and not try to manage things, document that when you install alternc-nss extrausers files are overwritten.

(sidenote there are validation tools like pwck)

@camlafit
Copy link
Contributor

camlafit commented Nov 6, 2023

Hello

I don't want presume any use case. Then we need to check file and manage only AlternC part.
We must be less intrusive as possible.

array_diff is here to exclude AlternC part following previous backup before to update it.
We can add a flock check to prevent simultaneous update. But in this case I don't think is useful.

In my mind/mantra we must be kiss indeed and also to be less intrusive :)

Mazamazine added a commit to Koumbit/alternc-nss that referenced this issue Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants