Bug 649 - Foregrounding a backgrounded process (vim) will fail at times (fg, curses, terminal, ^Z)
Foregrounding a backgrounded process (vim) will fail at times (fg, curses, te...
Status: RESOLVED FIXED
Product: Sudo
Classification: Unclassified
Component: Sudo
1.8.8
PC Linux
: low high
Assigned To: Todd C. Miller
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-24 05:51 MDT by Walter Doekes
Modified: 2014-09-24 09:32 MDT (History)
1 user (show)

See Also:


Attachments
Yep, looks like that is the culprit indeed. (2.02 KB, patch)
2014-05-24 06:11 MDT, Walter Doekes
Details | Diff
Indeed, only one change was needed for the fix. (437 bytes, patch)
2014-05-24 06:17 MDT, Walter Doekes
Details | Diff
Patch to ignore program-generated SIGTSTP (807 bytes, patch)
2014-05-27 10:47 MDT, Todd C. Miller
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Walter Doekes 2014-05-24 05:51:13 MDT
When 'fg'ing a backgrounded vim, about 50% of the time, the process isn't foregrounded immediately. Instead, the shell is partially cleared.

When then doing a second 'fg', vim doesn't get the right terminal state: now all keys are echoed to the screen instead of behaving normally, rendering the vim session unusable.


This happens with 1.8.8 and 1.8.9 and 1.8.10, but not with 1.8.7, I think.

After too many minutes messing with hg bisect, I found this:

  The first bad revision is:
  changeset:   8576:666e95c9a0f1
  user:        Todd C. Miller <Todd.Miller@courtesan.com>
  date:        Thu Jan 17 09:17:54 2013 -0500
  summary:     Rename handle_signals() to dispatch_signals().

I'll see if reverting parts of that makes things work normally again.

Cheers,
Walter Doekes
OSSO B.V.

P.S. Marking as 'high', because this is pretty annoying and a regression.
Comment 1 Todd C. Miller 2014-05-24 06:03:26 MDT
Thanks for tracking this down, I've been unable to replicate the bug myself.  Looking at the changeset the most likely culprit is the change from:

sigemptyset(&sa.sa_mask);

to:

sigfillset(&sa.sa_mask);

in exec.c

Unfortunately, as I cannot reproduce this, I can't say for certain.
Comment 2 Walter Doekes 2014-05-24 06:11:15 MDT
Created attachment 410 [details]
Yep, looks like that is the culprit indeed.

I did not go through all of the changes in the patch. Perhaps only half of it fixes the issue.

When patched, my problem is gone.

This patch is against branch 'default'.
Comment 3 Walter Doekes 2014-05-24 06:12:23 MDT
I can try trimming the patch, only replacing fill with empty...

Hang on.
Comment 4 Walter Doekes 2014-05-24 06:17:56 MDT
Created attachment 411 [details]
Indeed, only one change was needed for the fix.

You're right. This single replacement does the trick too.
Comment 5 Walter Doekes 2014-05-24 06:27:13 MDT
As for replicability. It looks like I can only reproduce it on local machines (gnome-terminal and getty) and not through an ssh session (gnome-terminal).
Comment 6 Walter Doekes 2014-05-24 06:36:34 MDT
Reproducing is done like this:

  sudo vi
  ^Z
  fg

Retry ^Z and fg as fast as you can. At one point (always within 10 times here, usually less), you'll notice that the 'fg' doesn't do what it should. That's when you've hit it.

If you now do a second fg, the enter key won't stop at the end of the document, and the arrow keys will produce escape sequences.
Comment 7 Todd C. Miller 2014-05-24 06:38:52 MDT
What's strange is that all the signal handler does is write to an internal  pipe.  The signal mask shouldn't really matter in this case.  What versino of Linux are you using (including kernel version)?  Also, how many CPUs?
Comment 8 Walter Doekes 2014-05-24 06:44:28 MDT
On both machines I've experienced this on, I run Ubuntu/Trusty.

Laptop:

  $ lsb_release -a 2>/dev/null | grep Desc
  Description:	Ubuntu 14.04 LTS
  $ uname -a
  Linux walter-laptop 3.13.0-24-generic #47-Ubuntu SMP
    Fri May 2 23:30:00 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
  $ cat /proc/cpuinfo | grep ^model\ name
  model name	: Intel(R) Core(TM) i7-3517U CPU @ 1.90GHz
  model name	: Intel(R) Core(TM) i7-3517U CPU @ 1.90GHz
  model name	: Intel(R) Core(TM) i7-3517U CPU @ 1.90GHz
  model name	: Intel(R) Core(TM) i7-3517U CPU @ 1.90GHz

Desktop:

  $ lsb_release -a 2>/dev/null | grep ^Des
  Description:	Ubuntu 14.04 LTS
  $ uname -a
  Linux walter-desktop 3.13.0-24-generic #46-Ubuntu SMP
    Thu Apr 10 19:11:08 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
  $ cat /proc/cpuinfo | grep ^model\ name
  model name	: Intel(R) Core(TM)2 Duo CPU     E8400  @ 3.00GHz
  model name	: Intel(R) Core(TM)2 Duo CPU     E8400  @ 3.00GHz
Comment 9 Todd C. Miller 2014-05-27 10:47:13 MDT
Created attachment 412 [details]
Patch to ignore program-generated SIGTSTP

Changing back to sigemptyset() just masks the actual problem, which appears to be sudo passing along SIGTSTP to the program that is running.  There is logic in sudo to avoid doing this when the signal is sent by the kernel (for example, ^Z entered on the terminal) but this is not effective for programs that catch SIGTSTP themselves and re-send the signal, such as vim.

The basic problem is that we want sudo to send the running program SIGTSTP if someone sends the signal directly to the sudo process but if the signal is sent to sudo's progress group, the running program may have already received it so it could be received twice.  A better workaround is to probably ignore signals sent by the command itself, which is what the attached patch does.

I haven't been able to reproduce the problem myself, but I always see SIGTSTP received before SIGCHLD, which doesn't appear to cause a problem.  Please try the attached patch to see whether it also fixes the bug.
Comment 10 Walter Doekes 2014-05-27 13:06:42 MDT
Excellent! `Patch to ignore program-generated SIGTSTP` fixes the problem too.

Thanks for finding the root cause.

Cheers.
Comment 11 Walter Doekes 2014-08-25 01:36:15 MDT
Hi. What can I do to speed this up?

Thanks,
Walter
Comment 12 Todd C. Miller 2014-08-25 07:12:49 MDT
The fix is present in sudo 1.8.11 which is currently in beta.
Comment 13 Walter Doekes 2014-08-25 07:47:01 MDT
I see it's indeed here:
http://www.sudo.ws/repos/sudo/rev/d590c899e194

Thanks for the update.
Comment 14 Todd C. Miller 2014-09-24 09:32:00 MDT
Fixed in sudo 1.8.11.