FYI, I've just rebased these, in case this is a better time to apply them: [1/3] logsys.c: factor out some duplication [2/3] logsys.c: avoid redundant strlen in else-block [3/3] logsys.c: avoid possibility of buffer overrun
>From 8e611ca850c60698ac32aeffa2d815abb89d9948 Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Thu, 23 Apr 2009 18:01:37 +0200 Subject: [PATCH 1/3] logsys.c: factor out some duplication * exec/logsys.c (log_printf_to_logs): Factor out repeated calls to strcpy_cutoff. --- exec/logsys.c | 33 +++++++++++++++++---------------- 1 files changed, 17 insertions(+), 16 deletions(-) diff --git a/exec/logsys.c b/exec/logsys.c index 0779cbd..c4dace5 100644 --- a/exec/logsys.c +++ b/exec/logsys.c @@ -434,6 +434,8 @@ static void log_printf_to_logs ( while (format_buffer[format_buffer_idx]) { cutoff = -1; if (format_buffer[format_buffer_idx] == '%') { + const char *p; + format_buffer_idx += 1; if (isdigit (format_buffer[format_buffer_idx])) { cutoff = atoi (&format_buffer[format_buffer_idx]); @@ -444,41 +446,40 @@ static void log_printf_to_logs ( switch (format_buffer[format_buffer_idx]) { case 's': - len = strcpy_cutoff (&output_buffer[output_buffer_idx], subsys, cutoff); - output_buffer_idx += len; + p = subsys; break; case 'n': - len = strcpy_cutoff (&output_buffer[output_buffer_idx], function_name, cutoff); - output_buffer_idx += len; + p = function_name; break; case 'f': - len = strcpy_cutoff (&output_buffer[output_buffer_idx], file_name, cutoff); - output_buffer_idx += len; + p = file_name; break; case 'l': sprintf (line_no, "%d", file_line); - len = strcpy_cutoff (&output_buffer[output_buffer_idx], line_no, cutoff); - output_buffer_idx += len; - break; - - case 'p': + p = line_no; break; case 't': gettimeofday (&tv, NULL); (void)strftime (char_time, sizeof (char_time), "%b %d %T", localtime ((time_t *)&tv.tv_sec)); - len = strcpy_cutoff (&output_buffer[output_buffer_idx], char_time, cutoff); - output_buffer_idx += len; + p = char_time; break; case 'b': - len = strcpy_cutoff (&output_buffer[output_buffer_idx], buffer, cutoff); - output_buffer_idx += len; + p = buffer; + break; + + case 'p': + default: + p = ""; break; } + len = strcpy_cutoff (&output_buffer[output_buffer_idx], + p, cutoff); + output_buffer_idx += len; format_buffer_idx += 1; } else { output_buffer[output_buffer_idx++] = format_buffer[format_buffer_idx++]; @@ -684,7 +685,7 @@ static void *logsys_worker_thread (void *data) */ for (;;) { int yield_counter = 1; - + logsys_lock(); if (log_requests_lost > 0) { printf ("lost %d log requests\n", log_requests_lost); -- 1.6.3.76.g775d >From dea96692e5743e82e77ae59723b64d4ec8d60749 Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Thu, 23 Apr 2009 18:38:53 +0200 Subject: [PATCH 2/3] logsys.c: avoid redundant strlen in else-block * exec/logsys.c (strcpy_cutoff): Also, with a field width (aka cutoff), and a shorter-than-field-width string, don't write the same memory twice: once with strncpy using NUL bytes, then again with spaces via the memset. --- exec/logsys.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-) diff --git a/exec/logsys.c b/exec/logsys.c index c4dace5..5e97741 100644 --- a/exec/logsys.c +++ b/exec/logsys.c @@ -65,6 +65,8 @@ #define YIELD_AFTER_LOG_OPS 10 +#define MIN(x,y) ((x) < (y) ? (x) : (y)) + /* * similar to syslog facilities/priorities tables, * make a tag table for internal use @@ -381,18 +383,15 @@ do { \ */ static inline int strcpy_cutoff (char *dest, const char *src, int cutoff) { + size_t len = strlen (src); if (cutoff <= 0) { - size_t len = strlen (src); memcpy (dest, src, len + 1); return (len); } else { - size_t len; - strncpy (dest, src, cutoff); + len = MIN (len, cutoff); + memcpy (dest, src, len); + memset (dest + len, ' ', cutoff - len); dest[cutoff] = '\0'; - len = strlen (dest); - if (len != cutoff) { - memset (&dest[len], ' ', cutoff - len); - } } return (cutoff); } -- 1.6.3.76.g775d >From c859c6a4d313505d02b33c588bee89863c2d36ae Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Thu, 23 Apr 2009 19:45:43 +0200 Subject: [PATCH 3/3] logsys.c: avoid possibility of buffer overrun * exec/logsys.c (strcpy_cutoff): Add buf_len parameter, and never write more than this number of bytes into "dest". Change type of "cutoff" parameter from int to size_t. Sentinel value changes from -1 to 0. (log_printf_to_logs): Adapt to those changes. Reverse condition of test so the much-shorter block is the "if-block". Factor out a common subexpression for readability. Exit the loop if output_buffer_idx ever reaches sizeof(output_buffer). --- exec/logsys.c | 49 ++++++++++++++++++++++++++++++++----------------- 1 files changed, 32 insertions(+), 17 deletions(-) diff --git a/exec/logsys.c b/exec/logsys.c index 5e97741..72ae9fa 100644 --- a/exec/logsys.c +++ b/exec/logsys.c @@ -381,18 +381,26 @@ do { \ /* * Internal threaded logging implementation */ -static inline int strcpy_cutoff (char *dest, const char *src, int cutoff) +static inline int strcpy_cutoff (char *dest, const char *src, size_t cutoff, + size_t buf_len) { size_t len = strlen (src); - if (cutoff <= 0) { - memcpy (dest, src, len + 1); - return (len); - } else { - len = MIN (len, cutoff); - memcpy (dest, src, len); - memset (dest + len, ' ', cutoff - len); - dest[cutoff] = '\0'; + if (buf_len <= 1) { + if (buf_len == 0) + dest[0] = 0; + return 0; + } + + if (cutoff == 0) { + cutoff = len; } + + cutoff = MIN (cutoff, buf_len - 1); + len = MIN (len, cutoff); + memcpy (dest, src, len); + memset (dest + len, ' ', cutoff - len); + dest[cutoff] = '\0'; + return (cutoff); } @@ -421,7 +429,7 @@ static void log_printf_to_logs ( unsigned int format_buffer_idx = 0; unsigned int output_buffer_idx = 0; struct timeval tv; - int cutoff; + size_t cutoff; unsigned int len; int subsysid; @@ -430,9 +438,13 @@ static void log_printf_to_logs ( return; } - while (format_buffer[format_buffer_idx]) { - cutoff = -1; - if (format_buffer[format_buffer_idx] == '%') { + int c; + while ((c = format_buffer[format_buffer_idx])) { + cutoff = 0; + if (c != '%') { + output_buffer[output_buffer_idx++] = c; + format_buffer_idx++; + } else { const char *p; format_buffer_idx += 1; @@ -476,12 +488,15 @@ static void log_printf_to_logs ( p = ""; break; } - len = strcpy_cutoff (&output_buffer[output_buffer_idx], - p, cutoff); + len = strcpy_cutoff (output_buffer + output_buffer_idx, + p, cutoff, + (sizeof (output_buffer) + - output_buffer_idx)); output_buffer_idx += len; format_buffer_idx += 1; - } else { - output_buffer[output_buffer_idx++] = format_buffer[format_buffer_idx++]; + } + if (output_buffer_idx == sizeof (output_buffer)) { + break; } } -- 1.6.3.76.g775d _______________________________________________ Openais mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/openais
