On Čt, 2014-09-18 at 02:17 +0400, Dmitry V. Levin wrote: > On Fri, Sep 05, 2014 at 07:24:31AM +0000, Tomáš Mráz wrote: > [...] > > I've discovered an inconsistency in the way how grantor is initialized: > > > --- a/libpam/pam_dispatch.c > > +++ b/libpam/pam_dispatch.c > > @@ -217,8 +217,14 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, > > status = retval; > > } > > } > > - if ( impression == _PAM_POSITIVE && action == _PAM_ACTION_DONE ) { > > - goto decision_made; > > + if ( impression == _PAM_POSITIVE ) { > > + if ( retval == PAM_SUCCESS ) { > > + h->grantor = 1; > > + } > > + > > + if ( action == _PAM_ACTION_DONE ) { > > + goto decision_made; > > + } > > } > > break; > > > > Here grantor is being set every time retval is PAM_SUCCESS and > impression is _PAM_POSITIVE, ... > > > @@ -262,6 +268,9 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, > > || (impression == _PAM_POSITIVE > > && status == PAM_SUCCESS) ) { > > if ( retval != PAM_IGNORE || cached_retval == retval ) { > > + if ( impression == _PAM_UNDEF && retval == PAM_SUCCESS ) { > > + h->grantor = 1; > > + } > > impression = _PAM_POSITIVE; > > status = retval; > > while here grantor is set only if retval is PAM_SUCCESS and > impression is not yet _PAM_POSITIVE, so if impression is already > _PAM_POSITIVE, grantor will not be set. I did that on purpose. In this case (jump in the second chain), the module is really grantor only when the impression is not set yet. In other cases it is just a jump and impression is kept as _PAM_POSITIVE.

On Čt, 2014-09-18 at 13:02 +0400, Dmitry V. Levin wrote: > On Thu, Sep 18, 2014 at 10:44:57AM +0200, Tomas Mraz wrote: > > On Čt, 2014-09-18 at 02:17 +0400, Dmitry V. Levin wrote: > > > On Fri, Sep 05, 2014 at 07:24:31AM +0000, Tomáš Mráz wrote: > > > [...] > > > > > > I've discovered an inconsistency in the way how grantor is initialized: > > > > > > > --- a/libpam/pam_dispatch.c > > > > +++ b/libpam/pam_dispatch.c > > > > @@ -217,8 +217,14 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, > > > > status = retval; > > > > } > > > > } > > > > - if ( impression == _PAM_POSITIVE && action == _PAM_ACTION_DONE ) { > > > > - goto decision_made; > > > > + if ( impression == _PAM_POSITIVE ) { > > > > + if ( retval == PAM_SUCCESS ) { > > > > + h->grantor = 1; > > > > + } > > > > + > > > > + if ( action == _PAM_ACTION_DONE ) { > > > > + goto decision_made; > > > > + } > > > > } > > > > break; > > > > > > > > > > Here grantor is being set every time retval is PAM_SUCCESS and > > > impression is _PAM_POSITIVE, ... > > > > > > > @@ -262,6 +268,9 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, > > > > || (impression == _PAM_POSITIVE > > > > && status == PAM_SUCCESS) ) { > > > > if ( retval != PAM_IGNORE || cached_retval == retval ) { > > > > + if ( impression == _PAM_UNDEF && retval == PAM_SUCCESS ) { > > > > + h->grantor = 1; > > > > + } > > > > impression = _PAM_POSITIVE; > > > > status = retval; > > > > > > while here grantor is set only if retval is PAM_SUCCESS and > > > impression is not yet _PAM_POSITIVE, so if impression is already > > > _PAM_POSITIVE, grantor will not be set. > > > > I did that on purpose. In this case (jump in the second chain), the > > module is really grantor only when the impression is not set yet. In > > other cases it is just a jump and impression is kept as _PAM_POSITIVE. > > Sorry, but "impression = _PAM_POSITIVE" and "status = retval" > assignments are present both in case of a jump and in case of > _PAM_ACTION_OK/_PAM_ACTION_DONE, and they mean exactly the same. > > Taking this into consideration, why do you suppose that a module is not > a "really grantor" in case of a jump but still a "really grantor" in case > of _PAM_ACTION_OK and _PAM_ACTION_DONE? Hmm, I really wonder whether the bug is not actually in the fact that for the cached chain the status=retval assignment is done for jump even if the impression is already _PAM_POSITIVE. This actually does not look right to me, because it means that in the cached chain the jumps affect the return value of the libpam call much more than for the initial chain. This behavior is in libpam for a long time - since version 0.75. But that does not mean that it is completely right. Even in the commit message I do not see any argument why jumps in cached chain should behave this way. I am not sure whether Andrew G. Morgan can still be reached and would be able to tell us what was the reason for this particular handling of jumps in the cached chain. -- Tomas Mraz No matter how far down the wrong road you've gone, turn back. Turkish proverb (You'll never know whether the road is wrong though.)