search

PacktPublishing / Building-Low-Latency-Applications-with-CPP

Building Low Latency Applications with CPP by Packt Publishing
MIT License
399 stars 117 forks source link

resource leak in socket creation? #5

Open helium3a opened 10 months ago

helium3a commented 10 months ago

For the createSocket() in common/socket_utils.h, the for loop in the code is iterating over the linked list of addrinfo structures returned by getaddrinfo(). In each iteration of the loop, a new socket is created and the socket_fd is overwritten. If the loop iterates more than once, the previous socket file descriptors will be lost and the sockets will not be closed, which can lead to resource leaks.

I don't know if I am missing something and I would appreciate if anyone can help me with this. Thanks.

sowgali commented 9 months ago

Even I am struggling to understand the potential reason for this design.

stacygaudreau commented 7 months ago

Good catch @helium3a

I've noticed a couple of other places where a matching call to socket close() is not encountered. For example, the destructors of TCPSocket and TCPServer should probably be calling close() on the socket members they are responsible for, though I reckon that's a smaller issue than the for loop.

It's also best to free the allocated addrinfo structs when done with them, eg: near the end of the createSocket() method, with a call to freeaddrinfo(result)

@sowgali Reckon it could be improved as well but it seems the premise is that getaddrinfo() is a sort of "search" or "filter" method, and it's possible to return more than one matching addrinfo in the linked list.

That being said, I've tried to think of a reason why there would ever be more than one match returned in our specific use case here, and I cannot think of one. We've explicitly fixed the IP version to IPv4, the protocol to either TCP or UDP, the port number to a given integer, and we are doing a "search" by a static IP address (and not hostname or something else that might be bound to multiple different interfaces).

If someone can think of a reason why there might ever be more than 0 or 1 results returned from the getaddrinfo() linked list in this instance, I'd be interested to hear.

In lieu of that I suppose it's safe as a quick fix to discard and close() any sockets beyond one match that are created by this method.