bin/75258: [patch] dd(1) has not async signal safe interrupt handlers

[Bruce Evans]

The following reply was made to PR bin/75258; it has been noted by GNATS.

From: Bruce Evans <bde at zeta.org.au>
To: "Oleg V. Nauman" <oleg at reis.zp.ua>
Cc: freebsd-gnats-submit at FreeBSD.org
Subject: Re: bin/75258: [patch] dd(1) has not async signal safe interrupt
 handlers
Date: Wed, 29 Dec 2004 01:14:39 +1100 (EST)

 [gnats put back in Cc and everything quoted so that gnats sees it]
 
 On Tue, 28 Dec 2004, Oleg V. Nauman wrote:
 
 > On Fri, Dec 24, 2004 at 02:22:19AM +1100, Bruce Evans wrote:
 > > On Sun, 19 Dec 2004, Oleg V. Nauman wrote:
 > >
 > > >  On Sun, Dec 19, 2004 at 06:11:06PM +0300, Maxim Konovalov wrote:
 > > >  > [...]
 > > >  > > >Description:
 > > >  > > 	dd(1) uses not safe interrupt handlers, they may leads to
 > > >  > > strange problems with dd
 > > >  >=20
 > > >  > Are you sure?  Do you have a testcase?
 > > >
 > > >  	No, sorry.
 > >
 > > However, the patch in the PR would cause strange problems like breaking
 > > termination of dd when the input is from certain slow devices, e.g.,
 > > terminals.  Input is normally restarted after a signal, so just setting
 > > a flag in the signal handler normally doesn't work.  Most "fixes" for
 > > unsafe signal handling get this wrong.  To get this right, something
 > > like the following must be done:
 > > - use sigaction() without SA_RESTART instead of signal(), or use
 > >   siginterrupt() to turn off SA_RESTART.  I've only seen this done
 > >   right for fixing unsafe signal handling in ping(8)  I've seen it
 > >   done wrong in top(1) (try hitting ^C in command mode in top).
 > > - handle the resulting EINTR errors from all syscalls that may now
 > >   fail with this error instead of being restarted.  This is difficult
 > >   or impossible in large programs.  If the signal is checked for and
 > >   acted on after every EINTR, then you have the same problem as acting
 > >   on it directly in signal handlers (the syscall might be in a context
 > >   that doesn't or shouldn't know if it is safe to act), plus
 > >   maintainence problems for checking and acting all over.  If the
 > >   acton is delayed, then the program must somehow be structured to
 > >   ensure that control is passed up to a level that can safely act;
 > >   in particular, no more syscalls that might block can be made.  I
 > >   suppose the latter could be implemented simply but laboriously by
 > >   wrapping all syscalls with
 > >   { if (signalflag) { errno = EINTR; return (-1); } else { normalcall(); }.
 >
 > 	Thank you for your feedback. It looks like I'm could write much more
 > better patch :)
 >
 > >
 > > >  >=20
 > > >  > > >How-To-Repeat:
 > > >  > >
 > > >  > > 	man 2 sigaction
 > > >  >=20
 > > >  > Well, stdio(3) is not signal-safe in general but it seems for me
 > > >  > summary() does not manipulate with the internal state of any file
 > > >  > descriptors (it uses write(2)) and should be safe.
 > > >
 > > >  	But s*printf() family uses malloc(3) for his internal purposes,
 > > >  and there is no any reasons for things like memory allocations in
 > > >  the signal handler, I think.
 > >
 > > s*printf() needs to be signal-safe in practice.  It is careful to use
 > > only local storage for this reason.  Only asprintf() uses malloc().
 >
 > 	It looks like snprintf() calls __vfprintf(), which calls __wcsconv()
 > and __find_arguments() (/usr/src/lib/libc/stdio/vfprintf.c on my
 > RELENG_5 system)
 > These functions directly calls malloc() (without counts how many
 > calls indirectly)
 
 Urk.  vfprintf.c also calls reallocf() directly, and some generic locale
 functions which may call malloc(), and for floating point it calls the
 gdtoa library which uses its own allocator that has much the same issues
 as malloc() (both have locking, but it doesn't help since it is null in
 the non-threaded case and would deadlock if reentered in the threaded
 case).  And dd/misc.c:summary() uses a floating point format.
 
 >
 > >
 > > summary() is obviously attempting to be signal-safe (even in rev.1.1).
 > > It avoids using printf() but depends on snprintf() being signal safe
 > > since the formatting would be too hard otherwise.
 > >
 > > Bruce
 
 I suppose making s*printf() signal-safe again is too hard, but Cc'ed some
 library maintainers to see what they think.  vfprintf.c is apparently
 intended to be signal-safe in 4.4BSD, but things are more complicated
 now.  In 4.4BSD, it has only the following external references:
          U __divdi3
          U __dtoa
          U __fpclassifyd
          U __moddi3
          U __sfvwrite
          U __swsetup
          U __udivdi3
          U __umoddi3
          U abort
          U fflush
          U memchr
 00000308 T vfprintf
 
 Here:
 - __fpclassifyd() is from a macro in current headers and isn't in 4.4BSD.
   It and the other general low-level functions are probably signal-safe.
 - dtoa() is even less signal-safe in 4.4BSD than in -current, since it
   calls malloc() in 4.4BSD.
 - the low-level stdio functions are probably signal-safe for s*printf().
   The main case where they aren't signal-safe is when __swsetup() and/or
   friends call malloc() for file i/o.
 - the standard function abort() is required to be signal-safe in a weak
   sense in C99, but POSIX requires it to flush stdio buffers so it cannot
   reasonably be signal-safe.  FreeBSD implements the POSIX requirement.
   This isn't a problem in 4.4BSD since abort() is only called in cases
   that can't happen.  -current also calls it for malloc() failures.
 - the standard function fflush() is not required to be signal-safe but
   probably is for s*printf() in FreeBSD.
 - the standard function memchr() is not required to be signal-safe but
   probably is in FreeBSD.
 
 The June version of vfprintf.o has the the following external references:
 
          U __divdi3
          U __dtoa
          U __fflush
          U __freedtoa
          U __hdtoa
          U __hldtoa
          U __isthreaded
          U __ldtoa
          U __moddi3
          U __sfvwrite
          U __swsetup
          U __udivdi3
          U __umoddi3
          U _flockfile
          U _funlockfile
          U abort
          U bcopy
          U free
          U localeconv
          U malloc
          U memchr
          U reallocf
          U wcrtomb
          U wcsrtombs
 00000570 T vfprintf
 000005c8 T __vfprintf
 
 The call to localconv() seems to be especially dangerous since it can
 result in updating global locale data.
 
 Bruce