[PATCH v2 00/14] Fix signal-unsafe logging

[Peter Hutterer]

On Fri, Apr 13, 2012 at 11:22:14AM +0200, Michal Suchanek wrote:
> On 13 April 2012 03:09, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> > On Wed, Apr 11, 2012 at 06:33:26PM -0700, Chase Douglas wrote:
> >> On 04/11/2012 04:53 PM, Peter Hutterer wrote:
> >> > On Mon, Apr 09, 2012 at 11:17:26AM -0700, Chase Douglas wrote:
> >> >> This new patch set includes a few fixes to the first and many more fixed
> >> >> logging paths. All paths in the server that occur when it encounters a
> >> >> fatal signal are now handled properly.
> >> >>
> >> >> I do not think this affects the crash I have been seeing in the Ubuntu
> >> >> server. I tracked that issue down to a bug in the synaptics input
> >> >> module. Since the only known issue is that running the server under
> >> >> valgrind may fail if messages are logged inappropriately, I suggest we
> >> >> not apply this to any stable branches.
> >> >>
> >> >> The full tree is still maintained at:
> >> >>
> >> >> http://cgit.freedesktop.org/~cndougla/xserver/log/?h=signal-logging
> >> >>
> >> >> Thanks!
> >> >
> >> > Looks good and this is certainly needed. I do have a few comments though:
> >> >
> >> > - I'd really like to see some tests of these new functions. Should be easy
> >> >   enough to write, given that they're all exposed and you can use strlen,
> >> >   strcmp, etc. to compare results.
> >> >   Plus, you should be able to test them inside a signal handler too with
> >> >   little effort.
> >>
> >> Some of the functions definitely could use tests, like strlen_sigsafe().
> >> I'll add some.
> >>
> >> However, I don't know if it's possible to add tests for behavior inside
> >> a signal handler. For a test to be useful it must be repeatable and fail
> >> every time it's wrong. Usually signal context corruption is
> >> indeterminate. Maybe it could be repeatable with the use of valgrind,
> >> but even then I'm not 100% sure.
> >>
> >> > - AFAICT, every single caller to LogMessageVerbSigSafe uses -1 as verbosity.
> >> >   How about a LogMessageSigSafe wrapper?
> >>
> >> There are a few places where I replaced non-ErrorF calls, where the
> >> verbosity remains 1 instead of -1.
> >>
> >> I thought about creating a non-verb wrapper, but I didn't really have
> >> much of an interest in reproducing all the different logging macros and
> >> variations we currently have. The idea is that logging in signal context
> >> should be fairly infrequent, and if you need it then you can get along
> >> with all the args.
> >
> > It should be infrequent, but any error between ReadInput and mieqEnqueue will
> > require it. That may not happen often, but the code still needs to be there.
> > So a sensible API is something we should definitely aim for.
> >
> >> Secondarily, what would you default to? ErrorF uses -1, but LogMessage
> >> uses 1.
> >
> > There's a reason that ErrorF is different (albeit just a wrapper) to
> > LogMessage - it makes the code more obvious. A LogMessageVerb(..., -1, ...)
> > is much less obvious and grep-able than ErrorF(). So at least having
> > ErrorFSigSafe() is a good thing to have. It would make any new code stick
> > out. "Hey, it's ErrorFSigSafe up there, but ErrorF here - one of them is
> > wrong".
> >
> >> > - the API is rather shocking, replacing one vararg function with a number of
> >> >   mixed calls makes it really hard to grep for the message.
> >> >   a poor man's printf parsing that searches for %d, %s and %x would help
> >> >   here.
> >>
> >> Variadic argument implementation is some of the blackest magic of C
> >> code, and varies a great deal even between architectures of glibc. I
> >> don't know if we can be sure that variadic arguments will always be
> >> signal safe. They aren't mentioned in the POSIX signal(7) man page as
> >> safe functions.
> >>
> >> I doubt we would actually support Interix SUA, for example, but it is
> >> POSIX compliant and its man page says va_* functions are not signal
> >> safe: http://www.suacommunity.com/man/3/varargs.3.html.
> >
> > yeah, unfortunately that's the only documentation I found too, nothing else
> > seems to hints at whether it's signal safe or not.
> >
> ...
> >  typedef union {
> >      uint32_t ui;
> >      int32_t i;
> >      char *c;
> >      const char *str;
> >      void *ptr;
> >  } log_param_t;
> >
> >  /* Log a message using only signal safe functions. */
> >  void
> >  LogMessageVerbSigSafe(MessageType type, int verb, const char *message,
> >                        log_param_t* param)
> ...
> > Instead of varargs, the callers need to supply a log_param_t array, which is
> > painful, but hopefully better than splitting log messages over several commands.
> >
> I don't think you can initialize an array of log_param_t sanely in
> standard C. Unless you are going to have gcc as hard dependency this
> is probably going to be more painful than just using multiple
> statements.

I was under the impression that the union is the size of the biggest member,
so 4 or 8 bytes in this case, depending on sizeof(void*). Isn't that the
case here?

Cheers,
  Peter