tty: set_termios/set_termiox should not return -EINTR

Hello.

The patch looks really trivial and I even cc'ed -stable.

But it needs the review from someone who understands the drivers/tty code, perhaps there is a reason why we should not restart TCGETA/etc. I do not even know what set_termios() actually does ;)

Oleg.

See https://bugzilla.redhat.com/show_bug.cgi?id=904907
read command causes bash to abort with double free or corruption (out).

A simple test-case from Roman:

	// Compile the reproducer and send sigchld ti that process.
	// EINTR occurs even if SA_RESTART flag is set.

	void handler(int sig)
	{
	}

	main()
	{
	  struct sigaction act;
	  act.sa_handler = handler;
	  act.sa_flags = SA_RESTART;
	  sigaction (SIGCHLD, &act, 0);
	  struct termio ttp;
	  ioctl(0, TCGETA, &ttp);
	  while(1)
	  {
	    if (ioctl(0, TCSETAW, ttp) < 0)
	      {
		if (errno == EINTR)
		{
		  fprintf(stderr, "BUG!"); return(1);
		}
	      }
	  }
	}

Change set_termios/set_termiox to return -ERESTARTSYS to fix this particular problem.

I didn't dare to change other EINTR's in drivers/tty/, but they look equally wrong.

---

@@ -617,7 +617,7 @@ static int set_termios(struct tty_struct *tty, void __user *arg, int opt)
 	if (opt & TERMIOS_WAIT) {
 		tty_wait_until_sent(tty, 0);
 		if (signal_pending(current))
-			return -EINTR;
+			return -ERESTARTSYS;
 	}
 
 	tty_set_termios(tty, &tmp_termios);

@@ -684,7 +684,7 @@ static int set_termiox(struct tty_struct *tty, void __user *arg, int opt)
 	if (opt & TERMIOS_WAIT) {
 		tty_wait_until_sent(tty, 0);
 		if (signal_pending(current))
-			return -EINTR;
+			return -ERESTARTSYS;
 	}
 
 	mutex_lock(&tty->termios_mutex);

This looks reasonable. However given the link above says:
You are not authorized to access bug #904907.
the description above is poor. What problem exactly does this fix? Why this should go to stable at all?

js
suse labs

A syscall must never return EINTR unless it can not be restarted (or it should not be restarted by, say, historical reasons).

In this case ioctl(TCSETAW) returns -EINTR even if it is interrupted by the signal which has the SA_RESTART handler. This doesn't look right no matter what.

OK, this is up to you.

But if this patch is correct, perhaps it should be backported. This -EINTR breaks /bin/bash which doesn't expect it, this leads to
"*** glibc detected *** ./bash-4.1.2-14.el6/bin/bash: double free or corruption (out):"

Perhaps /bin/bash is buggy too, I do not know. Probably Roman and Lingzhu can tell more.

Oleg.

Yes, and more, it is against POSIX. I don't dispute the correctness of the patch at all.

But I really want to hear more details here (the commit log deserves that). E.g. why it started causing problems right now. The code is there like forever.

js
suse labs

I have no idea, I only saw the test-case yesterday.

Perhaps it never caused the problems before, or perhaps it was never reported.

Sure. Lingzhu noticed bug on 2.6.32.

Oleg.

But if you ask how this affects /bin/bash, I can quote the description from Lingzhu,

	sigchld.sh (reproducer):

	#!/bin/bash
	( while :; do kill -CHLD $$ 2>&- || break; done ) &
	while :; do
		read -p 1 -t 0.3 -d ' '
		read -p 2
	done

	Double free happens in read_builtin, here

	    FREE (tofree);
	->  xfree (orig_input_string);

	    return (retval);

result:

	sigchld.sh: line 4: read: error setting terminal attributes: Interrupted system call
	1
	*** glibc detected *** ./bash-4.1.2-14.el6/bin/bash: double free or corruption (out): 0x00000000020f45b0 ***
	======= Backtrace: =========
	(...)

Oleg.

Because right now the situation occurs. It's not common to receive signal during syscall. But when it happens, it should work as documented.

RR