[Merge] lp:~tdaitx/ubuntu/focal/apport/lp-1851806-fix into lp:~ubuntu-core-dev/ubuntu/focal/apport/ubuntu

[Tiago Stürmer Daitx]

I added comments bellow.

Diff comments:

> 
> === modified file 'apport/ui.py'
> --- apport/ui.py	2019-12-03 03:18:16 +0000
> +++ apport/ui.py	2020-01-06 14:09:26 +0000
> @@ -243,8 +243,11 @@
>              logind_session = None
>          else:
>              reports = apport.fileutils.get_new_reports()
> -            proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
> -            logind_session = apport.Report.get_logind_session(proc_pid_fd)
> +            if _python2:

Thanks for catching that. Using PY3 is the right way.

> +                logind_session = apport.Report.get_logind_session(os.getpid())
> +            else:
> +                proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
> +                logind_session = apport.Report.get_logind_session(proc_pid_fd=proc_pid_fd)
>  
>          for f in reports:
>              if not self.load_report(f):
> 
> === modified file 'data/apport'
> --- data/apport	2019-12-05 01:47:09 +0000
> +++ data/apport	2020-01-06 14:09:26 +0000
> @@ -516,7 +516,7 @@
>              sys.exit(0)
>  
>          # Send all arguments except for the first (exec path) and last (global pid)
> -        args = ' '.join(sys.argv[1:-1])
> +        args = ' '.join(sys.argv[1:5])

This is the change related to "data/apport: fix number of arguments passed through socks into a container". 

After Michael's addition of an extra legacy parameter (for the executable-path), the previous condition is no longer valid: it would need to remove both global pid *and* the executable-path (if given). After some discussion over IRC we realized that we could just pass the fixed number of legacy parameters, thus 1:5.

The downside is that there is no way to pass the executable-path argument to a container in the way it is done now without breaking apport inside containers, as we can't tell if the container's apport supports all arguments. Adding such support would require more code changes unrelated to the fix, so we left it as it is and any improvement should be done in a separated effort.

Let me know if this should somehow be added to the changelog or the bug report.

>          try:
>              sock.sendmsg([args.encode()], [
>                  # Send a ucred containing the global pid
> 
> === modified file 'test/test_report.py'
> --- test/test_report.py	2019-12-12 23:13:17 +0000
> +++ test/test_report.py	2020-01-06 14:09:26 +0000
> @@ -2275,9 +2275,28 @@
>          self.assertEqual(expected, pr.crash_signature())
>  
>      def test_get_logind_session(self):
> +        ret = apport.Report.get_logind_session(os.getpid())
> +        if ret is None:
> +            # ensure that we don't run under logind, and thus the None is
> +            # justified
> +            with open('/proc/self/cgroup') as f:
> +                contents = f.read()
> +            sys.stdout.write('[not running under logind] ')
> +            sys.stdout.flush()
> +            self.assertNotIn('name=systemd:/user', contents)
> +            return
> +
> +        (session, timestamp) = ret
> +        self.assertNotEqual(session, '')
> +        # session start must be >= 2014-01-01 and "now"
> +        self.assertLess(timestamp, time.time())

On a side note: the original test was added back in https://bazaar.launchpad.net/~ubuntu-core-dev/ubuntu/focal/apport/ubuntu/revision/1369.34.961, we are just reusing it to test both os.getpid() and proc_pid_fd calls.

The comment says 
- session start must be >= 2014-01-01 and "now"
and I believe it actually means
1. session start must be >= 2014-01-01 and <= "now"
as the session is expected to have been started before the test is ran. Thus the test seems ok. 

I could improve the comment if you agree that the logic is sound, let me know if I should commit a change for that.

> +        self.assertGreater(timestamp,
> +                           time.mktime(time.strptime('2014-01-01', '%Y-%m-%d')))
> +
> +    def test_get_logind_session_fd(self):
>          proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
>          self.addCleanup(os.close, proc_pid_fd)
> -        ret = apport.Report.get_logind_session(proc_pid_fd)
> +        ret = apport.Report.get_logind_session(proc_pid_fd=proc_pid_fd)
>          if ret is None:
>              # ensure that we don't run under logind, and thus the None is
>              # justified


-- 
https://code.launchpad.net/~tdaitx/ubuntu/focal/apport/lp-1851806-fix/+merge/377182
Your team Ubuntu Core Development Team is subscribed to branch lp:~ubuntu-core-dev/ubuntu/focal/apport/ubuntu.