[systemd-bugs] [Bug 55213] New: Early boot vsnprintf/vasprintf segfault

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=55213

             Bug #: 55213
           Summary: Early boot vsnprintf/vasprintf segfault
    Classification: Unclassified
           Product: systemd
           Version: unspecified
          Platform: x86 (IA32)
        OS/Version: Linux (All)
            Status: NEW
          Severity: blocker
          Priority: medium
         Component: general
        AssignedTo: systemd-bugs at lists.freedesktop.org
        ReportedBy: jpsinthemix at verizon.net
         QAContact: systemd-bugs at lists.freedesktop.org


Created attachment 67533
  --> https://bugs.freedesktop.org/attachment.cgi?id=67533
Working (but ugly) fix of early boot vsnprintf/vasprintf segfault

Hi,

Beginning in post-systemd-189 git, and still present in systemd-191, there is a
nasty problem in function log_struct_internal() (src/shared/log.c), which
renders my PC unbootable, giving an early boot systemd[1] segfault in
/lib/libc-2.16.90.so, and 'Freezing execution.'

My system: i686-pc-linux-gnu, gcc-4.7.1, glibc-2.16.0, linux-3.6.0-rc6,
systemd-191 (all software built from source).

The segfault is due to a misuse of the vsnprintf() and vasprintf() functions.
Specifically, one must not use va_arg() after calls to vsnprintf() or
vasprintf() as these function/macros internally use va_arg() and may modify it
undefined ways. In particular, from the vsnprintf manpage:

The functions vprintf(), vfprintf(), vsprintf(), vsnprintf() are equivalent to
the functions printf(), fprintf(), sprintf(), snprintf(), respectively, except
that they are called with a va_list instead of a variable number of arguments. 
These  functions  do  not  call  the va_end macro.  Because they invoke the
va_arg macro, the value of ap is undefined after the call.  See stdarg(3),

and from the stdarg manpage:

va_arg()
    ...
    If ap is passed to a function that uses va_arg(ap,type) then the value of
ap is undefined after the return of that function.


In systemd-191/src/shared/log.c, we have the following problematic code in
log_struct_internal():

705                 va_start(ap, format);
706                 while (format && n + 1 < ELEMENTSOF(iovec)) {
707                         char *buf;
708 
709                         if (vasprintf(&buf, format, ap) < 0) {
710                                 r = -ENOMEM;
711                                 goto finish;
712                         }
713 
714                         IOVEC_SET_STRING(iovec[n++], buf);
715 
716                         iovec[n].iov_base = (char*) &nl;
717                         iovec[n].iov_len = 1;
718                         n++;
719 
720                         format = va_arg(ap, char *); <---- use of ap here
is invalid/non-portable
721                 }
722                 va_end(ap);


741                 /* Fallback if journal logging is not available */
742 
743                 va_start(ap, format);
744                 while (format) {
745 
746                         vsnprintf(buf, sizeof(buf), format, ap);
747                         char_array_0(buf);
748 
749                         if (startswith(buf, "MESSAGE=")) {
750                                 found = true;
751                                 break;
752                         }
753 
754                         format = va_arg(ap, char *); <---- use of ap here
is invalid/non-portable
755                 }
756                 va_end(ap);


It appears that the assumption has been made that ap is internally incremented
as values are formatted into buf, but there is no guarantee that this will
occur. Apparently it does occur for some architecture/compiler/libcs, while not
for others. This is truly unfortunate as the vsnprintf/vasprintf-relaled code
now in log_struct_internal() is, in my judgement, quite elegant.

One way around this would be to write code to parse the format strings enough
to figure out the number and types of args associated with each format (tricky
and counter to the use of vsnprintf/vasprintf in the first place, and probably
a pain to maintain and port).

I have attached a patch which 'fixes' this issue in the sense that I'm now up
and running with systemd-191, and all appears to be well.. but it's a bit of a
hack, and admittedly rather ugly. What I've done is to pass each
format-vararglist pair in now occuring in log_struct_internal() calls to a new
function log_struct_entry() which produces the desired string. Then,
log_struct_internal() calls simply have to deal with a vararglist of strings.

hope this helps, and thanks for your time,
John

PS: I've been using systemd since v188 (w/o sysvinit) for desktop/laptops (and
even in ramroots) and love it. The unit file syntax is nice and intuitive and
things mostly work great out-of-the-box. This, is after several years of using
Upstart, which worked adequately, but still was not as flexible as it should've
been, and a was bit fragile..

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.