8:03 a.m.
If the mmap cache cannot be initialized (such as insufficient
permissions or SELinux/AppArmor denial), we are supposed to fall back to
our 1.8 behavior of LDB cache only. However, we weren't properly
checking that the cache had been set up and we were always attempting to
dereference the mmap context in fill_pwent() and fill_grent().
Fixes https://fedorahosted.org/sssd/ticket/1346
9:52 a.m.
On Wed, May 23, 2012 at 09:03:16AM -0400, Stephen Gallagher wrote:
If the mmap cache cannot be initialized (such as insufficient
permissions or SELinux/AppArmor denial), we are supposed to fall back to
our 1.8 behavior of LDB cache only. However, we weren't properly
checking that the cache had been set up and we were always attempting to
dereference the mmap context in fill_pwent() and fill_grent().
Fixes https://fedorahosted.org/sssd/ticket/1346
I think we should do the same for sss_mmap_cache_gr_store, too. And
perhaps do the check inside the functions themselves and not the callers
so that we don't forget to add the check when we add a new call of these
functions.
12:42 p.m.
On Wed, 2012-05-23 at 16:52 +0200, Jakub Hrozek wrote:
On Wed, May 23, 2012 at 09:03:16AM -0400, Stephen Gallagher wrote:
> If the mmap cache cannot be initialized (such as insufficient
> permissions or SELinux/AppArmor denial), we are supposed to fall back to
> our 1.8 behavior of LDB cache only. However, we weren't properly
> checking that the cache had been set up and we were always attempting to
> dereference the mmap context in fill_pwent() and fill_grent().
>
> Fixes https://fedorahosted.org/sssd/ticket/1346
I think we should do the same for sss_mmap_cache_gr_store, too. And
perhaps do the check inside the functions themselves and not the callers
so that we don't forget to add the check when we add a new call of these
functions.
How do you think we should handle the return code if we add it to the
functions themselves? EOK would seem to be hiding things, but ENOMEM
(insufficient space in the cache) seems a little hackish.
3:47 a.m.
On Wed, May 23, 2012 at 01:42:58PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-23 at 16:52 +0200, Jakub Hrozek wrote:
> On Wed, May 23, 2012 at 09:03:16AM -0400, Stephen Gallagher wrote:
> > If the mmap cache cannot be initialized (such as insufficient
> > permissions or SELinux/AppArmor denial), we are supposed to fall back to
> > our 1.8 behavior of LDB cache only. However, we weren't properly
> > checking that the cache had been set up and we were always attempting to
> > dereference the mmap context in fill_pwent() and fill_grent().
> >
> > Fixes https://fedorahosted.org/sssd/ticket/1346
>
> I think we should do the same for sss_mmap_cache_gr_store, too. And
> perhaps do the check inside the functions themselves and not the callers
> so that we don't forget to add the check when we add a new call of these
> functions.
How do you think we should handle the return code if we add it to the
functions themselves? EOK would seem to be hiding things, but ENOMEM
(insufficient space in the cache) seems a little hackish.
I was thinking logging a MINOR_FAILURE and returning EOK.
But checking if the context exists in the caller is fine, too, we just
need to do it for both _pw_store and _gr_store.
6:17 a.m.
On Thu, 2012-05-24 at 10:47 +0200, Jakub Hrozek wrote:
On Wed, May 23, 2012 at 01:42:58PM -0400, Stephen Gallagher wrote:
> On Wed, 2012-05-23 at 16:52 +0200, Jakub Hrozek wrote:
> > On Wed, May 23, 2012 at 09:03:16AM -0400, Stephen Gallagher wrote:
> > > If the mmap cache cannot be initialized (such as insufficient
> > > permissions or SELinux/AppArmor denial), we are supposed to fall back to
> > > our 1.8 behavior of LDB cache only. However, we weren't properly
> > > checking that the cache had been set up and we were always attempting to
> > > dereference the mmap context in fill_pwent() and fill_grent().
> > >
> > > Fixes https://fedorahosted.org/sssd/ticket/1346
> >
> > I think we should do the same for sss_mmap_cache_gr_store, too. And
> > perhaps do the check inside the functions themselves and not the callers
> > so that we don't forget to add the check when we add a new call of these
> > functions.
>
> How do you think we should handle the return code if we add it to the
> functions themselves? EOK would seem to be hiding things, but ENOMEM
> (insufficient space in the cache) seems a little hackish.
I was thinking logging a MINOR_FAILURE and returning EOK.
Logging a minor failure would be much too noisy. We'd be polluting the
log with an error they may not be able to fix.
But checking if the context exists in the caller is fine, too, we
just
need to do it for both _pw_store and _gr_store.
Can you point out where I missed one? My patch fixes both fill_pwent()
and fill_grent(). Grepping through the sources shows me only those two
locations using sss_mmap_cache_*_store().
6:44 a.m.
On Thu, May 24, 2012 at 07:17:31AM -0400, Stephen Gallagher wrote:
On Thu, 2012-05-24 at 10:47 +0200, Jakub Hrozek wrote:
> On Wed, May 23, 2012 at 01:42:58PM -0400, Stephen Gallagher wrote:
> > On Wed, 2012-05-23 at 16:52 +0200, Jakub Hrozek wrote:
> > > On Wed, May 23, 2012 at 09:03:16AM -0400, Stephen Gallagher wrote:
> > > > If the mmap cache cannot be initialized (such as insufficient
> > > > permissions or SELinux/AppArmor denial), we are supposed to fall back
to
> > > > our 1.8 behavior of LDB cache only. However, we weren't properly
> > > > checking that the cache had been set up and we were always attempting
to
> > > > dereference the mmap context in fill_pwent() and fill_grent().
> > > >
> > > > Fixes https://fedorahosted.org/sssd/ticket/1346
> > >
> > > I think we should do the same for sss_mmap_cache_gr_store, too. And
> > > perhaps do the check inside the functions themselves and not the callers
> > > so that we don't forget to add the check when we add a new call of
these
> > > functions.
> >
> > How do you think we should handle the return code if we add it to the
> > functions themselves? EOK would seem to be hiding things, but ENOMEM
> > (insufficient space in the cache) seems a little hackish.
>
> I was thinking logging a MINOR_FAILURE and returning EOK.
>
Logging a minor failure would be much too noisy. We'd be polluting the
log with an error they may not be able to fix.
> But checking if the context exists in the caller is fine, too, we just
> need to do it for both _pw_store and _gr_store.
Can you point out where I missed one? My patch fixes both fill_pwent()
and fill_grent(). Grepping through the sources shows me only those two
locations using sss_mmap_cache_*_store().
You're right, you fixed both. I don't know why I didn't see that. Sorry
for the noise.
Ack.
7:32 a.m.
On Thu, 2012-05-24 at 13:44 +0200, Jakub Hrozek wrote:
On Thu, May 24, 2012 at 07:17:31AM -0400, Stephen Gallagher wrote:
> On Thu, 2012-05-24 at 10:47 +0200, Jakub Hrozek wrote:
> > On Wed, May 23, 2012 at 01:42:58PM -0400, Stephen Gallagher wrote:
> > > On Wed, 2012-05-23 at 16:52 +0200, Jakub Hrozek wrote:
> > > > On Wed, May 23, 2012 at 09:03:16AM -0400, Stephen Gallagher wrote:
> > > > > If the mmap cache cannot be initialized (such as insufficient
> > > > > permissions or SELinux/AppArmor denial), we are supposed to fall
back to
> > > > > our 1.8 behavior of LDB cache only. However, we weren't
properly
> > > > > checking that the cache had been set up and we were always
attempting to
> > > > > dereference the mmap context in fill_pwent() and fill_grent().
> > > > >
> > > > > Fixes https://fedorahosted.org/sssd/ticket/1346
> > > >
> > > > I think we should do the same for sss_mmap_cache_gr_store, too. And
> > > > perhaps do the check inside the functions themselves and not the
callers
> > > > so that we don't forget to add the check when we add a new call
of these
> > > > functions.
> > >
> > > How do you think we should handle the return code if we add it to the
> > > functions themselves? EOK would seem to be hiding things, but ENOMEM
> > > (insufficient space in the cache) seems a little hackish.
> >
> > I was thinking logging a MINOR_FAILURE and returning EOK.
> >
>
> Logging a minor failure would be much too noisy. We'd be polluting the
> log with an error they may not be able to fix.
>
> > But checking if the context exists in the caller is fine, too, we just
> > need to do it for both _pw_store and _gr_store.
>
> Can you point out where I missed one? My patch fixes both fill_pwent()
> and fill_grent(). Grepping through the sources shows me only those two
> locations using sss_mmap_cache_*_store().
You're right, you fixed both. I don't know why I didn't see that. Sorry
for the noise.
Ack.
Pushed to master.
3902
days inactive
3903
days old
sssd-devel@lists.fedorahosted.org
6 comments
2 participants
participants (2)
-
Jakub Hrozek -
Stephen Gallagher