Code review comment for lp:~jdstrand/snap-confine/ubuntu-core-launcher.nnp-off

Most of these changes look good in isolation but the "out:" label in seccomp_load_filters() is asked to do too many things. It is currently trying to handle:

- error returns
- normal code flow
- run as a 'real root'
- run via setuid execution

seccomp_load_filters() should be adjusted to die immediately on any error and avoid use of "out" for that case.

Furthermore the extensive efforts to allow testing in non-production environments is very complicated. The calls to getenv() for UBUNTU_CORE_LAUNCHER_DEBUG and UBUNTU_CORE_LAUNCHER_NO_ROOT should instead use secure_getenv().

The other environment variables should probably also be converted to use secure_getenv() as well but they have far less effect on the execution pathways, only on the correctness of the resulting policy. (secure_getenv() may break the current system but this may simply be a reflection that environment variables are being asked to do tasks they shouldn't.)

Here's the full notes I collected; not everything is direct result of this merge proposal, but this seems as good a place as any to put them:

- run_snappy_app_dev_add() does not check fork() error return
- write_string_to_file() doesn't check fclose() error return
  - close(2) manpage describes why this is important
- snappy_udev_setup_required() seems very brittle
  - why don't all snaps require this anyway?
- main() argv[NR_ARGS] = (char*)binary -- very odd, argv[NR_ARGS]
  should not have been modified at any point during the execution, and the
  statement uses a comma rather than ; to separate it from the next
  statement, exec().
- main() extra arguments passed through argv[] are passed to the executed
  program; is this intentional?

- seccomp_load_filters() "snadbox"
- seccomp_load_filters() snprintf() does not check for >= 128 error return
- seccomp_load_filters() line-too-long error handling is awkward; rather
  than jumping to out: it should probably just die immediately.
- seccomp_load_filters() line-too-long test could be written simpler
  without line length checks and use strchr('\n') instead. (I think both
  will fail if the file doesn't end with a new line character.)
- seccomp_load_filters() out: error-handling is far too complicated.
  Errors should probably just die() and allow out: for the "normal" exit
  path.
- seccomp_load_filters() "@unrestricted" check is awkward -- strncmp()
  isn't needed and only confuses the matter -- and hiding "@unrestricted"
  several hundred lines into a file means the rules that were processed to
  that point are just dropped entirely. IF we're going to allow an
  "@unrestricted" escape hatch we should enforce that it is the entire
  contents of the file, not just one line in a file.

Thanks