[SSSD] [PATCH] mmap_cache: Check if slot and name_ptr are not invalid.

Michal Židek mzidek at redhat.com
Wed Aug 7 16:38:47 UTC 2013


On 08/05/2013 09:30 PM, Simo Sorce wrote:
 > On Mon, 2013-08-05 at 21:13 +0200, Michal Židek wrote:
 >> This is very ugly hotfix for ticket:
 >> https://fedorahosted.org/sssd/ticket/2018
 >>
 >> So far we were not able to find out why the slot or name_ptr
 >> values were corrupted, but this should at least prevent segfaults.
 >
 > NACK, (but close)
 >
 > Please do not add all those comments about removing checks, those checks
 > are good to stay forever, the cache may be corrupted by a bad disk
 > sector or whatever, so they should never be removed.

Ok. I left just one FIXME (actually two, but related) label in the case
where this patch relies on the same offset values of strs in both
group and passwd data structure. I do not expect these structures to
be changed, but it is something we want to have at least noticed in
the code.

 >
 > Also please do not just return ENOENT, and pretend nothing happened.
 > If the cache is corrupted we want to take immediate corrective action.
 > I suggest we return a new SSSD error and in the topmost nsscache caller
 > we invalidate the current cache and reinit.

Actually I started doing it like this, but handling the problem in the
topmost caller is not very good idea here. When I was writing it
I found several topmost callers and even forcing the error
to bubble up to these callers required some api changes. In addition
we would have to make sure we handle the corrupted memory cache case
in the future, when we use the affected functions in other topmost
callers (it is not so easy to track). So I decided to handle it
on the place where we detect the error.

 >
 > Optional (may be should be a separate ticket)
 > We currently reinit by closing the file and creating a new one. We
 > should not do that, we should rather reset the header and then just zero
 > all the data and reinitialize all the various areas. The reason is that
 > if we have some bad bug and we keep creating new files we might run out
 > of space because clients may have old files open and mmapped, and unless
 > they perform a new operations, they may not release them. Note we should
 > *not* use ftruncate() here, just write the appropriate initialization
 > values everywhere needed.

I agree, but sss_mmap_cache_reinit is completely unsuitable for this
case and a huge overkill, even if we might change it in the future.
I created a new function sss_mmap_cache_reset which is very
lightweight. I will not create it as a separate ticket
since I do not want to use the reinit function even temporary for
this case.

New patch is attached.

Thanks
Michal



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mmap_cache-Check-if-slot-and-name_ptr-are-not-invali.patch
Type: text/x-patch
Size: 7680 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130807/51816c84/attachment.bin>


More information about the sssd-devel mailing list