]>
Commit | Line | Data |
---|---|---|
e74c0f31 WB |
1 | From 4dbc47f9d71b8a17b174ffed314988aa99dc4775 Mon Sep 17 00:00:00 2001 |
2 | From: Eric Blake <eblake@redhat.com> | |
3 | Date: Fri, 26 May 2017 22:04:21 -0500 | |
4 | Subject: [PATCH 18/23] nbd: Fully initialize client in case of failed | |
5 | negotiation | |
6 | ||
7 | If a non-NBD client connects to qemu-nbd, we would end up with | |
8 | a SIGSEGV in nbd_client_put() because we were trying to | |
9 | unregister the client's association to the export, even though | |
10 | we skipped inserting the client into that list. Easy trigger | |
11 | in two terminals: | |
12 | ||
13 | $ qemu-nbd -p 30001 --format=raw file | |
14 | $ nmap 127.0.0.1 -p 30001 | |
15 | ||
16 | nmap claims that it thinks it connected to a pago-services1 | |
17 | server (which probably means nmap could be updated to learn the | |
18 | NBD protocol and give a more accurate diagnosis of the open | |
19 | port - but that's not our problem), then terminates immediately, | |
20 | so our call to nbd_negotiate() fails. The fix is to reorder | |
21 | nbd_co_client_start() to ensure that all initialization occurs | |
22 | before we ever try talking to a client in nbd_negotiate(), so | |
23 | that the teardown sequence on negotiation failure doesn't fault | |
24 | while dereferencing a half-initialized object. | |
25 | ||
26 | While debugging this, I also noticed that nbd_update_server_watch() | |
27 | called by nbd_client_closed() was still adding a channel to accept | |
28 | the next client, even when the state was no longer RUNNING. That | |
29 | is fixed by making nbd_can_accept() pay attention to the current | |
30 | state. | |
31 | ||
32 | Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1451614 | |
33 | ||
34 | Signed-off-by: Eric Blake <eblake@redhat.com> | |
35 | Message-Id: <20170527030421.28366-1-eblake@redhat.com> | |
36 | Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> | |
37 | --- | |
38 | nbd/server.c | 8 +++----- | |
39 | qemu-nbd.c | 2 +- | |
40 | 2 files changed, 4 insertions(+), 6 deletions(-) | |
41 | ||
42 | diff --git a/nbd/server.c b/nbd/server.c | |
43 | index 924a1fe2db..edfda84d43 100644 | |
44 | --- a/nbd/server.c | |
45 | +++ b/nbd/server.c | |
46 | @@ -1376,16 +1376,14 @@ static coroutine_fn void nbd_co_client_start(void *opaque) | |
47 | ||
48 | if (exp) { | |
49 | nbd_export_get(exp); | |
50 | + QTAILQ_INSERT_TAIL(&exp->clients, client, next); | |
51 | } | |
52 | + qemu_co_mutex_init(&client->send_lock); | |
53 | + | |
54 | if (nbd_negotiate(data)) { | |
55 | client_close(client); | |
56 | goto out; | |
57 | } | |
58 | - qemu_co_mutex_init(&client->send_lock); | |
59 | - | |
60 | - if (exp) { | |
61 | - QTAILQ_INSERT_TAIL(&exp->clients, client, next); | |
62 | - } | |
63 | ||
64 | nbd_client_receive_next_request(client); | |
65 | ||
66 | diff --git a/qemu-nbd.c b/qemu-nbd.c | |
67 | index e080fb7c75..b44764eb87 100644 | |
68 | --- a/qemu-nbd.c | |
69 | +++ b/qemu-nbd.c | |
70 | @@ -324,7 +324,7 @@ out: | |
71 | ||
72 | static int nbd_can_accept(void) | |
73 | { | |
74 | - return nb_fds < shared; | |
75 | + return state == RUNNING && nb_fds < shared; | |
76 | } | |
77 | ||
78 | static void nbd_export_closed(NBDExport *exp) | |
79 | -- | |
80 | 2.11.0 | |
81 |