Permalink
Browse files

Stronger protection around parallel installs

Now all tarball pack/unpack operations are wrapped in a lock
  • Loading branch information...
isaacs committed Jun 29, 2012
1 parent ccc775c commit d517f420c2f5164b7fbb80adcf634a4805dceb18
Showing with 77 additions and 35 deletions.
  1. +21 −6 lib/cache.js
  2. +56 −29 lib/utils/tar.js
View
@@ -54,6 +54,8 @@ exports = module.exports = cache
exports.read = read
exports.clean = clean
exports.unpack = unpack
+exports.lock = lock
+exports.unlock = unlock
var mkdir = require("mkdirp")
, exec = require("./utils/exec.js")
@@ -788,11 +790,24 @@ function addPlacedTarball_ (p, name, uid, gid, cb) {
var target = path.dirname(p)
, folder = path.join(target, "package")
- rm(folder, function (er) {
- if (er) {
- log.error("addPlacedTarball", "Could not remove %j", folder)
- return cb(er)
- }
+ lock(folder, function (er) {
+ if (er) return cb(er)
+ rmUnpack()
+ })
+
+ function rmUnpack () {
+ rm(folder, function (er) {
+ unlock(folder, function () {
+ if (er) {
+ log.error("addPlacedTarball", "Could not remove %j", folder)
+ return cb(er)
+ }
+ thenUnpack()
+ })
+ })
+ }
+
+ function thenUnpack () {
tar.unpack(p, folder, null, null, uid, gid, function (er) {
if (er) {
log.error("addPlacedTarball", "Could not unpack %j to %j", p, target)
@@ -837,7 +852,7 @@ function addPlacedTarball_ (p, name, uid, gid, cb) {
})
})
})
- })
+ }
}
function addLocalDirectory (p, name, cb) {
View
@@ -9,6 +9,8 @@ var npm = require("../npm.js")
, rm = require("rimraf")
, readJson = require("read-package-json")
, cache = require("../cache.js")
+ , lock = cache.lock
+ , unlock = cache.unlock
, myUid = process.getuid && process.getuid()
, myGid = process.getgid && process.getgid()
, tar = require("tar")
@@ -43,34 +45,43 @@ function pack (targetTarball, folder, pkg, dfc, cb) {
}
}
-function pack_ (targetTarball, folder, pkg, cb) {
- new Packer({ path: folder, type: "Directory", isDirectory: true })
- .on("error", function (er) {
- if (er) log.error("tar pack", "Error reading " + folder)
- return cb(er)
+function pack_ (targetTarball, folder, pkg, cb_) {
+ function cb (er) {
+ unlock(targetTarball, function () {
+ return cb_(er)
})
+ }
+ lock(targetTarball, function (er) {
+ if (er) return cb(er)
- // By default, npm includes some proprietary attributes in the
- // package tarball. This is sane, and allowed by the spec.
- // However, npm *itself* excludes these from its own package,
- // so that it can be more easily bootstrapped using old and
- // non-compliant tar implementations.
- .pipe(tar.Pack({ noProprietary: !npm.config.get("proprietary-attribs") }))
- .on("error", function (er) {
- if (er) log.error("tar.pack", "tar creation error", targetTarball)
- cb(er)
- })
- .pipe(zlib.Gzip())
- .on("error", function (er) {
- if (er) log.error("tar.pack", "gzip error "+targetTarball)
- cb(er)
- })
- .pipe(fstream.Writer({ type: "File", path: targetTarball }))
- .on("error", function (er) {
- if (er) log.error("tar.pack", "Could not write "+targetTarball)
- cb(er)
- })
- .on("close", cb)
+ new Packer({ path: folder, type: "Directory", isDirectory: true })
+ .on("error", function (er) {
+ if (er) log.error("tar pack", "Error reading " + folder)
+ return cb(er)
+ })
+
+ // By default, npm includes some proprietary attributes in the
+ // package tarball. This is sane, and allowed by the spec.
+ // However, npm *itself* excludes these from its own package,
+ // so that it can be more easily bootstrapped using old and
+ // non-compliant tar implementations.
+ .pipe(tar.Pack({ noProprietary: !npm.config.get("proprietary-attribs") }))
+ .on("error", function (er) {
+ if (er) log.error("tar.pack", "tar creation error", targetTarball)
+ cb(er)
+ })
+ .pipe(zlib.Gzip())
+ .on("error", function (er) {
+ if (er) log.error("tar.pack", "gzip error "+targetTarball)
+ cb(er)
+ })
+ .pipe(fstream.Writer({ type: "File", path: targetTarball }))
+ .on("error", function (er) {
+ if (er) log.error("tar.pack", "Could not write "+targetTarball)
+ cb(er)
+ })
+ .on("close", cb)
+ })
}
@@ -87,13 +98,29 @@ function unpack (tarball, unpackTarget, dMode, fMode, uid, gid, cb) {
})
}
-function unpack_ ( tarball, unpackTarget, dMode, fMode, uid, gid, cb ) {
+function unpack_ ( tarball, unpackTarget, dMode, fMode, uid, gid, cb_ ) {
var parent = path.dirname(unpackTarget)
, base = path.basename(unpackTarget)
- rm(unpackTarget, function (er) {
+ function cb (er) {
+ unlock(unpackTarget, function () {
+ return cb_(er)
+ })
+ }
+
+ lock(unpackTarget, function (er) {
if (er) return cb(er)
+ rmGunz()
+ })
+ function rmGunz () {
+ rm(unpackTarget, function (er) {
+ if (er) return cb(er)
+ gtp()
+ })
+ }
+
+ function gtp () {
// gzip {tarball} --decompress --stdout \
// | tar -mvxpf - --strip-components=1 -C {unpackTarget}
gunzTarPerm( tarball, unpackTarget
@@ -103,7 +130,7 @@ function unpack_ ( tarball, unpackTarget, dMode, fMode, uid, gid, cb ) {
if (er) return cb(er)
readJson(path.resolve(folder, "package.json"), cb)
})
- })
+ }
}

0 comments on commit d517f42

Please sign in to comment.