On 16 October 2017 at 21:16, Daniel P. Berrange <berra...@redhat.com> wrote:
> From: Knut Omang <knut.om...@oracle.com>
>
> If an offset of ports is specified to the inet_listen_saddr function(),
> and two or more processes tries to bind from these ports at the same time,
> occasionally more than one process may be able to bind to the same
> port. The condition is detected by listen() but too late to avoid a failure.
>
> This function is called by socket_listen() and used
> by all socket listening code in QEMU, so all cases where any form of dynamic
> port selection is used should be subject to this issue.
>
> Add code to close and re-establish the socket when this
> condition is observed, hiding the race condition from the user.
>
> Also clean up some issues with error handling to allow more
> accurate reporting of the cause of an error.
>
> This has been developed and tested by means of the
> test-listen unit test in the previous commit.
> Enable the test for make check now that it passes.
>
> Reviewed-by: Bhavesh Davda <bhavesh.da...@oracle.com>
> Reviewed-by: Yuval Shaia <yuval.sh...@oracle.com>
> Reviewed-by: Girish Moodalbail <girish.moodalb...@oracle.com>
> Signed-off-by: Knut Omang <knut.om...@oracle.com>
> Reviewed-by: Daniel P. Berrange <berra...@redhat.com>
> Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
Hi. Coverity points out that this code could leak a socket fd
(CID 1381805):
> - /* create socket + bind */
> + /* create socket + bind/listen */
> for (e = res; e != NULL; e = e->ai_next) {
> getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
> uaddr,INET6_ADDRSTRLEN,uport,32,
> @@ -266,37 +269,58 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>
> slisten = create_fast_reuse_socket(e);
Every time around this outer for() loop we create an fd by calling
create_fast_reuse_socket(), which means that the previous fd
we had in that variable is leaked.
> if (slisten < 0) {
> - if (!e->ai_next) {
> - error_setg_errno(errp, errno, "Failed to create socket");
> - }
> continue;
> }
>
> + socket_created = true;
> port_min = inet_getport(e);
> port_max = saddr->has_to ? saddr->to + port_offset : port_min;
> for (p = port_min; p <= port_max; p++) {
> inet_setport(e, p);
> - if (try_bind(slisten, saddr, e) >= 0) {
> - goto listen;
> - }
> - if (p == port_max) {
> - if (!e->ai_next) {
> + rc = try_bind(slisten, saddr, e);
> + if (rc) {
> + if (errno == EADDRINUSE) {
> + continue;
> + } else {
> error_setg_errno(errp, errno, "Failed to bind socket");
> + goto listen_failed;
> }
> }
> + if (!listen(slisten, 1)) {
> + goto listen_ok;
> + }
> + if (errno != EADDRINUSE) {
> + error_setg_errno(errp, errno, "Failed to listen on socket");
> + goto listen_failed;
> + }
> + /* Someone else managed to bind to the same port and beat us
> + * to listen on it! Socket semantics does not allow us to
> + * recover from this situation, so we need to recreate the
> + * socket to allow bind attempts for subsequent ports:
> + */
> + closesocket(slisten);
> + slisten = create_fast_reuse_socket(e);
(Here we do close the old fd before opening a new one, so the
inner loop doesn't leak, only the outer one.)
> + if (slisten < 0) {
> + error_setg_errno(errp, errno,
> + "Failed to recreate failed listening
> socket");
> + goto listen_failed;
> + }
> }
> + }
thanks
-- PMM
