[pulseaudio-discuss] [PATCH v2 05/12] pulsecore: SHM: Introduce memfd support

[David Henningsson]

On 2016-02-13 02:58, Ahmed S. Darwish wrote:
> Hi!
>
> On Fri, Feb 12, 2016 at 04:49:23PM +0100, David Henningsson wrote:
>>
>> On 2016-02-12 01:14, Ahmed S. Darwish wrote:
>>> +    /* Do not close file descriptors which are not our own */
>>> +    if (type != PA_MEM_TYPE_SHARED_MEMFD)
>>> +        pa_assert_se(pa_close(fd) == 0);
>>
>> I believe the fd should be closed regardless of type?
>>
>> Now you seem to leak an fd if type == PA_MEM_TYPE_SHARED_MEMFD?
>>
>
> Hmm, that comment should've been further clarified..
>
> By "not close file descriptors which are not our own", I meant
> not to close the fd behind our caller's back. It was just passed
> to us as a parameter and thus we do not have any kind of
> ownership to close it.
>
> This is unlike attaching to a Posix SHM  where the fd was created
> by us in the same code path [ using shm_open() ] and thus we have
> its complete ownership -- and must close it afterwards.
>
> So for memfd attachment, the caller owns the passed fd and is
> responsible for closing it when it sees fit. That's also why we
> do not cache the passed fd in the callee and set pa_shm.memfd.fd
> to -1.

Right, so if you would have instead written something like:

if (type == PA_MEM_TYPE_SHARED_MEMFD)
     m->per_type.memfd.fd = fd;
else
     pa_assert_se(pa_close(fd) == 0);

...it would be more obvious that the ownership of the fd is transferred 
into the struct at that point, or closed.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic