Merge lp:~brian-murray/apport/bug-1500541 into lp:~apport-hackers/apport/trunk

[Brian Murray]

When we initially load the report we only open it for reading, later after retracing we open it for writing and if we can't write to the report apport crashes. This is after a fair bit of work, so it'd be better to write to a new file rather than lose that work.

[Martin Pitt (pitti)]

Making up new file names on the fly like this is dangerous. It's susceptible to symlink attacks in world-write directories such as /tmp/ or /var/crash/, or it might overwrite an already existing file.

The check whether to create an output file could be moved to the top, and apport-retrace could then (and only then) open the file as 'r+b', and seek() to the start before writing.

Alternatively it could open the .out file as 'xb' for exclusive creation, which avoids the above two problems. This isn't available for python 2 yet though; for the deb packages that doesn't matter, but we must pay attention to the data center retracers. Mine don't use output files, so it'd be fine for me. But I still don't like the creation of arbitrary output file names, that's not very predictable.

Thanks!

review: Needs Fixing

Unmerged revisions

3044. By Brian Murray on 2016-01-06

If we cannot write to the output file, write to a new report rather than crash.

=== modified file 'bin/apport-retrace'
--- bin/apport-retrace	2015-09-24 12:19:28 +0000
+++ bin/apport-retrace	2016-01-06 18:06:54 +0000
@@ -402,7 +402,12 @@
 
     else:
         if options.output is None:
-            out = open(options.report, 'wb')
+            try:
+                out = open(options.report, 'wb')
+            except PermissionError:
+                apport.error("Cannot write to %s, writing to %s.retraced instead." %
+                             (options.report, options.report))
+                out = open('%s.out' % options.report, 'wb')
         elif options.output == '-':
             if sys.version[0] < '3':
                 out = sys.stdout