GNOME Bugzilla – Bug 400579
gnome-screensaver fails to compile against openpam
Last modified: 2007-02-22 02:51:35 UTC
gs-auth-pam.c: In function ‘pam_conversation’: gs-auth-pam.c:229: error: ‘PAM_INCOMPLETE’ undeclared (first use in this function) gs-auth-pam.c:229: error: (Each undeclared identifier is reported only once gs-auth-pam.c:229: error: for each function it appears in.) gmake[4]: *** [gs-auth-pam.o] Error 1 Patch to follow
Created attachment 81172 [details] [review] Use PAM_TRY_AGAIN Tested against LinuxPAM as well and found to be working.
This patch has been added to Gentoo. Seems to work fine.
Roy, I've noticed you've been taking a lot of patches from the FreeBSD ports tree and submitting them upstream. However, you fail to mention that these patches were actually written by someone else. I don't mind you submitting patches upstream, but please show us the respect of saying that they were obtained from the FreeBSD ports tree, or produced by the FreeBSD GNOME Team. The patch mentioned here was actually committed to our tree on October 14, 2006 as seen here http://www.freebsd.org/cgi/cvsweb.cgi/ports/x11/gnome-screensaver/files/patch-src_gs-auth-pam.c . I know this is the same patch because everything is absolutely the same down to the seconds on the timestamps (as seen at http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/x11/gnome-screensaver/files/patch-src_gs-auth-pam.c?rev=1.1&content-type=text/plain).
(In reply to comment #3) > Roy, I've noticed you've been taking a lot of patches from the FreeBSD ports > tree and submitting them upstream. However, you fail to mention that these > patches were actually written by someone else. Yes, you're right. This patch was taken directly from your ports tree. I apologize for not stating this here. However, I did state this for the libgtop patches on bug #387200. I think those are the only two bugs here where I've taken patches from FreeBSD ports. Other patches I have made myself.
Joe has a valid point - this is at least very uncool. Ok well it looks like this symbol is available with Linux-PAM too but I'm having trouble finding a good reference for the exact differences between the two. Apparently documenting PAM_INCOMPLETE is on the Linux-PAM TODO. However, it seems like they are certainly not interchangable on Linux. There are a number of specific checks for PAM_INCOMPLETE etc. Perhaps you can do something like: #ifndef PAM_INCOMPLETE #define PAM_INCOMPLETE PAM_TRY_AGAIN #endif Ray, do you have any insight here?
(In reply to comment #5) > Joe has a valid point - this is at least very uncool. I've already apologized for not giving due credit for a one line patch. > Ray, do you have any insight here? I've queried our resident PAM specialist and hopefully he'll be able to tell me. A quick Google shows that PAM_INCOMPLETE appears to be Linux specific and a fair few applications do use the above mapping - again, these applications seem to be Linux based and then moving out. This is from Linux PAM _pam_types.h +#define PAM_TRY_AGAIN 24 /* Preliminary check by password service */ +#define PAM_INCOMPLETE 31 /* please call this function again to + complete authentication stack. Before + calling again, verify that conversation + is completed */ And this from http://metric.it.uab.edu/doc/linux/pam-0.75/html/pam_modules-2.html PAM_CONV_AGAIN - converstation did not complete and the caller is required to return control to the application, until such time as the application has completed the conversation process. A module calling pam_get_user() that obtains this return code, should return PAM_INCOMPLETE and be prepared (when invoked the next time) to recall pam_get_user() to fill in the user's name, and then pick up where it left off as if nothing had happened. This procedure is needed to support an event-driven application programming model. Which implies that PAM_TRY_AGAIN is used for a failure and PAM_INCOMPLETE implies that something is missing or failed. This suggests that in this case, PAM_TRY_AGAIN should be preferred. I'll post back when I find out if this indeed the case from someone who knows more about PAM than me.
So judging by the quoted bits in comment 6, PAM_INCOMPLETE is only for pam modules to use, not for apps to use. I don't think PAM_TRY_AGAIN is right either. I *think* the only error codes the conversation function should use are PAM_SUCCESS and the onces prefixed with PAM_CONV_ I think that gnome-screensaver is doing the wrong thing. On failure it's currently: 1) s I think what it should be doing on failure is: 1) looping through the already filled out replies and freeing them one by one. 2) setting *resp to NULL 3) returning PAM_CONV_ERROR That's what gdm and the basic conversation that ships in libpam_misc do anyway. Tomas, is that right?
oops, I didn't complete the "what it's currently" outline. On failure it's currently, 1) setting pam_retcode to PAM_INCOMPLETE 2) continuing on the next reply 3) setting *resp to the reply block 4) returning PAM_SUCCESS
The problem with PAM_INCOMPLETE/PAM_CONV_AGAIN is that not many modules (if any at all) respond correctly to the PAM_CONV_AGAIN error value. They should return PAM_INCOMPLETE to application but they will most probably return PAM_CONV_ERR or PAM_AUTH_ERR. So implementing the suggestion above is perhaps the safest thing.
Created attachment 83082 [details] [review] free up finished replies and exit with conversation error So I think something like the above patch should work, although I haven't tried compiling it or testing it.
Created attachment 83083 [details] [review] free up finished replies and exit with conversation error and of course, immediately after posting the last attachment and looking at it, I noticed that I used the wrong index variable in the loop that goes through and cleans things up.
Yes that looks good and from some testing seems to work. I've committed this to 2.16 and trunk with a change to use a less error prone index variable ;) Thanks a lot Ray.