On Mon, Nov 11, 2013 at 8:50 PM, Eric Blake <ebl...@redhat.com> wrote: > Quick - identify the bug in this code (from ui/curses.c): > > static void curses_winch_handler(int signum) > { > struct winsize { > unsigned short ws_row; > unsigned short ws_col; > unsigned short ws_xpixel; /* unused */ > unsigned short ws_ypixel; /* unused */ > } ws; > > /* terminal size changed */ > if (ioctl(1, TIOCGWINSZ, &ws) == -1)
An unsafe function is called in a signal. See man 7 signal, section 'Async-signal-safe functions'. This should be avoided. > return; > > resize_term(ws.ws_row, ws.ws_col); > curses_calc_pad(); > invalidate = 1; > > /* some systems require this */ > signal(SIGWINCH, curses_winch_handler); > } > > Here's a hint: ioctl() can clobber errno. I believe it cannot, at least in linux, as technically the signal handler is always called in a new thread, specifically created to only handle that signal, and errno should be thread-local. > But if a signal handler is > called in the middle of other code that is using errno, then the handler > MUST restore the value of errno before returning, if it is to guarantee > that the interrupted context won't be corrupted. > > More reading on the topic: > https://plus.google.com/+LennartPoetteringTheOneAndOnly/posts/gHSscCJkakd > > I have not done a full audit of qemu's signal handlers, so much as a > quick look to see if I could find violations; it was surprisingly easy > to find a bad example. A signal handler that resets the signal to > SIG_DFL then calls raise() is exempt from caring about errno, but any > signal handler that can fall through to the end and return execution to > the caller MUST ensure that errno is left unchanged, for errno to be > useful in the remaining body of code. Which is why the best signal > handlers tend to be the one that only flag a volatile variable that is > later checked at safe points of execution, rather than trying to make > complex calls from within the handler context. -- Thanks. -- Max