From 949e9147bfed119fb04cadd2b6916fe9bfc1d9b0 Mon Sep 17 00:00:00 2001 From: George Joseph Date: Thu, 13 Apr 2017 10:14:48 -0600 Subject: 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 --- channels/chan_skinny.c | 122 ++++++++++++++++++++++++++----------------------- 1 file changed, 66 insertions(+), 56 deletions(-) (limited to 'channels/chan_skinny.c') diff --git a/channels/chan_skinny.c b/channels/chan_skinny.c index a0530007b..aeb060569 100644 --- a/channels/chan_skinny.c +++ b/channels/chan_skinny.c @@ -6658,7 +6658,7 @@ static int handle_capabilities_res_message(struct skinny_req *req, struct skinny #ifdef AST_DEVMODE struct ast_str *codec_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN); #endif - + if (!codecs) { return 0; @@ -7499,6 +7499,8 @@ static void skinny_session_cleanup(void *data) destroy_session(s); } +#define PACKET_TIMEOUT 10000 + static void *skinny_session(void *data) { int res; @@ -7545,78 +7547,86 @@ static void *skinny_session(void *data) } } - if (fds[0].revents) { + 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; - } + 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; + 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; + 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; - } + 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; - } + 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_debug(1, "Read header: Message ID: 0x%04x, %d bytes in packet\n", eventmessage, dlen); - 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; + 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; } - s->lockstate = 0; - ast_mutex_unlock(&s->lock); + 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); + } - pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL); - res = handle_message(req, s); - pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL); + 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); req = NULL; -- cgit v1.2.3