shutil.copyfile() is not atomic. Package builder may use
an broken RPM accidently during the copy process.
The race will only affect replacement of publishRPMs. Because
other packages use a map in python to notify others when the copy is
ready. But publishRPMs does not do any check at all.
As a result, doing publishRPM replacement, installToolChainRPM()
may accidently try to install a broken RPM.
Change-Id: Ib4c2e1927d16cc6156d535c33bf5c2f96cf07caf
Reviewed-on: http://photon-jenkins.eng.vmware.com:8082/4245
Tested-by: gerrit-photon <photon-checkins@vmware.com>
Reviewed-by: Xiaolin Li <xiaolinl@vmware.com>
... | ... |
@@ -6,10 +6,17 @@ class CommandUtils(object): |
6 | 6 |
self.findBinary = "find" |
7 | 7 |
|
8 | 8 |
def findFile (self, filename, sourcePath): |
9 |
- process = subprocess.Popen([self.findBinary, "-L", sourcePath, "-name", filename, "-not", "-type", "d"], stdout=subprocess.PIPE) |
|
10 |
- returnVal = process.wait() |
|
11 |
- if returnVal != 0: |
|
12 |
- return None |
|
9 |
+ process = subprocess.Popen([self.findBinary, "-L", sourcePath, "-name", filename, "-not", "-type", "d"], stdout=subprocess.PIPE) |
|
10 |
+ # We don't check the return val here because find could return 1 but still be able to find |
|
11 |
+ # the result. We shouldn't blindly return None without even checking the result. |
|
12 |
+ # The reason we want to suppress this is because for built RPMs, we first copy it to |
|
13 |
+ # the location with a random name and move it to the real name. find will complain our |
|
14 |
+ # action and return 1. |
|
15 |
+ # find's flag ignore_readdir_race can suppress this but it isn't working. |
|
16 |
+ # https://bugs.centos.org/view.php?id=13685 |
|
17 |
+ |
|
18 |
+ #if returnVal != 0: |
|
19 |
+ # return None |
|
13 | 20 |
result=process.communicate()[0] |
14 | 21 |
if result is None: |
15 | 22 |
return None |
... | ... |
@@ -27,7 +34,7 @@ class CommandUtils(object): |
27 | 27 |
if retval==0: |
28 | 28 |
return True |
29 | 29 |
return False |
30 |
- |
|
30 |
+ |
|
31 | 31 |
def runCommandInShell2(self,cmd,chrootCmd=None): |
32 | 32 |
if chrootCmd is not None: |
33 | 33 |
cmd = chrootCmd+" "+cmd |
... | ... |
@@ -128,7 +128,7 @@ class PackageBuilderContainer(object): |
128 | 128 |
outputMap[threadName]=True |
129 | 129 |
except Exception as e: |
130 | 130 |
# TODO: self.logger might be None |
131 |
- self.base.logger.error(e) |
|
131 |
+ self.base.logger.exception(e) |
|
132 | 132 |
outputMap[threadName]=False |
133 | 133 |
raise e |
134 | 134 |
|
... | ... |
@@ -277,7 +277,7 @@ class PackageBuilderChroot(object): |
277 | 277 |
outputMap[threadName]=True |
278 | 278 |
except Exception as e: |
279 | 279 |
# TODO: self.logger might be None |
280 |
- self.base.logger.error(e) |
|
280 |
+ self.base.logger.exception(e) |
|
281 | 281 |
outputMap[threadName]=False |
282 | 282 |
raise e |
283 | 283 |
|
... | ... |
@@ -5,6 +5,8 @@ import platform |
5 | 5 |
import shutil |
6 | 6 |
from constants import constants |
7 | 7 |
import re |
8 |
+import random |
|
9 |
+import string |
|
8 | 10 |
from time import sleep |
9 | 11 |
import PullSources |
10 | 12 |
import json |
... | ... |
@@ -60,11 +62,14 @@ class PackageUtils(object): |
60 | 60 |
cmdUtils = CommandUtils() |
61 | 61 |
rpmName=os.path.basename(rpmFile) |
62 | 62 |
rpmDestDir=self.getRPMDestDir(rpmName,destDir) |
63 |
+ # shutil is not atomic. copy & move to ensure atomicity. |
|
63 | 64 |
rpmDestPath=rpmDestDir+"/"+rpmName |
65 |
+ rpmDestPathTemp = rpmDestDir + "/." + ''.join([random.choice(string.ascii_letters + string.digits) for n in xrange(10)]) |
|
64 | 66 |
if os.geteuid()==0: |
65 | 67 |
if not os.path.isdir(rpmDestDir): |
66 | 68 |
cmdUtils.runCommandInShell("mkdir -p "+rpmDestDir) |
67 |
- shutil.copyfile(rpmFile, rpmDestPath) |
|
69 |
+ shutil.copyfile(rpmFile, rpmDestPathTemp) |
|
70 |
+ shutil.move(rpmDestPathTemp, rpmDestPath) |
|
68 | 71 |
return rpmDestPath |
69 | 72 |
|
70 | 73 |
def installRPM(self,package,chrootID,noDeps=False,destLogPath=None): |
... | ... |
@@ -183,8 +188,8 @@ class PackageUtils(object): |
183 | 183 |
chrootCmd=self.runInChrootCommand+" "+chrootID |
184 | 184 |
shutil.copyfile(specFile, chrootID+chrootSpecPath+specName ) |
185 | 185 |
|
186 |
-# FIXME: some sources are located in SPECS/.. how to mount? |
|
187 |
-# if os.geteuid()==0: |
|
186 |
+ # FIXME: some sources are located in SPECS/.. how to mount? |
|
187 |
+ # if os.geteuid()==0: |
|
188 | 188 |
self.copySourcesTobuildroot(listSourcesFiles,package,chrootSourcePath) |
189 | 189 |
self.copySourcesTobuildroot(listPatchFiles,package,chrootSourcePath) |
190 | 190 |
|