Hi, On Thu, Dec 20, 2012 at 06:49:49PM +0400, p37sitdu at lavabit.com wrote:
On Sun, Dec 16, 2012 at 07:43:19PM -0300, Jorge Arellano Cid wrote:
On Sun, Dec 09, 2012 at 02:58:43PM +0400, p37sitdu at lavabit.com wrote:
Also, comment says "the server closes sock_fd on error". However, sock_fd should still be closed by client to free allocated file descriptor. OS don't know if Dillo is going to read from it (in which case error such as ECONNRESET should happen) so file descriptor can't be reused.
File descriptor should be closed in case of error in a_Dpip_build_cmd too.
The file descriptor is properly closed by its CCC afterwards (Visible by defining VERBOSE in chain.c).
The CCC manages the error propagation/handling, in both the requesting and answer branches.
File descriptor is properly closed if there were no errors. But if there is an error in a_Dpip_build_cmd or Dpi_blocking_write, sock_fd should be closed before returning from Dpi_connect_socket.
Dpi_connect_socket calls a_Dpip_build_cmd to build command and Dpi_blocking_write to write this command to sock_fd. a_Dpip_build_cmd does not accept sock_fd as its argument so it can't close it. Dpi_blocking_write only writes to fd, but does not close it on error.
So if one of these two function fails, Dpi_connect_socket should close sock_fd before returning -1. In the current state it just returns -1 and leave sock_fd open.
By the time Dpi_connect_socket returns, sock_fd is lost and will not be closed by any code outside Dpi_connect_socket.
Attached patch adds code to close sock_fd in case of error.
As from the patch you sent (same as before), and the explanation you give (ignoring my explanation), I guess you're making conclusions out of reading the code in Dpi_connect_socket() *only*. Have you checked what you state as fact? (i.e. that the FD is leaked and not closed by the CCC). FWIW, I did check before writing my previous answer. Of course I'm human and do make mistakes, so if this is the case: can you please verify and show how to reproduce the bug? -- Cheers Jorge.-