Fix missing return value checks in multi_process_float()

Fix the missing return value checks on hash_remove() and hash_add() by replacing the calls with an single hash_add() call with the replace parameters set to true so that is can't fail. Then just ASSERT() that this is indeed the case.

This also replaces the other add/remove combinations with a single add-replace, because that should be slightly faster (and this is in the 'hot path').

---
 src/openvpn/multi.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

index 93a554d..a834e94 100644
@@ -2320,9 +2320,6 @@ void multi_process_float (struct multi_context* m, struct multi_instance* mi)
 	mroute_addr_print (&mi->real, &gc),
 	print_link_socket_actual (&m->top.c2.from, &gc));
 
-    ASSERT (hash_remove(m->hash, &mi->real));
-    ASSERT (hash_remove(m->iter, &mi->real));
-
     /* change external network address of the remote peer */
     mi->real = real;
     generate_prefix (mi);

@@ -2336,12 +2333,11 @@ void multi_process_float (struct multi_context* m, struct multi_instance* mi)
 
     tls_update_remote_addr (mi->context.c2.tls_multi, &mi->context.c2.from);
 
-    ASSERT (hash_add (m->hash, &mi->real, mi, false));
-    ASSERT (hash_add (m->iter, &mi->real, mi, false));
+    ASSERT (hash_add (m->hash, &mi->real, mi, true));
+    ASSERT (hash_add (m->iter, &mi->real, mi, true));
 
 #ifdef MANAGEMENT_DEF_AUTH
-    hash_remove (m->cid_hash, &mi->context.c2.mda_context.cid);
-    hash_add (m->cid_hash, &mi->context.c2.mda_context.cid, mi, false);
+    ASSERT (hash_add (m->cid_hash, &mi->context.c2.mda_context.cid, mi, true));
 #endif
 
 done:

2.7.4

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

ACK.

Your patch has been applied to the master branch

commit b59fc7f42137a0474c069ab226c4d67c148e504f

Author: Steffan Karger
Date:   Tue Nov 8 21:07:43 2016 +0100

     Fix missing return value checks in multi_process_float()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12968.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYLHo2AAoJEIbPlEyWcf3y25kQAKyExAHmuufyBBy20AJqN2Cb zjbDXKKTJsttAFYoEYfakoFNYECCWjOdIu7VupF+YoUWFPTntUvgnc5vsnbNyOit 1SUVBi+4/LRlgrpJaGGy4IdtTEzrPAoIsUL6uutad5jGAgPmEClxuBz2xVmDh7op 6nv6YN83JdrHDFZg4gavSwS0qAwCAxbzQVeJktQ582Gtrx0Brv5hO5Ah64unuRGj kxrYpvX4oSB8sE+QmnMgbIGJaSWKABZqisK1SH3DfHo/cUDNwGFmL8yFgVrPwQeo FcgT6xJrPaf9BYVQBTd62o3V7g/HQlTb8mFXpr8BC6PjwttcCdaBkzgcRq0AAYTh 9Lj72fvmfoDX6MHmDSaGB8k/95/O2uiYZSXzJHGyg3d6tMAWBYNcTNRxhpmpauZm 3A8F6f/QcYLBS+wzW0zR04ekVDgOrj1BJJo7k1iIHib3Z9dqzL2K+Xjva6igFkp9 RxIjD7bvtvY9Wj+Q9P5+Wf7lI+yJ3dCTZiCPzQYQ8DI3flG99FgAWzH5l5GUN0FQ ZfmTZeMUWH3Qu98O9TMzCQSGKLUJJAMWxONhN888IlwPZmidz5UMlOtw/gqiTuXu qaVxbRNUB4mC3kiCSxqH9w1AzXGViUvXBVJ5s3DrF6AvL4j1P9ugc7rdjNNHgivq D9r2qBZi4F663LOy0sr/
=OUzk
-----END PGP SIGNATURE-----

Commit c14c4a9e merged the hash_remove() and hash_add() calls in multi_process_float(), but didn't notice that the hash key (mi->real) was updated between these calls. So we now try to remove the *new* address instead of the *old* address from the hash table. This leaks memory and might break stuff when a different client floats to the old address/port of this client. Restore that.

---
 src/openvpn/multi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index 8f3d34e..4fc8b02 100644
@@ -2317,6 +2317,10 @@ void multi_process_float (struct multi_context* m, struct multi_instance* mi)
 	mroute_addr_print (&mi->real, &gc),
 	print_link_socket_actual (&m->top.c2.from, &gc));
 
+    /* remove old address from hash table before changing address */
+    ASSERT (hash_remove (m->hash, &mi->real));
+    ASSERT (hash_remove (m->iter, &mi->real));
+
     /* change external network address of the remote peer */
     mi->real = real;
     generate_prefix (mi);

@@ -2330,8 +2334,8 @@ void multi_process_float (struct multi_context* m, struct multi_instance* mi)
 
     tls_update_remote_addr (mi->context.c2.tls_multi, &mi->context.c2.from);
 
-    ASSERT (hash_add (m->hash, &mi->real, mi, true));
-    ASSERT (hash_add (m->iter, &mi->real, mi, true));
+    ASSERT (hash_add (m->hash, &mi->real, mi, false));
+    ASSERT (hash_add (m->iter, &mi->real, mi, false));
 
 #ifdef MANAGEMENT_DEF_AUTH
     ASSERT (hash_add (m->cid_hash, &mi->context.c2.mda_context.cid, mi, true));

2.7.4

ACK. Explanation makes sense :) - the original change was "too easy"...

Your patch has been applied to the master branch.

commit c593885997e32a683f718138f7b9273d720c63e3

Author: Steffan Karger
Date:   Sat Nov 19 18:12:46 2016 +0100

     multi_process_float: revert part of c14c4a9e

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13128.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering