> 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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
