From 55fca11421055d0bcee79f118ea2a035393cc6e5 Mon Sep 17 00:00:00 2001 From: Mark Vitale Date: Mon, 30 Apr 2018 18:34:28 -0400 Subject: [PATCH] rx: fix out-of-range value for RX_CONN_NAT_PING 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 Reviewed-by: Benjamin Kaduk --- doc/txt/rx-debug.txt | 2 ++ src/rx/rx.h | 2 +- src/rx/rx_conn.h | 3 ++- src/rx/rx_packet.c | 2 +- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/doc/txt/rx-debug.txt b/doc/txt/rx-debug.txt index d3f8aff245..51d555a2e8 100644 --- a/doc/txt/rx-debug.txt +++ b/doc/txt/rx-debug.txt @@ -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: diff --git a/src/rx/rx.h b/src/rx/rx.h index f91f457885..219ad2d656 100644 --- a/src/rx/rx.h +++ b/src/rx/rx.h @@ -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 diff --git a/src/rx/rx_conn.h b/src/rx/rx_conn.h index 60ae9776ef..90e4fcf400 100644 --- a/src/rx/rx_conn.h +++ b/src/rx/rx_conn.h @@ -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 */ diff --git a/src/rx/rx_packet.c b/src/rx/rx_packet.c index c52937eac1..6cf997b962 100644 --- a/src/rx/rx_packet.c +++ b/src/rx/rx_packet.c @@ -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) {