Date: Tue, 28 Dec 2004 14:20:30 GMT From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs@FreeBSD.org Subject: Re: bin/75258: [patch] dd(1) has not async signal safe interrupt handlers Message-ID: <200412281420.iBSEKUhK059344@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/75258; it has been noted by GNATS.
From: Bruce Evans <bde@zeta.org.au>
To: "Oleg V. Nauman" <oleg@reis.zp.ua>
Cc: freebsd-gnats-submit@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200412281420.iBSEKUhK059344>