[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]

 



Sorry for late reply.

On Tue, 6 Aug 2013 08:56:56 -0700
Nick Bartos <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> wrote:
> 
> > 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
> >



[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