Re: (ITS#8844) LMDB mdb_env_close is unsafe in forked child

[hyc@symas.com]

Hallvard Breien Furuseth wrote:
> On 14/06/2018 20:34, Howard Chu wrote:
>> Hallvard Breien Furuseth wrote:
>>> Wait, what?  The child didn't write any pids to the table.  If
>>> there are pids for it to clear, something is wrong. That can be:
>>
>> No, there most likely aren't PIDs for it to clear. But most importantly, it 
>> must not clear using env->me_pid since that is still the parent's PID.
>>
>>> - The pid is from an older run, and the system reused the pid.  If we
>>>    code for this, it should be to clear such pids during mdb_env_open.
>>>    It gets very arbitrary to clear them when the user does env_close()
>>>    in the child and the child happens to have the that reused pid.
>>
>> No. If the system reused the PID then that means the previous owner of that 
>> PID is dead already anyway.
> 
> Right...
> 
>> If there are any table entries under that PID there's no problem removing them.
> 
> The problem is that it's arbitrary.  Something is exiting without
> clearing its pid slots, and 0.001% of the time this code will catch it.

Sure. But I don't think it's worthwhile to make a special case here to skip 
this step.

> Better to skip this cleanup, or to always do it - but "always" belongs
> in env_open().   If a new env is seeing its pid in use, then it can safely 
> clean it out. Last we talked about that, I liked it and you
> did not:-)

And I don't see any particular reason to add this step.

>>> - The child opened a new MDB_env with the same path, and closes the
>>>    parent's env afterwards.  Don't Do That, users.  But what to do in
>>>    this situation, if we are to have code for it, is to do nothing with
>>>    the lockfile.  Not close it either, since closing it will lose the
>>>    lock for the env which was opened in the child.
>>
>> That's a completely different case. This ITS is specifically about a process 
>> with an open environment that subsequently forks.
> 
> No, I mean:
> 
> Parent, pid P:
>   open env A.
> 
> Child, pid Q:
> - open and start using env B with the same path.
> - close env A, while B is still open.
>    With the new code, this clears pid Q - i.e. B's pids.
>    But lmdb.h already lists that as a "don't do that",
>    because it also removes B's lock on the lockfile.
> 
Yeah, this is still a "don't do that" though, regardless of this particular ITS.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/