net: posix: Fix close() not closing sockets properly

[icsys-aal]

With CONFIG_POSIX_API enabled close() does not call
sock_obj_core_dealloc. This results in sockets not being
closed properly, which can be verified by running the
shell command 'net sockets'

[jukkar]

This is called by zsock_close() so wondering why we would need to call it again here. What kind of socket is this, a TLS one?

[jukkar]

Also, I don't think we should call socket specific API here in the generic code.

[icsys-aal]

This is a TCP socket.
sock_obj_core_dealloc is indeed called by zsock_close, however with POSIX_API enabled we never reach zsock_close, and comparing the two functions the only real difference is that zsock_close actually calls sock_obj_core_dealloc, which properly closes the socket.
zsock_close does the following: https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/net/lib/sockets/sockets.c#L137

[jukkar]

Something is now wrong if socket close is not called. Either this function should not be called at all but zsock_close() should, or the system should be configured so that the fdtable[fd].vtable->close(fdtable[fd].obj); would call the network socket close. In all cases this function should not do anything socket specific as this is a generic function that just dispatches the call to correct subsystem.

[cfriedt]

Yeah, I would agree - the vtable call should deallocate any fs-specific object associated with the generic fd.

[jukkar]

Something is now wrong if socket close is not called. Either this function should not be called at all but zsock_close() should, or the system should be configured so that the fdtable[fd].vtable->close(fdtable[fd].obj); would call the network socket close. In all cases this function should not do anything socket specific as this is a generic function that just dispatches the call to correct subsystem.

[jukkar]

@icsys-aal how did you see the issue, is there easy way to reproduce with network samples for example?

[icsys-aal]

I saw this issue while debugging a thirdparty web server, I ran net sockets to see what sockets were open, and to my surprise there were multiple entries with the same file descriptor after close() had been called on them.
To reproduce the issue I do believe any socket sample used along with CONFIG_POSIX_API enabled would work.

[icsys-aal]

Just to expand on the behavior we see, there is a difference between what happens when we call close() vs when we call zsock_close(). Our expectation is that these should be the same, but in practice they aren't.

When calling close, with CONFIG_POSIX_API enabled, we see this call sequence:
close => zvfs_close => zsock_close_ctx => net_context_put
However, when calling zsock_close(), we see this sequence:
zsock_close => z_impl_zsock_close => sock_close_vmeth => zsock_close_ctx => net_context_put
The only difference that actually seems to matter between zvfs_close() and zsock_close() is the call to sock_obj_core_dealloc() that is added in this PR.

It's possible to reproduce similar behavior to what we see in our application by modifying the http_server sample application, which has CONFIG_POSIX_API=y. Replacing zsock_close() with plain close() in http_server_core.c, leads to the application leaking sockets after a couple of HTTP GET requests using curl, without the fix proposed in this PR:

                  Creator  Name          Flags  FD   Lifetime (ms)   Sent      Received

          http_server_tid  af_inet46     4ST    4    87280           0         0    
          http_server_tid  af_inet46     4S     5    84883           2124      1587 
          http_server_tid  af_inet46     4S     6    74379           236       72   
          http_server_tid  af_inet46     4S     5    2537            0         0    
          http_server_tid  af_inet46     4S     5    2340            0         0    
          http_server_tid  af_inet46     4S     5    2192            0         0    
          http_server_tid  af_inet46     4S     5    2023            0         0    
          http_server_tid  af_inet46     4S     5    1827            0         0    
          http_server_tid  af_inet46     4S     5    1677            0         0    

9 active sockets found.

However, with the original zsock_close() calls, the same type of testing gives sockets that are actually closed:

uart:~$ net sockets
                  Creator  Name          Flags  FD   Lifetime (ms)   Sent      Received

          http_server_tid  af_inet46     4ST    4    21799           0         0    
          http_server_tid  af_inet46     4S     C    56              236       72   
          http_server_tid  af_inet46     4S     C    73              236       72   
          http_server_tid  af_inet46     4S     C    59              236       72   
          http_server_tid  af_inet46     4S     C    56              236       72   
          http_server_tid  af_inet46     4S     C    51              236       72   
          http_server_tid  af_inet46     4S     C    68              236       72   
          http_server_tid  af_inet46     4S     C    60              236       72   
          http_server_tid  af_inet46     4S     C    69              236       72   

1 active socket found.
8 closed sockets found.

Adding sock_obj_core_dealloc(fd), as proposed in this PR, to zvfs_close() makes the application behave the same both with close() and zsock_close().

I do agree that performing a socket operation in fdtable.c seems a bit strange, but struggle to see any better options. It could perhaps be a solution to make zvfs_close() call zsock_close() (e.g. use zsock_close() in the sockets vtable), but zvfs_close() currently use the .obj field of the fd as the argument for close(), which is not compatible with the API of zsock_close(). Do you see any other, better solutions?

[cfriedt]

Let's keep this code generic, and use the vtable call to deallocate fd-specific resources.

[cfriedt]

Couldn't the vtable method simply call zsock_close()? On mobile at the moment, so not looking too deeply at the net API

[cfriedt]

I would suggest adding a file-descriptor memory leak test to the posix network testsuite as well.

It should be sufficient to attempt to call socket() and close() twice the number of CONFIG_ZVFS_FD_MAX times (if I remember the Kconfig option correctly).

[icsys-aal]

