Re: [PATCH mptcp-net v2 2/3] selftests: mptcp: close IPC descriptor on server side

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 04/09/2025 10:26, Geliang Tang wrote:
> Hi Matt,
> 
> On Wed, 2025-09-03 at 13:57 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 03/09/2025 06:08, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> The client-side function 'connect_one_server()' correctly closes
>>> the IPC
>>> descriptor (a pipe or UNIX socket) after use. However, the server-
>>> side
>>> functions 'process_one_client()' in both 'mptcp_sockopt.c' and
>>> 'mptcp_inq.c'
>>> were missing the corresponding 'close()' call for their IPC
>>> descriptors.
>>>
>>> This omission could lead to resource leaks (file descriptors) in
>>> the test
>>> server processes over time.
>>>
>>> This patch adds the missing 'close(pipefd)' and 'close(unixfd)'
>>> calls in the
>>> server-side code, ensuring symmetric and correct resource cleanup.
>>
>> I don't know if we need such patch. I mean: yes, that's better to
>> close
>> such FD before closing the application, but then:
>>
>> - it is strange to close it in the middle of a function
>> (process_one_client()), and not where it has been created (main())
>>
> 
> Yes, close it in main() is much better. I added close(pipefds[1]) after
> calling server(pipefds[1]) in the new version instead of adding
> close(pipefd) in process_one_client().
> 
>> - if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in the
>> server process.
> 
> Yes, indeed. close(pipefds[0]) is missing in the server process too.
> 
>> So if you really want to fix that, I think it would be better to
>> close
>> all these unix FD in the 'main()' function, no?
> 
> I also moved 'close(pipefd)' from connect_one_server() to main(), just
> after calling client(pipefds[0]). And here's the new version:
> 
>         e1 = pipe(pipefds);
>         if (e1 < 0)
>                 die_perror("pipe");
> 
>         s = xfork();
>         if (s == 0) {
>                 close(pipefds[0]);
>                 ret = server(pipefds[1]);
>                 close(pipefds[1]);
>                 return ret;
>         }
> 
>         close(pipefds[1]);
> 
>         /* wait until server bound a socket */
>         e1 = read(pipefds[0], &e1, 4); 
>         assert(e1 == 4); 
> 
>         c = xfork();
>         if (c == 0) {
>                 ret = client(pipefds[0]);
>                 close(pipefds[0]);
>                 return ret;
>         }
> 
>         close(pipefds[0]);
> 
> Is this better?

(sorry, I thought I replied...)

Yes, that looks more manageable like that: all close() by the "function
owner" (where they have been opened).

Also, please don't bother about closing all FD in the error exit paths,
e.g. in xfork(), etc. when xerror() is called. These are more unexpected
errors, and closing everything properly is this case, for the selftests
is not a priority, and might cause issues if the code is more complex (+
issues for the backports, etc.)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.