[PATCH] makedumpfile-1.5.4 dump dmesg may not work due to improper sprintf usage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Nick,

(2013/08/16 1:29), Nick Bartos wrote:
> You do bring up an interesting point.  What do you think about just using
> the buffer to handle writing the timestamp (which could actually be a smaller
>  buffer than is used now), then writing the rest of the message 1 character
>  at a time as it is read?  That will make the process take a little bit longer
>  due to inefficient use of i/o, however in this case I do not believe it will
> take much longer since dmesg is not that big.  Additionally this would fix the
> problem of possible message truncation (and of make the code simpler),
>  which I think is worth the small speed penalty.

I prefer to use the buffer to reduce the system call count.
Additionally, the current buffer size is only 1k bytes, so I don't think
to reduce memory consumption smaller than now is meaningful.


Thanks
Atsushi Kumagai

>
> On Thu, Aug 15, 2013 at 12:56 AM, Atsushi Kumagai <kumagai-atsushi at mxc.nes.nec.co.jp <mailto:kumagai-atsushi at mxc.nes.nec.co.jp>> wrote:
>
>     Sorry for late reply.
>
>     On Tue, 6 Aug 2013 08:56:56 -0700
>     Nick Bartos <nick at pistoncloud.com <mailto:nick at pistoncloud.com>> wrote:
>
>      > Yes you are correct on changing DEBUG_MSG to ERRMSG.  I didn't read enough
>      > of the code to see that was an option.
>      >
>      > Each buffer size check does something specific, which was intended to
>      > handle all failure cases.  While it's probably possible to do the same
>      > checks in less actual code, simply removing any of the checks will make it
>      > fail in some cases.  Also, if someone sets BUFSIZE to a very small number
>      > when nothing will work, it makes sense to exit with failure without trying
>      > to read anything.  However if only a few lines are going to be truncated
>      > because they are too long, it makes sense to continue on and get as much
>      > debug output as possible (but with printing the error message about
>      > truncation).
>
>     I also think it's good to output messages as much as possible even if they
>     are truncated.
>
>     Now, I hit upon another idea. How about dividing the read and write processing
>     in multiple parts like below? This way can treat any length message with
>     fixed BUFSIZE without truncation.
>
>
>     message      [  112.919861] sd 2:1:2:0: [sdf] Unhandled error code\n\0
>
>                   |---- BUFSIZE -----|---- BUFSIZE -----|---- BUFSIZE -----|
>     read/write            1                  2                  3
>     times
>
>
>     If that is OK with you, could you post your v2 patch ?
>
>
>     Thanks
>     Atsushi Kumagai
>
>      > Good catch on using a null character, that makes more sense as the former
>      > would likely result in a compiler warning.
>      >
>      >
>      > On Mon, Aug 5, 2013 at 9:15 PM, Atsushi Kumagai <
>      > kumagai-atsushi at mxc.nes.nec.co.jp <mailto:kumagai-atsushi at mxc.nes.nec.co.jp>> wrote:
>      >
>      > > Hello Nick,
>      > >
>      > > On Mon, 29 Jul 2013 09:27:57 -0700
>      > > Nick Bartos <nick at pistoncloud.com <mailto: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
>      > >
>
>



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux