kohsuke / libpam4j

libpam4j
http://libpam4j.kohsuke.org/
MIT License
44 stars 47 forks source link

Inconsistent "getGroups()" and garbage getShell() results under load #9

Closed jamshid closed 6 years ago

jamshid commented 10 years ago

When I run the test program https://gist.github.com/jamshid/8776435 (written for another bug) I get inconsistent results from getGroups() and sometimes some corruption in the getShell() result. Tested on CentOS 6.5 and Ubuntu 13.

I added the user gw-john with:

# groupadd gw-admin
# useradd gw-john --groups gw-admin -M
# echo gw-john:password | chpasswd

The program starts 1000 threads doing PAM.authenticate() and outputs the user's info, but many of the results do not list the gw-admin group and there's even some garbage (seen on Ubuntu 13). Btw I usually get the inconsistent groups even with 10 threads.

Any ideas? I guess I should try a non-java version to see if it's reproducible.

$ sudo java -classpath ./jna-3.5.2.jar:./libpam4j-1.7.jar:. PamTest 1000 | grep User  | sort | uniq
User gw-john 1001 1004 /home/gw-john  /bin/sh in [gw-admin]
User gw-john 1001 1004 /home/gw-john  /bin/sh in [gw-admin, gw-john]
User gw-john 1001 1004 /home/gw-john  /bin/sh in [gw-john]
User gw-john 1001 1004 /home/gw-john  /bin/sh in [operatorx, gw-john]
User gw-john 1001 1004 /home/gw-john  /dbus:/bise/shvar/lib/gnats/bin/sh in [gw-admin, gw-john]
jamshid commented 9 years ago

Can still reproduce this bug, tried using latest libpam4j-1.8.jar and jna-4.1.0.jar. I notice CLibrary is using the non-threadsafe getpwnam instead of getpwnam_r (http://man7.org/linux/man-pages/man3/getpwnam.3.html), so I guess that's why it fails with multiple threads? Is that intentional?

https://github.com/kohsuke/libpam4j/blob/master/src/main/java/org/jvnet/libpam/impl/CLibrary.java

        public static passwd loadPasswd(String userName) throws PAMException {
            passwd pwd = libc.getpwnam(userName);

Btw I know the PAM class says "Instances are thread unsafe and non reentrant." but I'm not reusing an instance of PAM, I'm creating an independent one in each thread. I guess it needs to be a singleton with synchronized methods.

matthiasblaesing commented 8 years ago

I pushed a potential fix into a branch, that should fix this. I'm not sure whether my assumptions with regard to the memory sizes are sound, but it should work. The testsuite completes.

jamshid commented 6 years ago

Thank you @kohsuke and @matthiasblaesing!

This will be released to maven repos as org.kohsuke libpam4j 1.10?