I agree that calling a socket function in fdtable.c seems wrong, but I'm unable to see where else it is possible to do so.

Currently, the close function pointer for sockets in fd_op_table points at sock_close_vmeth(), which is just a wrapper for zsock_close_ctx(), and both those functions take a net_context* as an argument. The function that needs to be called to resolve the socket leak is _sock_obj_core_dealloc(), and this function takes an fd as an argument.

As far as I can see, the only place that maps an fd to its corresponding net_context* is fdtable. It hence seems impossible to make the sock_close_vmeth() function or its descendants properly call sock_obj_core_dealloc(), since we can't get hold of the correct argument, e.g. the fd itself, from the net_context*.

The idea to call sock_obj_core_dealloc() came from websocket_interal_disconnect() in websocket.c, but in that case the websocket_context* has a field for the actual socket to make this possible. Adding such field to net_context would of course be possible, but seems a bit too invasive IMHO.

Are there any other options we're missing?

[cfriedt]

I agree that calling a socket function in fdtable.c seems wrong, but I'm unable to see where else it is possible to do so.

Currently, the close function pointer for sockets in fd_op_table points at sock_close_vmeth(), which is just a wrapper for zsock_close_ctx(), and both those functions take a net_context* as an argument. The function that needs to be called to resolve the socket leak is _sock_obj_core_dealloc(), and this function takes an fd as an argument.

sock_close_vmeth() and z_impl_zsock_close() could be close to identical past here.

zephyr/subsys/net/lib/sockets/sockets.c

Line 166 in 1ec5ce0

(void)k_mutex_lock(lock, K_FOREVER);

[icsys-aal]

sock_close_vmeth() and z_impl_zsock_close() could be close to identical past here.

Yeah, line 178 here, the call to sock_obj_core_dealloc, is what is required to fix the leak. Unfortunately, calling that function requires the socket itself as the argument, and as mentioned above I don't see how we can get hold of the socket from inside sock_close_vmeth().

[cfriedt]

sock_close_vmeth() and z_impl_zsock_close() could be close to identical past here.

Yeah, line 178 here, the call to sock_obj_core_dealloc, is what is required to fix the leak.

zephyr/subsys/net/lib/sockets/socket_obj_core.c

Line 193 in 6300e1b

int sock_obj_core_dealloc(int fd)

The fd is only used to get the object, so it would make some sense to maybe factor that part out to a common function (preferably static) that could be called by both sock_close_vmeth() and z_impl_zsock_close().

[icsys-aal]

The fd is only used to get the object, so it would make some sense to maybe factor that part out to a common function (preferably static) that could be called by both sock_close_vmeth() and z_impl_zsock_close().

Thanks, but unfortunately it seems like the object used inside sock_obj_core_dealloc is not the same object that is provided to sock_close_vmeth. sock_close_vmeth is passed a net_context* while sock_obj_core_dealloc uses the fd to find a sock_obj. I don't see any way to go from one to the other.
Any ideas?

[cfriedt]

Any ideas?

This is a strange case of fdtable usage because the same integer file descriptor is associated with two separate vtables.

In that case, I would suggest that a union be made using the int (*close)(void *obj) and a new int (*close2)(void *obj, int fd).

https://github.com/zephyrproject-rtos/zephyr/blob/main/include%2Fzephyr%2Fsys%2Ffdtable.h

The zsock_close_vmeth() signature should be adjusted accordingly, along with the vtable. TLS sockets might also need to be updated. And zvfs_close() should pass the additional fd when the object is of socket type (although I think it should be safe anyway to use for any fd).

With that, you can directly call zsock_obj_dealloc().

[jukkar]

This is a strange case of fdtable usage because the same integer file descriptor is associated with two separate vtables.

Actually there is one fdtable involved, the

zephyr/subsys/net/lib/sockets/sockets.c

Line 53 in 6e653a9

ctx = zvfs_get_fd_obj_and_vtable(sock,

is calling fdtable as expected.

There would be no issues if close(int) could call zsock_close(int) but it is not able to do that. Now the extra housekeeping done in zsock_close() is not done (socket tracing and socket number tracking). As described by @icsys-aal one of the problem is that it is difficult to get the socket number from the socket object pointer.

[cfriedt]

Perhaps calling it a vtable was the wrong terminology. Effectively though, the close method just needs access to the integer fd.

I don't see any reason to keep that a secret from vtable methods though.

Please see the solution in my previous comment.

[jukkar]

I think we could do

int z_impl_zsock_close(int sock)
{
	return zvfs_close(sock);
}

and let zvfs_close() to call the actual socket close. I can experiment with this solution a bit.

[jukkar]

I don't see any reason to keep that a secret from vtable methods though.

Please see the solution in my previous comment.

Yes, I agree, we could easily pass the fd to close2()

[jukkar]

And zvfs_close() should pass the additional fd when the object is of socket type (although I think it should be safe anyway to use for any fd).

@cfriedt How can we know in fdtable.c the type of the fd, this is not very clear to me?
There is k_object_is_valid(ctx, K_OBJ_NET_SOCKET) but that is only used if CONFIG_USERSPACE=y

[jukkar]

How can we know in fdtable.c the type of the fd, this is not very clear to me?

Found it, there is the ZVFS_MODE_IFSOCK flag we can use.

[jukkar]

I created alternative proposal here #80232, @icsys-aal please take a look if it works for you.