Bug 1614765

Created attachment 1474985 [details]
virt-p2v-cancel-conversion-log.zip

The Cancel button just sends ^C to the other end.  Presumably the
interactivity (bug 1507901) is causing that to be ignored for some
reason.  However this is just a consequence of bug 1507901 so I'm
setting that as a dep and moving it to 7.7.

(In reply to Richard W.M. Jones from comment #3)
> The Cancel button just sends ^C to the other end.  Presumably the
> interactivity (bug 1507901) is causing that to be ignored for some
> reason.  However this is just a consequence of bug 1507901 so I'm
> setting that as a dep and moving it to 7.7.

A possible solution here can be to have a grace period (say 1 to 5 minutes or so) after Ctrl+C is sent, and then forcefully kill virt-v2v, and the ssh connection.

This bug will be addressed in next major release.

After evaluating this issue, there are no plans to address it further or fix it in an upcoming release.  Therefore, it is being closed.  If plans change such that this issue will be fixed in an upcoming release, then the bug can be reopened.

I apologise for the actions of the "stale" bug process above.  This bug
is not stale, and I am reopening it.  All bugs are important.

After evaluating this issue, there are no plans to address it further or fix it in an upcoming release.  Therefore, it is being closed.  If plans change such that this issue will be fixed in an upcoming release, then the bug can be reopened.

My apologies, this bug was closed by a broken process that we
do not have any control over.  Reopening.

I think I have a solution to the riddle. The key information is the screenshot from comment#0. It is *libvirt-client* logic that's asking for a password.

Now, in libvirt, the default authentication callback, for collecting credentials, is virConnectAuthCallbackDefault() [src/libvirt.c]. For the VIR_CRED_PASSPHRASE and VIR_CRED_NOECHOPROMPT credential types, this function calls virGetPassword(). virGetPassword() [src/util/virutil.c] in turn uses the "once standard" function getpass(), for reading the password.

Now, if you look up getpass()'s Linux/glibc manual:

https://man7.org/linux/man-pages/man3/getpass.3.html#NOTES

you find that "Line editing is not disabled". That's the explanation.

The terminal is configured so that ^C (or the INTR character in general) does not cause the terminal driver to generate SIGINT. So, when virt-p2v sends byte 0x03 via cancel_conversion() -> set_cancel_requested() -> mexp_send_interrupt(), it does not result in a SIGINT on the remote end, *even though* ssh did allocate a pseudo-terminal.

I've actually tested this right now on RHEL-9.1, in a local XTerm, with the following trivial (SUSv1) program:

#define _XOPEN_SOURCE
#include <unistd.h>
int main(void)
{
  getpass("hello: ");
  return 0;
}

And sure enough, if I press ^C while at the "hello: " prompt, the program is *not* interrupted.

Now, the getpass() specification is marked TO BE WITHDRAWN even in SUSv1 (aka XSHv4.2), it is marked LEGACY in SUSv2 (aka XSHv5), and it was removed in SUSv3 (aka XSHv6). The justification in SUSv1 is given as "This interface is marked TO BE WITHDRAWN because its name is misleading, and it provides no functionality which the user could not easily implement."

The Linux manual even starts with <https://man7.org/linux/man-pages/man3/getpass.3.html#DESCRIPTION>: "This function is obsolete.  Do not use it.  If you want to read input without terminal echoing enabled, see the description of the ECHO flag in termios(3)". And that's exactly what the read_key() function in libguestfs-common does [options/keys.c].

So, what we have here is arguably a bug in libvirt, which just happens to play badly with virt-p2v.

Now, I can't exactly pinpoint the problematic commit in libvirt. The first getpass call was introduced in commit e332ccdf710d ("Wire up SASL interaction callbacks to libvirt callbacks. Provide default callback impl", 2007-12-05). A later commit (0d14fc0cbb4d / 2007-12-07) stated that getpass() would be pulled in from gnulib. A much later commit 3d63a187baf7 ("bootstrap: remove 18 more gnulib modules", 2020-01-29) said: "getpass: simplified impl is imported". As of today, I can't find any getpass() implementation embedded in libvirt. My own test above was done with plain glibc, *not* gnulib.

FWIW, in gnulib, as of commit 71e20f3e05dd, I find, in "lib/getpass.c":

  /* Turn echoing off if it is on now.  */
# if HAVE_TCGETATTR
  if (tcgetattr (fileno (in), &t) == 0)
    {
      /* Save the old one. */
      s = t;
      /* Tricky, tricky. */
      t.c_lflag &= ~(ECHO | ISIG);
      tty_changed = (tcsetattr (fileno (in), TCSAFLUSH | TCSASOFT, &t) == 0);
    }
# endif

Yeah. I'm not amused by that "tricky, tricky" comment. I agree it is a problem if an interactive program is interrupted with SIGINT while the program has ECHO disabled. The solution to that is for the program to install a SIGINT handler, and to deal with the terminal flags explicitly. The solution is not to *try* to suppress terminal driver originated signals; a SIGTERM for example could still be sent from a different process totally asynchronously, and that would live ECHO just as disabled. So, disabling ISIG here is arguably a bug, and libvirt calling getpass() is arguably a bug too. Arguably, any sane interactive shell will also sanitize the terminal mode when its children in the terminal's foreground process group exit with WIFSIGNALED / WTERMSIG / SIGINT.

The clearing of ISIG in gnulib goes back to commit 4f4a05dccfac ("New file, from Bruno Haible.  Required for BeOS.", 2000-06-21), i.e., the introduction of the whole file. It makes no comments on ISIG.

The glibc history is more detailed, and interesting:

- the glibc *git* repository's initial commit, namely 28f540f45bba ("initial import", 1995-02-18), imported getpass with just ECHO cleared.
- commit df4ef2ab9c08 ("update from main archive 960105", 1997-01-06) went on to clear ISIG and ICANON *in addition* to ECHO (and added the useless and self-imporant comment "Tricky, tricky")
- commit ceb2d9aaa86c ("update from main archive 970226", 1997-02-27) reverted the clearing of ICANON -- note the manual of today: "Line editing is not disabled"; ICANON stands for erase and kill processing --, but kept clearing both ECHO and ISIG.

So we have an ancient, unjustified / undocumented decision to clear ISIG, from 1997, in both glibc and gnulib, biting us now.

With all that, I claim that this symptom, and bug 1507901, have *different* root causes. In bug 1507901, virt-v2v expects input we can't provide from virt-p2v. In this bug, we send 0x03 over the pseudo-terminal's master end, but at the slave end, it does not result in the desired SIGINT, because the getpass() function must have cleared the ISIG local mode flag on that end.

For now, I'm removing said the dependency. Beyond that, I'm unsure how to fix this; arguably, we should move this BZ over to libvirt.

... with the request: "please replace getpass() with your own password reading function that only disables the ECHO local mode flag".