Skip to contentSkip to conversation

reintroduce md5_digest wrapper struct to fix gcc warnings

PATCH In review Steffan Karger 2 participants 3 messages Jul 26, 2015 – Jul 27, 2015 1 message not in the archive

lore
Share & tools
lore

Steffan Karger

author
#0 lore raw

PATCH 1/1 reintroduce md5_digest wrapper struct to fix gcc warnings

I was wrong to assume that adding the const qualifier to the pointer-to- fixed-size-array contruction used in options_hash_changed_or_zero() was allowed. GCC actually warns about this, but I was using clang and clang seems to be fine with the contruction. To make GCC happy too, reintroduce the md5_digest wrapped struct, and use that when passing around the digest.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/crypto.h  |  5 +++++
 src/openvpn/init.c    | 12 ++++++------
 src/openvpn/openvpn.h |  4 ++--
 src/openvpn/push.c    |  2 +-
 4 files changed, 14 insertions(+), 9 deletions(-)
src/openvpn/crypto.h +5 -0
index 504896d..b32a900 100644
@@ -108,6 +108,11 @@
 #include "packet_id.h"
 #include "mtu.h"
 
+/** Wrapper struct to pass around MD5 digests */
+struct md5_digest {
+  uint8_t digest[MD5_DIGEST_LENGTH];
+};
+
 /*
  * Defines a key type and key length for both cipher and HMAC.
  */
src/openvpn/init.c +6 -6
index 98cd288..b7c153b 100644
@@ -1616,11 +1616,12 @@ tun_abort()
  * equal, or either one is all-zeroes.
  */
 static bool
-options_hash_changed_or_zero(const uint8_t (*a)[MD5_DIGEST_LENGTH],
-    const uint8_t (*b)[MD5_DIGEST_LENGTH])
+options_hash_changed_or_zero(const struct md5_digest *a,
+    const struct md5_digest *b)
 {
-  const uint8_t zero[MD5_DIGEST_LENGTH] = {0};
-  return memcmp (*a, *b, MD5_DIGEST_LENGTH) || memcmp (*a, zero, MD5_DIGEST_LENGTH);
+  const struct md5_digest zero = {{0}};
+  return memcmp (a, b, sizeof(struct md5_digest)) ||
+      memcmp (a, &zero, sizeof(struct md5_digest));
 }
 #endif /* P2MP */
 

@@ -1664,8 +1665,7 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
       if (c->c2.did_open_tun)
 	{
 #if P2MP
-	  memcpy(c->c1.pulled_options_digest_save, c->c2.pulled_options_digest,
-	      sizeof(c->c1.pulled_options_digest_save));
+	  c->c1.pulled_options_digest_save = c->c2.pulled_options_digest;
 #endif
 
 	  /* if --route-delay was specified, start timer */
src/openvpn/openvpn.h +2 -2
index ef7ca1d..1c2a80b 100644
@@ -199,7 +199,7 @@ struct context_1
 #endif
 
   /* if client mode, hash of option strings we pulled from server */
-  uint8_t pulled_options_digest_save[MD5_DIGEST_LENGTH];
+  struct md5_digest pulled_options_digest_save;
                                 /**< Hash of option strings received from the
                                  *   remote OpenVPN server.  Only used in
                                  *   client-mode. */

@@ -465,7 +465,7 @@ struct context_2
   /* hash of pulled options, so we can compare when options change */
   bool pulled_options_md5_init_done;
   md_ctx_t pulled_options_state;
-  uint8_t pulled_options_digest[MD5_DIGEST_LENGTH];
+  struct md5_digest pulled_options_digest;
 
   struct event_timeout server_poll_interval;
src/openvpn/push.c +1 -1
index 46a44b1..6e92392 100644
@@ -481,7 +481,7 @@ process_incoming_push_msg (struct context *c,
 	      case 0:
 	      case 1:
 		md_ctx_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest);
+		md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
 		md_ctx_cleanup (&c->c2.pulled_options_state);
 	        c->c2.pulled_options_md5_init_done = false;
 		ret = PUSH_MSG_REPLY;
Signature · 1 line
2.1.4

A message that is not in the archive (sent off-list or not yet received): [email protected]...

author
#2 lore raw
Quoted 7 lines · David Sommerseth
Quoted 6 lines · Steffan Karger

I was wrong to assume that adding the const qualifier to the pointer-to- fixed-size-array contruction used in options_hash_changed_or_zero() was allowed. GCC actually warns about this, but I was using clang and clang seems to be fine with the contruction. To make GCC happy too, reintroduce the md5_digest wrapped struct, and use that when passing around the digest.

Quoted 5 lines
Quoted 1 line
Signed-off-by Steffan Karger <[email protected]...>

On holiday these days, so too lazy to check the git commit log ... but is this a revert of a single commit? If so, having a reference to that commit would be beneficial to include in this commit message.

It doesn't really revert a commit, since the commit that introduced this changed a lot more code. But it indeed does make sense to reference it explicitly. Gert, could you include a reference to 827de237860813d2859aaae3aca292d42a9c2a82 in the commit msg?

-Steffan

#3 lore raw
In reply to Steffan Karger

Your patch has been applied to the master branch. Reference to 827de237860813d2859a added, as requested :-)

commit 2dd6501e3d679046a1ed488f22d62defdf737cf3 (master)

Author: Steffan Karger
Date:   Sun Jul 26 13:27:19 2015 +0200
     reintroduce md5_digest wrapper struct to fix gcc warnings

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/9949
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering