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

[Michal Suchanek]

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.

Thanks

Michal