[Asterisk-code-review] AST-2017-004: chan skinny: Add EOF check in skinny session (asterisk[certified/13.13])

Matthew Fredrickson asteriskteam at digium.com
Fri May 19 15:11:26 CDT 2017


Matthew Fredrickson has submitted this change and it was merged. ( https://gerrit.asterisk.org/5670 )

Change subject: AST-2017-004: chan_skinny:  Add EOF check in skinny_session
......................................................................


AST-2017-004: chan_skinny:  Add EOF check in skinny_session

The while(1) loop in skinny_session wasn't checking for EOF so
a packet that was longer than a header but still truncated
would spin the while loop infinitely.  Not only does this
permanently tie up a thread and drive a core to 100% utilization,
the call of ast_log() in such a tight loop eats all available
process memory.

Added poll with timeout to top of read loop

ASTERISK-26940 #close
Reported-by: Sandro Gauci

Change-Id: I2ce65f3c5cb24b4943a9f75b64d545a1e2cd2898
---
M channels/chan_skinny.c
1 file changed, 81 insertions(+), 71 deletions(-)

Approvals:
  George Joseph: Looks good to me, but someone else must approve; Verified
  Matthew Fredrickson: Looks good to me, approved; Approved for Submit



diff --git a/channels/chan_skinny.c b/channels/chan_skinny.c
index 4565591..1054ced 100644
--- a/channels/chan_skinny.c
+++ b/channels/chan_skinny.c
@@ -6646,7 +6646,7 @@
 #ifdef AST_DEVMODE
 	struct ast_str *codec_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN);
 #endif
-	
+
 
 	if (!codecs) {
 		return 0;
@@ -7487,6 +7487,8 @@
 	destroy_session(s);
 }
 
+#define PACKET_TIMEOUT 10000
+
 static void *skinny_session(void *data)
 {
 	int res;
@@ -7533,77 +7535,85 @@
 			}
 		}
 
-		if (fds[0].revents) {
-
-			if (!(req = ast_calloc(1, SKINNY_MAX_PACKET))) {
-				ast_log(LOG_WARNING, "Unable to allocated memorey for skinny_req.\n");
-				break;
-			}
-
-			ast_mutex_lock(&s->lock);
-			s->lockstate = 1;
-
-			if ((res = read(s->fd, req, skinny_header_size)) != skinny_header_size) {
-				if (res < 0) {
-					ast_log(LOG_WARNING, "Header read() returned error: %s\n", strerror(errno));
-				} else {
-					ast_log(LOG_WARNING, "Unable to read header. Only found %d bytes.\n", res);
-				}
-				break;
-			}
-
-			eventmessage = letohl(req->e);
-			if (eventmessage < 0) {
-				ast_log(LOG_ERROR, "Event Message is NULL from socket %d, This is bad\n", s->fd);
-				break;
-			}
-
-			dlen = letohl(req->len) - 4;
-			if (dlen < 0) {
-				ast_log(LOG_WARNING, "Skinny Client sent invalid data.\n");
-				break;
-			}
-			if (dlen > (SKINNY_MAX_PACKET - skinny_header_size)) {
-				ast_log(LOG_WARNING, "Skinny packet too large (%d bytes), max length(%d bytes)\n", dlen+8, SKINNY_MAX_PACKET);
-				break;
-			}
-
-			bytesread = 0;
-			while (1) {
-				if ((res = read(s->fd, ((char*)&req->data)+bytesread, dlen-bytesread)) < 0) {
-					ast_log(LOG_WARNING, "Data read() returned error: %s\n", strerror(errno));
-					break;
-				}
-				bytesread += res;
-				if (bytesread >= dlen) {
-					if (res < bytesread) {
-						ast_log(LOG_WARNING, "Rest of partial data received.\n");
-					}
-					if (bytesread > dlen) {
-						ast_log(LOG_WARNING, "Client sent wrong amount of data (%d), expected (%d).\n", bytesread, dlen);
-						res = -1;
-					}
-					break;
-				}
-
-				ast_log(LOG_WARNING, "Partial data received, waiting (%d bytes read of %d)\n", bytesread, dlen);
-				if (sched_yield() < 0) {
-					ast_log(LOG_WARNING, "Data yield() returned error: %s\n", strerror(errno));
-					res = -1;
-					break;
-				}
-			}
-
-			s->lockstate = 0;
-			ast_mutex_unlock(&s->lock);
-			if (res < 0) {
-				break;
-			}
-
-			pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL);
-			res = handle_message(req, s);
-			pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
+		if (!fds[0].revents) {
+			continue;
 		}
