[PATCH 0/1] tty: set_termios/set_termiox should not return -EINTR

[PATCH 0/1] tty: set_termios/set_termiox should not return -EINTR

[Oleg Nesterov]

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.

[PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR

[Oleg Nesterov]

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.

Reported-by: Roman Rakus <rrakus@redhat.com>
Reported-by: Lingzhu Xiang <lxiang@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@kernel.org
---

--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -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);

Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR

[Jiri Slaby]

On 01/29/2013 08:07 PM, Oleg Nesterov wrote:
> 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.

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

Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR

[Oleg Nesterov]

On 01/29, Jiri Slaby wrote:
>
> On 01/29/2013 08:07 PM, Oleg Nesterov wrote:
> >
> > Change set_termios/set_termiox to return -ERESTARTSYS to fix this
> > particular problem.
>
> 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?

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.

> Why this should go to stable at all?

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.

Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR

[Jiri Slaby]

On 01/29/2013 08:49 PM, Oleg Nesterov wrote:
> On 01/29, Jiri Slaby wrote:
>>
>> On 01/29/2013 08:07 PM, Oleg Nesterov wrote:
>>>
>>> Change set_termios/set_termiox to return -ERESTARTSYS to fix this
>>> particular problem.
>>
>> 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?
> 
> 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.

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

>> Why this should go to stable at all?
> 
> 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.

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

Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR

[Oleg Nesterov]

On 01/29, Jiri Slaby wrote:
>
> On 01/29/2013 08:49 PM, Oleg Nesterov wrote:
> >
> > Perhaps /bin/bash is buggy too, I do not know. Probably Roman and
> > Lingzhu can tell more.
>
> But I really want to hear more details here (the commit log deserves
> that). E.g. why it started causing problems right now.

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

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

> The code is there
> like forever.

Sure. Lingzhu noticed bug on 2.6.32.

Oleg.

Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR

[Oleg Nesterov]

On 01/30, Oleg Nesterov wrote:
>
> On 01/29, Jiri Slaby wrote:
> >
> > On 01/29/2013 08:49 PM, Oleg Nesterov wrote:
> > >
> > > Perhaps /bin/bash is buggy too, I do not know. Probably Roman and
> > > Lingzhu can tell more.
> >
> > But I really want to hear more details here (the commit log deserves
> > that). E.g. why it started causing problems right now.
>
> I have no idea, I only saw the test-case yesterday.

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.

Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR

[Roman Rakus]

On 01/29/2013 11:01 PM, Jiri Slaby wrote:
> E.g. why it started causing problems right now. The code is there
> like forever.
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