On Mon, 2012-01-09 at 18:24 -0500, Simo Sorce wrote:
> On Mon, 2012-01-09 at 16:30 -0500, Stephen Gallagher wrote:
> > On Tue, 2012-01-03 at 18:00 -0500, Simo Sorce wrote:
> > > >From [PATCH 0/0] A shared memory cache to perform better:
> > > 0/4: Actual memory cache implementation
> > > These is the bulk of the work, these patches are still a bit rough at
> > > the edges, grep for FIXMEs and TODOs and you'll see some plumbing
> > > (for example configure options in sssd to set expiration time and
> > > cache sizes are missing and are still harcoded).
> >
> > Patch 0008: Ack
> >
> > Patch 0009: Nack
> >
> > As the fixmes say, please parameterize the cache sizes and timeout
> > period.
> >
> > You have a time-of-check/time-of-use bug with the stat of mc_ctx->file.
> > Please open it first and then use fstat() instead, to avoid a
> > race-condition exploit.
>
> Not sure what race condition is there.
>
> sssd_nss is the only thing that can create/manipulate that file, in what
> case do you see a race condition ?
If filesystem protections are ever incorrect, then you have a situation
where theoretically a user could do something like replace the mmap file
with a symlink to /etc/passwd in the short gap between the stat() check
and the file open. We would then fail to validate the file and promptly
destroy it, hosing the system.
Although it's very unlikely scenario, I agree that we always choose not to
leave anything to chance in similar scenarios. Is there any downside to moving
the code?
> The problem of using fstat() is that it unnecessarily(IMO)
complicates
> the code for this case.
I think it's a necessary complication. Please use it.
> > Please use errno_t for the return code of functions that return errno
> > values.
>
> Ok.
>
> > I think the check_header pieces are unnecessary. When the NSS provider
> > is started, it should always remove an existing fast cache. This will
> > make the behavior better match users' expectations.
>
> Not sure about this, it unnecessarily causes cache lookups to go though
> the pipe to re-populate the fast cache. Perhaps we need to discuss a bit
> more pros-cons of either approach.
Sure, we can talk about it. I'm looking at it from the users'
perspectives, who I think would generally expect (and be alright with)
the fast cache being emptied on service restart. Since we still have the
not-quite-as-fast persistent LDB cache, I think the gain isn't worth the
user confusion.
I agree
> > Don't use ftruncate() to generate the cache file.
It's not safe on all
> > platforms or filesystems. Probably better to use fseek() (even though
> > it's a bit slower. It's a rare operation).
>
> I asked our filesystem gurus and they think ftruncate() is the best way
> to go. (and the kernel sources confirm it works with every local file
> systems even VFAT :-)
Well, my concern was more with porting to other platforms, but I suppose
we can burn those bridges when we come to them.
I thought we've already done that in several parts of the code, haven't we?
I also have one cosmetic proposal for patch #0009:
1) On line 452 use MC_ALIGN64 instead of (n_elem + 7) & (~7)
Thanks
Jan