[Asterisk-code-review] thread safety: Don't use getprotobyname() (asterisk[master])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5251 )

Change subject: thread safety: Don't use getprotobyname()
......................................................................


thread safety: Don't use getprotobyname()

POSIX does not require getprotobyname() to be thread safe and some
implementations use static memory which causes issues when multiple
threads are used.

Further, our usage of it today is just to ultimately get IPPROTO_TCP
for calls to setsockopt(). So instead we just use IPPROTO_TCP directly.

Change-Id: I2e14e58674808f7ce99b2f5e900d0f90d0d8da48
---
M channels/chan_skinny.c
M include/asterisk/network.h
M main/http.c
M main/manager.c
4 files changed, 11 insertions(+), 10 deletions(-)

Approvals:
  George Joseph: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/channels/chan_skinny.c b/channels/chan_skinny.c
index a3a2f87..d1c2b92 100644
--- a/channels/chan_skinny.c
+++ b/channels/chan_skinny.c
@@ -7639,7 +7639,6 @@
 	struct sockaddr_in sin;
 	socklen_t sinlen;
 	struct skinnysession *s;
-	struct protoent *p;
 	int arg = 1;
 
 	for (;;) {
@@ -7656,12 +7655,10 @@
 			continue;
 		}
 
-		p = getprotobyname("tcp");
-		if(p) {
-			if( setsockopt(as, p->p_proto, TCP_NODELAY, (char *)&arg, sizeof(arg) ) < 0 ) {
-				ast_log(LOG_WARNING, "Failed to set Skinny tcp connection to TCP_NODELAY mode: %s\n", strerror(errno));
-			}
+		if (setsockopt(as, IPPROTO_TCP, TCP_NODELAY, (char *) &arg, sizeof(arg)) < 0) {
+			ast_log(LOG_WARNING, "Failed to set TCP_NODELAY on Skinny TCP connection: %s\n", strerror(errno));
 		}
+
 		if (!(s = ast_calloc(1, sizeof(struct skinnysession)))) {
 			close(as);
 			ast_atomic_fetchadd_int(&unauth_sessions, -1);
diff --git a/include/asterisk/network.h b/include/asterisk/network.h
index 3371e58..5216f4c 100644
--- a/include/asterisk/network.h
+++ b/include/asterisk/network.h
@@ -86,6 +86,11 @@
 #endif
 #define inet_ntoa __dont__use__inet_ntoa__use__ast_inet_ntoa__instead__
 
+#ifdef getprotobyname
+#undef getprotobyname
+#endif
+#define getprotobyname __getprotobyname_is_not_threadsafe__do_not_use__
+
 /*! \brief Compares the source address and port of two sockaddr_in */
 static force_inline int inaddrcmp(const struct sockaddr_in *sin1, const struct sockaddr_in *sin2)
 {
diff --git a/main/http.c b/main/http.c
index 0db6ee7..ea85a28 100644
--- a/main/http.c
+++ b/main/http.c
@@ -1917,9 +1917,8 @@
 	 * This is necessary to prevent delays (caused by buffering) as we
 	 * write to the socket in bits and pieces.
 	 */
-	if (setsockopt(ast_iostream_get_fd(ser->stream), IPPROTO_TCP, TCP_NODELAY, (char *) &arg, sizeof(arg) ) < 0) {
+	if (setsockopt(ast_iostream_get_fd(ser->stream), IPPROTO_TCP, TCP_NODELAY, (char *) &arg, sizeof(arg)) < 0) {
 		ast_log(LOG_WARNING, "Failed to set TCP_NODELAY on HTTP connection: %s\n", strerror(errno));
-		ast_log(LOG_WARNING, "Some HTTP requests may be slow to respond.\n");
 	}
 	ast_iostream_nonblock(ser->stream);
 
diff --git a/main/manager.c b/main/manager.c
index eae1ca5..c1d73dc 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -6647,8 +6647,8 @@
 	/* here we set TCP_NODELAY on the socket to disable Nagle's algorithm.
 	 * This is necessary to prevent delays (caused by buffering) as we
 	 * write to the socket in bits and pieces. */
-	if (setsockopt(ast_iostream_get_fd(ser->stream), IPPROTO_TCP, TCP_NODELAY, (char *)&arg, sizeof(arg) ) < 0) {
-		ast_log(LOG_WARNING, "Failed to set manager tcp connection to TCP_NODELAY mode: %s\nSome manager actions may be slow to respond.\n", strerror(errno));
+	if (setsockopt(ast_iostream_get_fd(ser->stream), IPPROTO_TCP, TCP_NODELAY, (char *) &arg, sizeof(arg)) < 0) {
+		ast_log(LOG_WARNING, "Failed to set TCP_NODELAY on manager connection: %s\n", strerror(errno));
 	}
 	ast_iostream_nonblock(ser->stream);
 

-- 
To view, visit https://gerrit.asterisk.org/5251
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I2e14e58674808f7ce99b2f5e900d0f90d0d8da48
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>