[MIR] ubuntu-terminal-app

I reviewed version 0.7ubuntu2~yakketytest in ppa:popey/touch.

This will need a team bug subscriber once it's in Ubuntu.

Need-more-info:

- What's the story with the konsole plugin? Is that bundled code from another package?

Non-blocking notes:

- I would expect to see dh-python3 used (with ${python3:Depends} in debian/control) for the autopilot package. No big deal in this specific case, but that's the usual way to handle python modules.

- It would be nicer to use the proper qml module names of qml-module-* before we land in Ubuntu (instead of qtdeclarative5-*).

- Is qtdeclarative5-pamauthentication0.1 supposed to be installed into a com/ubuntu subdirectory of the normal qml install path? Is it considered a private library? If so, why package it separately from ubuntu-terminal-path?

- I'm sad there's no unit tests. :(

Thanks Mike for the review. Addressing some of the questions:

> - What's the story with the konsole plugin? Is that bundled code from another package?

Initially the Terminal app evolved from Konsole, but later (IIRC) the UI was decoupled from the terminal backend into what became the QML Terminal widget. If I'm not mistaken, the Konsole code is no longer used. I asked popey to look into it, and I see the MP that removed the Konsole code has now been merged:

http://bazaar.launchpad.net/~ubuntu-terminal-dev/ubuntu-terminal-app/reboot/revision/210

> - I would expect to see dh-python3 used (with ${python3:Depends} in debian/control) for the autopilot package. No big deal in this specific case, but that's the usual way to handle python modules.

> - It would be nicer to use the proper qml module names of qml-module-* before we land in Ubuntu (instead of qtdeclarative5-*).

Luke Yelavich has helped us packaging terminal, so I'd probably forward these questions to him.

> - Is qtdeclarative5-pamauthentication0.1 supposed to be installed into a com/ubuntu subdirectory of the normal qml install path? Is it considered a private library? If so, why package it separately from ubuntu-terminal-path?

I can offer some context hoping that it helps. That qml plugin was created as a requirement before Terminal could be made available in the store and executed in customer phones: it provides an additional level of security by asking you for a password before Terminal can be started. IIRC it has the same requirements in terms of path of installation as any other QML plugin, so if it happens to diverge from that it might be worth correcting it.

Note though, that on Unity 8 running on the desktop you won't be prompted for the password.

Finally, one of the reasons it's a separate package is because the File Manager app reused the same package for password prompt. The next question would probably be: why is qtdeclarative5-pamauthentication0.1 not its own separate project? Mainly for historical reasons: at the time the CI infrastructure was put together for core apps Jenkins could not build .debs for MPs from a mixture of repositories, thus the authentication code was included in the Terminal tree to make it a self-contained project.

> - I'm sad there's no unit tests. :(

No excuses, but just some context: at the time the terminal app development was most active, the guidance was to create Autopilot tests instead.

This has a launchpad page now, so you can sub a team to bugs:
https://launchpad.net/ubuntu/+source/ubuntu-terminal-app

That's the last must-fix issue I think.

I added ~ubuntu-terminal-dev (I think). Which should resolve that?

Here's a quick MR for the packaging issues: https://code.launchpad.net/~michael-sheldon/ubuntu-terminal-app/packaging-fixes/+merge/306252

We keep a list of teams we watch for to make sure we have maintainer coverage:
http://bazaar.launchpad.net/~ubuntu-archive/ubuntu-archive-tools/trunk/view/head:/package-subscribers#L107

Maybe ~system-apps-team would be a better fit for that purpose than ~ubuntu-terminal-dev?

Agreed @mterry. Unfortunately I don't have rights to add ~system-apps-team to bug contact for https://launchpad.net/ubuntu/+source/ubuntu-terminal-app, @bfiller, do you?

subscribed

Emily mentioned that the Security team would like to take a look at this so assigning over to them and setting back to new.

Status changed to 'Confirmed' because the bug affects multiple users.

There's no source package in Yakkety?

no package in the distro, only in PPAs

Package has been uploaded to Y and is in the queue.

Package is now in the archive. Pre-promoting to main in advance of security review, per agreement with management.

Override component to main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety i386: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety powerpc: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety ppc64el: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety s390x: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety i386: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety powerpc: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety ppc64el: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety s390x: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety i386: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety powerpc: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety ppc64el: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety s390x: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety i386: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety powerpc: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety ppc64el: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety s390x: universe/misc/extra/100% -> main
ubuntu-terminal-app 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
ubuntu-terminal-app 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
ubuntu-terminal-app 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
ubuntu-terminal-app 0.7.218ubuntu2 in yakk...

Status changed to 'Confirmed' because the bug affects multiple users.

I reviewed ubuntu-terminal-app version 0.7.216 as checked into yakkety.
This should not be considered a full security review.

The pieces of this program that I've read so far give me a mixed
impression. It appears that sections of Qt and KDE frameworks are pulled
into this source package rather than using the packages in the archive. I
suspect much of this code aren't actually used in the terminal emulator.
Its presence here is definitely distracting, and if there is a way to
reach the code then those bugs become our bugs to fix twice.

The mixing is especially complicated because the terminal and process
handling routines are some of the most complicated code to make work
cross-platform -- why, for example, does this package need to know how
to start terminals on IRIX or read OpenBSD MIB tables via SNMP?

Whatever modifications are needed to Qt and KDE frameworks should be
handled as patches to upstream, so that we can more easily reduce our
technical debt by upstreaming the changes. I don't see documentation
here for patches (for example, there's no debian/patches/ directory).
Perhaps this is handled in source control, but the lack of package-level
visibility will greatly complicate other teams (such as the security
team) from interacting with this package when bugs are discovered in
the embedded libraries.

cppcheck is clean.

Compilation logs are mixed; most warnings look harmless but the code
could be re-formatted so that the warnings are no longer emitted.

Here's some of the things I've found so far; I realize that there's a
chance very little of what I found here is actually in Canonical-authored
code.

./src/plugin/qmltermwidget/src/ksession.cpp:
- appears to set _program to getenv("SHELL") || "/bin/bash"
- setenv("TERM", "xterm")

./src/plugin/qmltermwidget/lib/qtermwidget.cpp:
- appears to set _program to getenv("SHELL")

./src/plugin/qmltermwidget/lib/Session.cpp:
- if _program exists and starts with / see if it is executable
  - otherwise use getenv("SHELL"), if that is executable
  - otherwise use /bin/sh
- if _program doesn't exist or doesn't start with / use it directly

- Pty::start() expects the command name to be representable in latin-1.

Normally a shell like /bin/bash is executed with:
execve("/bin/bash", ["-bash", NULL], env);
I don't see any logic to prefix the shell command name with a hyphen.

I'm surprised that the logic to select the program to execute is spread
over three different files. (That I've found so far.)

I'm surprised the shell-picking logic doesn't use getpwent(3)'s pw_shell
field. I'd expect getenv("SHELL"), then getpwent(), then /bin/sh to be
used, in that order, with no environment variable expansion and no
second-guessing if the specified program is executable or not.

- There are a surprising number of mechanisms in KProcess:: for starting
  processes. Some take lists, some take strings, some are "detached"? What
  does it mean to start a process attached? Detached? When would the
  terminal emulator use one vs the other?

- KPty::open() openpty() discards errno in error message
- KPty::open() is very nearly unreadable; I think the ancient Linux nested
  for loops are executed even when openpty()...

Since Unity8 has been removed from main, this MIR is obsolete so I'm going to go ahead and close this bug.