+		ast_debug(1, "Reading header\n");
+
+		if (!(req = ast_calloc(1, SKINNY_MAX_PACKET))) {
+			ast_log(LOG_WARNING, "Unable to allocated memorey for skinny_req.\n");
+			break;
+		}
+
+		ast_mutex_lock(&s->lock);
+		s->lockstate = 1;
+
+		if ((res = read(s->fd, req, skinny_header_size)) != skinny_header_size) {
+			if (res < 0) {
+				ast_log(LOG_WARNING, "Header read() returned error: %s\n", strerror(errno));
+			} else {
+				ast_log(LOG_WARNING, "Unable to read header. Only found %d bytes.\n", res);
+			}
+			break;
+		}
+
+		eventmessage = letohl(req->e);
+		if (eventmessage < 0) {
+			ast_log(LOG_ERROR, "Event Message is NULL from socket %d, This is bad\n", s->fd);
+			break;
+		}
+
+		dlen = letohl(req->len) - 4;
+		if (dlen < 0) {
+			ast_log(LOG_WARNING, "Skinny Client sent invalid data.\n");
+			break;
+		}
+		if (dlen > (SKINNY_MAX_PACKET - skinny_header_size)) {
+			ast_log(LOG_WARNING, "Skinny packet too large (%d bytes), max length(%d bytes)\n", dlen+8, SKINNY_MAX_PACKET);
+			break;
+		}
+
+		ast_debug(1, "Read header: Message ID: 0x%04x,  %d bytes in packet\n", eventmessage, dlen);
+
+		bytesread = 0;
+		while (bytesread < dlen) {
+			ast_debug(1, "Waiting %dms for %d bytes of %d\n", PACKET_TIMEOUT, dlen - bytesread, dlen);
+			fds[0].revents = 0;
+			res = ast_poll(fds, 1, PACKET_TIMEOUT);
+			if (res <= 0) {
+				if (res == 0) {
+					ast_debug(1, "Poll timed out waiting for %d bytes\n", dlen - bytesread);
+				} else {
+					ast_log(LOG_WARNING, "Poll failed waiting for %d bytes: %s\n",
+						dlen - bytesread, strerror(errno));
+				}
+				ast_verb(3, "Ending Skinny session from %s (bad input)\n", ast_inet_ntoa(s->sin.sin_addr));
+				res = -1;
+
+				break;
+			}
+			if (!fds[0].revents) {
+				continue;
+			}
+
+			res = read(s->fd, ((char*)&req->data)+bytesread, dlen-bytesread);
+			if (res < 0) {
+				ast_log(LOG_WARNING, "Data read() returned error: %s\n", strerror(errno));
+				break;
+			}
+			bytesread += res;
+			ast_debug(1, "Read %d bytes.  %d of %d now read\n", res, bytesread, dlen);
+		}
+
+		s->lockstate = 0;
+		ast_mutex_unlock(&s->lock);
+		if (res < 0) {
+			break;
+		}
+
+		pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL);
+		res = handle_message(req, s);
+		pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
 
 		if (req) {
 			ast_free(req);

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2ce65f3c5cb24b4943a9f75b64d545a1e2cd2898
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.13
Gerrit-Owner: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>



More information about the asterisk-code-review mailing list