[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