From 5ae495ab95c6a8da631c81ecff651288fe514459 Mon Sep 17 00:00:00 2001 From: winlin Date: Mon, 8 Aug 2022 08:22:02 +0800 Subject: [PATCH] For #1229: Check the return value of vsnprintf. --- trunk/src/app/srs_app_log.cpp | 53 +++++++++++++++++----- trunk/src/app/srs_app_rtc_codec.cpp | 2 +- trunk/src/kernel/srs_kernel_error.cpp | 12 +++-- trunk/src/protocol/srs_protocol_log.cpp | 59 ++++++++++++++++++------- 4 files changed, 94 insertions(+), 32 deletions(-) diff --git a/trunk/src/app/srs_app_log.cpp b/trunk/src/app/srs_app_log.cpp index 4f9547f3a..95d355313 100644 --- a/trunk/src/app/srs_app_log.cpp +++ b/trunk/src/app/srs_app_log.cpp @@ -97,10 +97,15 @@ void SrsFileLog::verbose(const char* tag, SrsContextId context_id, const char* f va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); + int r0 = vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); va_end(ap); + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= LOG_MAX_SIZE - size) { + return; + } + size += r0; + write_log(fd, log_data, size, SrsLogLevelVerbose); } @@ -119,10 +124,15 @@ void SrsFileLog::info(const char* tag, SrsContextId context_id, const char* fmt, va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); + int r0 = vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); va_end(ap); + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= LOG_MAX_SIZE - size) { + return; + } + size += r0; + write_log(fd, log_data, size, SrsLogLevelInfo); } @@ -141,10 +151,15 @@ void SrsFileLog::trace(const char* tag, SrsContextId context_id, const char* fmt va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); + int r0 = vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); va_end(ap); + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= LOG_MAX_SIZE - size) { + return; + } + size += r0; + write_log(fd, log_data, size, SrsLogLevelTrace); } @@ -163,10 +178,15 @@ void SrsFileLog::warn(const char* tag, SrsContextId context_id, const char* fmt, va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); + int r0 = vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); va_end(ap); + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= LOG_MAX_SIZE - size) { + return; + } + size += r0; + write_log(fd, log_data, size, SrsLogLevelWarn); } @@ -185,14 +205,25 @@ void SrsFileLog::error(const char* tag, SrsContextId context_id, const char* fmt va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); + int r0 = vsnprintf(log_data + size, LOG_MAX_SIZE - size, fmt, ap); va_end(ap); + + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= LOG_MAX_SIZE - size) { + return; + } + size += r0; // add strerror() to error msg. // Check size to avoid security issue https://github.com/ossrs/srs/issues/1229 if (errno != 0 && size < LOG_MAX_SIZE) { - size += snprintf(log_data + size, LOG_MAX_SIZE - size, "(%s)", strerror(errno)); + r0 = snprintf(log_data + size, LOG_MAX_SIZE - size, "(%s)", strerror(errno)); + + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= LOG_MAX_SIZE - size) { + return; + } + size += r0; } write_log(fd, log_data, size, SrsLogLevelError); diff --git a/trunk/src/app/srs_app_rtc_codec.cpp b/trunk/src/app/srs_app_rtc_codec.cpp index ae04299b1..f8d382b87 100644 --- a/trunk/src/app/srs_app_rtc_codec.cpp +++ b/trunk/src/app/srs_app_rtc_codec.cpp @@ -49,7 +49,7 @@ public: { static char buf[4096] = {0}; int nbytes = vsnprintf(buf, sizeof(buf), fmt, vl); - if (nbytes > 0) { + if (nbytes > 0 && nbytes < sizeof(buf)) { // Srs log is always start with new line, replcae '\n' to '\0', make log easy to read. if (buf[nbytes - 1] == '\n') { buf[nbytes - 1] = '\0'; diff --git a/trunk/src/kernel/srs_kernel_error.cpp b/trunk/src/kernel/srs_kernel_error.cpp index 9f60cbe3f..d89a9d009 100644 --- a/trunk/src/kernel/srs_kernel_error.cpp +++ b/trunk/src/kernel/srs_kernel_error.cpp @@ -101,7 +101,7 @@ SrsCplxError* SrsCplxError::create(const char* func, const char* file, int line, va_list ap; va_start(ap, fmt); static char buffer[4096]; - vsnprintf(buffer, sizeof(buffer), fmt, ap); + int r0 = vsnprintf(buffer, sizeof(buffer), fmt, ap); va_end(ap); SrsCplxError* err = new SrsCplxError(); @@ -111,7 +111,9 @@ SrsCplxError* SrsCplxError::create(const char* func, const char* file, int line, err->line = line; err->code = code; err->rerrno = rerrno; - err->msg = buffer; + if (r0 > 0 && r0 < sizeof(buffer)) { + err->msg = string(buffer, r0); + } err->wrapped = NULL; if (_srs_context) { err->cid = _srs_context->get_id(); @@ -126,7 +128,7 @@ SrsCplxError* SrsCplxError::wrap(const char* func, const char* file, int line, S va_list ap; va_start(ap, fmt); static char buffer[4096]; - vsnprintf(buffer, sizeof(buffer), fmt, ap); + int r0 = vsnprintf(buffer, sizeof(buffer), fmt, ap); va_end(ap); SrsCplxError* err = new SrsCplxError(); @@ -138,7 +140,9 @@ SrsCplxError* SrsCplxError::wrap(const char* func, const char* file, int line, S err->code = v->code; } err->rerrno = rerrno; - err->msg = buffer; + if (r0 > 0 && r0 < sizeof(buffer)) { + err->msg = string(buffer, r0); + } err->wrapped = v; if (_srs_context) { err->cid = _srs_context->get_id(); diff --git a/trunk/src/protocol/srs_protocol_log.cpp b/trunk/src/protocol/srs_protocol_log.cpp index 149f8fee9..38cc03fef 100644 --- a/trunk/src/protocol/srs_protocol_log.cpp +++ b/trunk/src/protocol/srs_protocol_log.cpp @@ -134,9 +134,14 @@ void SrsConsoleLog::verbose(const char* tag, SrsContextId context_id, const char va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); + int r0 = vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); va_end(ap); + + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= SRS_BASIC_LOG_SIZE - size) { + return; + } + size += r0; fprintf(stdout, "%s\n", buffer); } @@ -154,9 +159,14 @@ void SrsConsoleLog::info(const char* tag, SrsContextId context_id, const char* f va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); + int r0 = vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); va_end(ap); + + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= SRS_BASIC_LOG_SIZE - size) { + return; + } + size += r0; fprintf(stdout, "%s\n", buffer); } @@ -174,9 +184,14 @@ void SrsConsoleLog::trace(const char* tag, SrsContextId context_id, const char* va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); + int r0 = vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); va_end(ap); + + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= SRS_BASIC_LOG_SIZE - size) { + return; + } + size += r0; fprintf(stdout, "%s\n", buffer); } @@ -194,9 +209,14 @@ void SrsConsoleLog::warn(const char* tag, SrsContextId context_id, const char* f va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); + int r0 = vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); va_end(ap); + + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= SRS_BASIC_LOG_SIZE - size) { + return; + } + size += r0; fprintf(stderr, "%s\n", buffer); } @@ -214,13 +234,24 @@ void SrsConsoleLog::error(const char* tag, SrsContextId context_id, const char* va_list ap; va_start(ap, fmt); - // we reserved 1 bytes for the new line. - size += vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); + int r0 = vsnprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, fmt, ap); va_end(ap); + + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= SRS_BASIC_LOG_SIZE - size) { + return; + } + size += r0; // add strerror() to error msg. if (errno != 0) { - size += snprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, "(%s)", strerror(errno)); + r0 = snprintf(buffer + size, SRS_BASIC_LOG_SIZE - size, "(%s)", strerror(errno)); + + // Something not expected, drop the log. + if (r0 <= 0 || r0 >= SRS_BASIC_LOG_SIZE - size) { + return; + } + size += r0; } fprintf(stderr, "%s\n", buffer); @@ -277,11 +308,7 @@ bool srs_log_header(char* buffer, int size, bool utc, bool dangerous, const char // Exceed the size, ignore this log. // Check size to avoid security issue https://github.com/ossrs/srs/issues/1229 - if (written >= size) { - return false; - } - - if (written == -1) { + if (written <= 0 || written >= size) { return false; }