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
