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:
> 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
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
New patch is attached.