[PATCH for-4.5] tools/pygrub: Fix TOCTOU race introduced by c/s 63dcc68

[PATCH for-4.5] tools/pygrub: Fix TOCTOU race introduced by c/s 63dcc68

[Andrew Cooper]

In addition, use os.makedirs() which will also create intermediate directories
if they don't exist.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Olaf Hering <olaf@aepfle.de>
---
 tools/pygrub/src/pygrub |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
index 1ae34a2..37dde32 100644
--- a/tools/pygrub/src/pygrub
+++ b/tools/pygrub/src/pygrub
@@ -13,7 +13,7 @@
 # Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 #
 
-import os, sys, string, struct, tempfile, re, traceback, stat
+import os, sys, string, struct, tempfile, re, traceback, stat, errno
 import copy
 import logging
 import platform
@@ -846,8 +846,14 @@ if __name__ == "__main__":
     if debug:
 	logging.basicConfig(level=logging.DEBUG)
 
-    if not os.path.isdir(output_directory):
-        os.mkdir(output_directory, 0700)
+
+    try:
+        os.makedirs(output_directory, 0700)
+    except OSError,e:
+        if (e.errno == errno.EEXIST) and os.path.isdir(output_directory):
+            pass
+        else:
+            raise
 
     if output is None or output == "-":
         fd = sys.stdout.fileno()
-- 
1.7.10.4

Re: [PATCH for-4.5] tools/pygrub: Fix TOCTOU race introduced by c/s 63dcc68

[Olaf Hering]

On Wed, Oct 29, Andrew Cooper wrote:

> In addition, use os.makedirs() which will also create intermediate directories
> if they don't exist.

Can this happen in practice, given that /var/run/xen is created by the
runlevel scripts already? The mkdir is now really just @XEN_RUN_DIR@/pygrub.

Olaf

Re: [PATCH for-4.5] tools/pygrub: Fix TOCTOU race introduced by c/s 63dcc68

[Andrew Cooper]

On 29/10/14 16:03, Olaf Hering wrote:
> On Wed, Oct 29, Andrew Cooper wrote:
>
>> In addition, use os.makedirs() which will also create intermediate directories
>> if they don't exist.
> Can this happen in practice, given that /var/run/xen is created by the
> runlevel scripts already? The mkdir is now really just @XEN_RUN_DIR@/pygrub.
>
> Olaf

Yes, although I presume not given the runlevel scripts.

XenServer, and Xapi in particular, is very different from an upstream xl
based system.  It has traditionally had its own initscripts.  We are in
the process of disentangling these, but it is not a trivial amount of work.

The result is that I have positively proved that XenServer hit this race
during automatic testing.  I agree that it shouldn't happen in reality
for a system configured as expected, but that fact alone doesn't
invalidate the fix itself.

~Andrew

Re: [PATCH for-4.5] tools/pygrub: Fix TOCTOU race introduced by c/s 63dcc68

[Olaf Hering]

On Wed, Oct 29, Andrew Cooper wrote:

> The result is that I have positively proved that XenServer hit this race
> during automatic testing.  I agree that it shouldn't happen in reality
> for a system configured as expected, but that fact alone doesn't
> invalidate the fix itself.

I think it can happen even with upstream if two guests start at the same
time and both execute the code at the same time. Both will find the dir
does not exist yet and both will attempt to create the dir. I think one
of both will get an exception.

Olaf

Re: [PATCH for-4.5] tools/pygrub: Fix TOCTOU race introduced by c/s 63dcc68

[Ian Campbell]

On Wed, 2014-10-29 at 14:09 +0000, Andrew Cooper wrote:
> In addition, use os.makedirs() which will also create intermediate directories
> if they don't exist.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Olaf Hering <olaf@aepfle.de>

Acked and applied as a bug fix.

In the future when you include a "for-4.5" tag on a patch please
*always* state your reasoning (usually after the "---"). Don't make the
maintainer / release manager / committer guess why you think it should
be in 4.5. If you consider it "obviously a bug fix" then say so and then
say something about the impact of the bug and the risk associated with
the fix.

I'm a bit grumpy about the number of patches which don't do this, such
patches are mostly going to the back of my queue, alongside the 4.6
patches, since without any rationale that is effectively what I consider
them to be despite the use of the for-4.5 tag.

Ian.

Please *justify* your use of the for-4.5 tag when posting patches

[Ian Campbell]

On Tue, 2014-11-04 at 10:48 +0000, Ian Campbell wrote:

Reposting with a more eyecatching subject, since this was by far not the
only patch with this issue.

> In the future when you include a "for-4.5" tag on a patch please
> *always* state your reasoning (usually after the "---"). Don't make the
> maintainer / release manager / committer guess why you think it should
> be in 4.5. If you consider it "obviously a bug fix" then say so and then
> say something about the impact of the bug and the risk associated with
> the fix.
> 
> I'm a bit grumpy about the number of patches which don't do this, such
> patches are mostly going to the back of my queue, alongside the 4.6
> patches, since without any rationale that is effectively what I consider
> them to be despite the use of the for-4.5 tag.