Hello Nick,
On Mon, 29 Jul 2013 09:27:57 -0700
Nick Bartos <nick at pistoncloud.com> wrote:
> I am reporting this bug as requested in the "BUG REPORT" of
> the makedumpfile README.
>
> There are several times when sprintf is used to append to a buffer like so:
> sprintf(buf, "%s.", buf);
>
> In the sprintf manpage, it describes that behavior is not consistent. I
> first noticed this as my hardned system would make all dmesg output lines
> empty. Here is the excerpt from the man page:
>
> C99 and POSIX.1-2001 specify that the results are undefined if a call
> to sprintf(), snprintf(), vsprintf(), or vsnprintf() would cause copying
> to take place between objects that overlap (e.g., if the target string
> array and one of the supplied input arguments refer to the same buffer).
> See NOTES.
>
> NOTES
> Some programs imprudently rely on code such as the following
>
> sprintf(buf, "%s some further text", buf);
>
> to append text to buf. However, the standards explicitly note that
> the results are undefined if source and destination buffers overlap
> when calling sprintf(), snprintf(), vsprintf(), and vsnprintf().
> Depending on the version of gcc(1) used, and the compiler options employed,
> calls such as the above will not produce the expected results.
>
> The glibc implementation of the functions snprintf() and vsnprintf()
> conforms to the C99 standard, that is, behaves as described above, since
> glibc version 2.1. Until glibc 2.0.6 they would return -1 when the output
> was truncated.
>
>
> In addition to using sprintf improperly, it appears that there may be
> buffer overflow potential in the current code, since the loop does not stop
> if the size of buf is exhausted.
>
> I have attached a patch which modifies this behavior.
I have some comments on this patch, please see below.
> diff -U3 -r makedumpfile-1.5.4.orig/makedumpfile.c makedumpfile-1.5.4/makedumpfile.c
> --- makedumpfile-1.5.4.orig/makedumpfile.c 2013-07-01 08:08:49.000000000 +0000
> +++ makedumpfile-1.5.4/makedumpfile.c 2013-07-22 22:47:34.195721706 +0000
> @@ -3701,6 +3701,7 @@
> char *msg, *p;
> unsigned int i, text_len;
> unsigned long long ts_nsec;
> + int buf_s;
> char buf[BUFSIZE];
> ulonglong nanos;
> ulong rem;
> @@ -3713,21 +3714,49 @@
>
> msg = logptr + SIZE(log);
>
> - sprintf(buf, "[%5lld.%06ld] ", nanos, rem/1000);
> + buf_s = snprintf(buf, BUFSIZE, "[%5lld.%06ld] ", nanos, rem/1000);
> + if (buf_s < 0) {
> + DEBUG_MSG("snprintf returned an error in dump_log_entry.\n");
> + return FALSE;
> + }
This is the error case, so you should use ERRMSG instead of DEBUG_MSG.
The same can be said about all other messages.
> + if (buf_s >= BUFSIZE) {
> + DEBUG_MSG("snprintf truncated output in dump_log_entry.\n");
> + DEBUG_MSG(" increase BUFSIZE and recompile.\n");
> + /* No point in continuing, we can't append anything useful */
> + return FALSE;
> + }
> +
> + /* It's unlikely but possible we may not have enough to terminate the
> + string */
> + if (buf_s >= BUFSIZE - 1) {
> + DEBUG_MSG("not enough buffer space in dump_log_entry.\n");
> + DEBUG_MSG(" increase BUFSIZE and recompile.\n");
> + return FALSE;
> + }
> +
> + for (i = 0, p = msg; i < text_len; i++, p++, buf_s++) {
> + if (buf_s >= BUFSIZE - 2) {
> + DEBUG_MSG("output truncated in dump_log_entry.\n");
> + DEBUG_MSG(" increase BUFSIZE and recompile.\n");
> + break;
> + }
I don't think to check the remaining buffer space 3 times is meaningful,
they indicate just "lack of buffer space".
Is the last one enough to check the remaining buffer space ?
>
> - for (i = 0, p = msg; i < text_len; i++, p++) {
> if (*p == '\n')
> - sprintf(buf, "%s.", buf);
> + buf[buf_s] = '.';
> else if (isprint(*p) || isspace(*p))
> - sprintf(buf, "%s%c", buf, *p);
> + buf[buf_s] = *p;
> else
> - sprintf(buf, "%s.", buf);
> + buf[buf_s] = '.';
> }
>
> - sprintf(buf, "%s\n", buf);
> + buf[buf_s] = '\n';
> + buf_s++;
> + buf[buf_s] = NULL;
Null character(\0) should be used to terminate strings instead of
null pointer.
>
> - if (write(info->fd_dumpfile, buf, strlen(buf)) < 0)
> + if (write(info->fd_dumpfile, buf, strlen(buf)) < 0) {
> + DEBUG_MSG("Problem writing to dump file.\n");
> return FALSE;
> + }
> else
> return TRUE;
> }
Thanks
Atsushi Kumagai
