rx: fix out-of-range value for RX_CONN_NAT_PING 41/13041/4
authorMark Vitale <mvitale@sinenomine.net>
Mon, 30 Apr 2018 22:34:28 +0000 (18:34 -0400)
committerBenjamin Kaduk <kaduk@mit.edu>
Fri, 24 Jul 2020 03:06:14 +0000 (23:06 -0400)
Commit 496fb87372555f6acddd4fd88b03c94c85f48511 ("rx: avoid nat ping
until connection is attached") introduced functionality to defer turning
on NAT ping for server connections until after reachability had been
established for the client.

Unfortunately, this feature could never work correctly because it
assigned an out-of-range flag value of 256 (0x100) for the u_char flags
field. Instead of calling this out as an error, both gcc and Solaris cc
elide this flag so that it is never set in
rx_SetConnSecondsUntilNatPing(), Furthermore, the test in
rxi_ConnClearAttachWait() will always fail; therefore
rxi_ScheduleNatKeepAliveEvent is never called after attach wait has
ended.

Fortunately, this bug is currently moot - not actually exposed in
OpenAFS. (It was discovered by inspection). This is because there are
currently no rx_connection objects in the tree that have both NAT ping
and checkReach (rx_SetCheckReach) enabled. I also searched git history
and found no time when this bug could ever have been exposed. This does
raise the question of why the original commit was needed; but instead of
reverting the original commit, this commit attempts to fix it.

To prevent problems if NAT ping and checkReach are ever both enabled for
an rx_connection, enlarge the rx_connection flags member so that the
RX_CONN_NAT_PING value is no longer out of range.

Change-Id: Ib667ece632f66fa5c63a76398acb3153fed6f9c3
Reviewed-on: https://gerrit.openafs.org/13041
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

doc/txt/rx-debug.txt
src/rx/rx.h
src/rx/rx_conn.h
src/rx/rx_packet.c

index d3f8aff..51d555a 100644 (file)
@@ -194,6 +194,8 @@ as follows:
         afs_int32 sparel[9];
     };
 
+Note: the char 'flags' member is no longer able to represent all possible values in the
+rx_connection 'flags' member, after the latter was enlarged from u_char to afs_uint32.
 
 An obsolete layout, which exhibited a problem with data alignment, was used in
 Version 'L'. This is defined as:
index f91f457..219ad2d 100644 (file)
@@ -365,7 +365,7 @@ struct rx_service {
 #define RX_CONN_BUSY               32  /* connection is busy; don't delete */
 #define RX_CONN_ATTACHWAIT        64   /* attach waiting for peer->lastReach */
 #define RX_CONN_MAKECALL_ACTIVE   128   /* a thread is actively in rx_NewCall */
-#define RX_CONN_NAT_PING          256   /* nat ping requested */
+#define RX_CONN_NAT_PING          256   /* NAT ping requested but deferred during attachWait */
 
 /* Type of connection, client or server */
 #define        RX_CLIENT_CONNECTION    0
index 60ae977..90e4fcf 100644 (file)
@@ -54,10 +54,11 @@ struct rx_connection {
     struct rx_service *service;        /* used by servers only */
     u_short serviceId;         /* To stamp on requests (clients only) */
     afs_int32 refCount;                /* Reference count (rx_refcnt_mutex) */
-    u_char flags;              /* Defined below - (conn_data_lock) */
+    u_char spare;              /* was flags - placeholder for alignment */
     u_char type;               /* Type of connection, defined below */
     u_char secondsUntilPing;   /* how often to ping for each active call */
     u_char securityIndex;      /* corresponds to the security class of the */
+    afs_uint32 flags;          /* Defined in rx.h RX_CONN_* */
     /* securityObject for this conn */
     struct rx_securityClass *securityObject;   /* Security object for this connection */
     void *securityData;                /* Private data for this conn's security class */
index c52937e..6cf997b 100644 (file)
@@ -1896,7 +1896,7 @@ rxi_ReceiveDebugPacket(struct rx_packet *ap, osi_socket asocket,
 
                        tconn.natMTU = htonl(tc->peer->natMTU);
                        tconn.error = htonl(tc->error);
-                       tconn.flags = tc->flags;
+                       tconn.flags = (u_char) (tc->flags & 0xff);  /* compat. */
                        tconn.type = tc->type;
                        tconn.securityIndex = tc->securityIndex;
                        if (tc->securityObject) {