Skip to content

Commit 97c46fc

Browse files
isaacsisaacs
committed
fix(unpack): always resume parsing after an entry error
Also, close the fd, no matter what happens. Fix: #265 Fix: #276
1 parent 488ab8c commit 97c46fc

File tree

2 files changed

+215
-12
lines changed

2 files changed

+215
-12
lines changed

‎lib/unpack.js

+36-12
Original file line numberDiff line numberDiff line change
@@ -335,17 +335,31 @@ class Unpack extends Parser {
335335
mode: mode,
336336
autoClose: false,
337337
})
338-
stream.on('error', er => this[ONERROR](er, entry))
338+
stream.on('error', er => {
339+
if (stream.fd)
340+
fs.close(stream.fd, () => {})
341+
this[ONERROR](er, entry)
342+
fullyDone()
343+
})
339344

340345
let actions = 1
341346
const done = er => {
342-
if (er)
343-
return this[ONERROR](er, entry)
347+
if (er) {
348+
/* istanbul ignore else - we should always have a fd by now */
349+
if (stream.fd)
350+
fs.close(stream.fd, () => {})
351+
this[ONERROR](er, entry)
352+
fullyDone()
353+
return
354+
}
344355

345356
if (--actions === 0) {
346357
fs.close(stream.fd, er => {
358+
if (er)
359+
this[ONERROR](er, entry)
360+
else
361+
this[UNPEND]()
347362
fullyDone()
348-
er ? this[ONERROR](er, entry) : this[UNPEND]()
349363
})
350364
}
351365
}
@@ -380,7 +394,10 @@ class Unpack extends Parser {
380394

381395
const tx = this.transform ? this.transform(entry) || entry : entry
382396
if (tx !== entry) {
383-
tx.on('error', er => this[ONERROR](er, entry))
397+
tx.on('error', er => {
398+
this[ONERROR](er, entry)
399+
fullyDone()
400+
})
384401
entry.pipe(tx)
385402
}
386403
tx.pipe(stream)
@@ -390,8 +407,9 @@ class Unpack extends Parser {
390407
const mode = entry.mode & 0o7777 || this.dmode
391408
this[MKDIR](entry.absolute, mode, er => {
392409
if (er) {
410+
this[ONERROR](er, entry)
393411
fullyDone()
394-
return this[ONERROR](er, entry)
412+
return
395413
}
396414

397415
let actions = 1
@@ -482,8 +500,9 @@ class Unpack extends Parser {
482500

483501
this[MKDIR](path.dirname(entry.absolute), this.dmode, er => {
484502
if (er) {
503+
this[ONERROR](er, entry)
485504
done()
486-
return this[ONERROR](er, entry)
505+
return
487506
}
488507
fs.lstat(entry.absolute, (er, st) => {
489508
if (st && (this.keep || this.newer && st.mtime > entry.mtime)) {
@@ -509,8 +528,11 @@ class Unpack extends Parser {
509528
}
510529

511530
[MAKEFS] (er, entry, done) {
512-
if (er)
513-
return this[ONERROR](er, entry)
531+
if (er) {
532+
this[ONERROR](er, entry)
533+
done()
534+
return
535+
}
514536

515537
switch (entry.type) {
516538
case 'File':
@@ -534,10 +556,12 @@ class Unpack extends Parser {
534556
// XXX: get the type ('file' or 'dir') for windows
535557
fs[link](linkpath, entry.absolute, er => {
536558
if (er)
537-
return this[ONERROR](er, entry)
559+
this[ONERROR](er, entry)
560+
else {
561+
this[UNPEND]()
562+
entry.resume()
563+
}
538564
done()
539-
this[UNPEND]()
540-
entry.resume()
541565
})
542566
}
543567
}

‎test/unpack.js

+179
Original file line numberDiff line numberDiff line change
@@ -2701,3 +2701,182 @@ t.test('using strip option when top level file exists', t => {
27012701
check(t, path)
27022702
})
27032703
})
2704+
2705+
t.test('handle EPERMs when creating symlinks', t => {
2706+
// https://github.com/npm/node-tar/issues/265
2707+
const msg = 'You do not have sufficient privilege to perform this operation.'
2708+
const er = Object.assign(new Error(msg), {
2709+
code: 'EPERM',
2710+
})
2711+
t.teardown(mutateFS.fail('symlink', er))
2712+
const data = makeTar([
2713+
{
2714+
path: 'x',
2715+
type: 'Directory',
2716+
},
2717+
{
2718+
path: 'x/y',
2719+
type: 'File',
2720+
size: 'hello, world'.length,
2721+
},
2722+
'hello, world',
2723+
{
2724+
path: 'x/link1',
2725+
type: 'SymbolicLink',
2726+
linkpath: './y',
2727+
},
2728+
{
2729+
path: 'x/link2',
2730+
type: 'SymbolicLink',
2731+
linkpath: './y',
2732+
},
2733+
{
2734+
path: 'x/link3',
2735+
type: 'SymbolicLink',
2736+
linkpath: './y',
2737+
},
2738+
{
2739+
path: 'x/z',
2740+
type: 'File',
2741+
size: 'hello, world'.length,
2742+
},
2743+
'hello, world',
2744+
'',
2745+
'',
2746+
])
2747+
2748+
const dir = path.resolve(unpackdir, 'eperm-symlinks')
2749+
mkdirp.sync(`${dir}/sync`)
2750+
mkdirp.sync(`${dir}/async`)
2751+
2752+
const check = path => {
2753+
t.match(WARNINGS, [
2754+
['TAR_ENTRY_ERROR', msg],
2755+
['TAR_ENTRY_ERROR', msg],
2756+
['TAR_ENTRY_ERROR', msg],
2757+
], 'got expected warnings')
2758+
t.equal(WARNINGS.length, 3)
2759+
WARNINGS.length = 0
2760+
t.equal(fs.readFileSync(`${path}/x/y`, 'utf8'), 'hello, world')
2761+
t.equal(fs.readFileSync(`${path}/x/z`, 'utf8'), 'hello, world')
2762+
t.throws(() => fs.statSync(`${path}/x/link1`), { code: 'ENOENT' })
2763+
t.throws(() => fs.statSync(`${path}/x/link2`), { code: 'ENOENT' })
2764+
t.throws(() => fs.statSync(`${path}/x/link3`), { code: 'ENOENT' })
2765+
}
2766+
2767+
const WARNINGS = []
2768+
const u = new Unpack({
2769+
cwd: `${dir}/async`,
2770+
onwarn: (code, msg, er) => WARNINGS.push([code, msg]),
2771+
})
2772+
u.on('end', () => {
2773+
check(`${dir}/async`)
2774+
const u = new UnpackSync({
2775+
cwd: `${dir}/sync`,
2776+
onwarn: (code, msg, er) => WARNINGS.push([code, msg]),
2777+
})
2778+
u.end(data)
2779+
check(`${dir}/sync`)
2780+
t.end()
2781+
})
2782+
u.end(data)
2783+
})
2784+
2785+
t.test('close fd when error writing', t => {
2786+
const data = makeTar([
2787+
{
2788+
type: 'Directory',
2789+
path: 'x',
2790+
},
2791+
{
2792+
type: 'File',
2793+
size: 1,
2794+
path: 'x/y',
2795+
},
2796+
'.',
2797+
'',
2798+
'',
2799+
])
2800+
t.teardown(mutateFS.fail('write', new Error('nope')))
2801+
const CLOSES = []
2802+
const OPENS = {}
2803+
const {open} = require('fs')
2804+
t.teardown(() => fs.open = open)
2805+
fs.open = (...args) => {
2806+
const cb = args.pop()
2807+
args.push((er, fd) => {
2808+
OPENS[args[0]] = fd
2809+
cb(er, fd)
2810+
})
2811+
return open.call(fs, ...args)
2812+
}
2813+
t.teardown(mutateFS.mutateArgs('close', ([fd]) => {
2814+
CLOSES.push(fd)
2815+
return [fd]
2816+
}))
2817+
const WARNINGS = []
2818+
const dir = path.resolve(unpackdir, 'close-on-write-error')
2819+
mkdirp.sync(dir)
2820+
const unpack = new Unpack({
2821+
cwd: dir,
2822+
onwarn: (code, msg) => WARNINGS.push([code, msg]),
2823+
})
2824+
unpack.on('end', () => {
2825+
for (const [path, fd] of Object.entries(OPENS))
2826+
t.equal(CLOSES.includes(fd), true, 'closed fd for ' + path)
2827+
t.end()
2828+
})
2829+
unpack.end(data)
2830+
})
2831+
2832+
t.test('close fd when error setting mtime', t => {
2833+
const data = makeTar([
2834+
{
2835+
type: 'Directory',
2836+
path: 'x',
2837+
},
2838+
{
2839+
type: 'File',
2840+
size: 1,
2841+
path: 'x/y',
2842+
atime: new Date('1979-07-01T19:10:00.000Z'),
2843+
ctime: new Date('2011-03-27T22:16:31.000Z'),
2844+
mtime: new Date('2011-03-27T22:16:31.000Z'),
2845+
},
2846+
'.',
2847+
'',
2848+
'',
2849+
])
2850+
// have to clobber these both, because we fall back
2851+
t.teardown(mutateFS.fail('futimes', new Error('nope')))
2852+
t.teardown(mutateFS.fail('utimes', new Error('nooooope')))
2853+
const CLOSES = []
2854+
const OPENS = {}
2855+
const {open} = require('fs')
2856+
t.teardown(() => fs.open = open)
2857+
fs.open = (...args) => {
2858+
const cb = args.pop()
2859+
args.push((er, fd) => {
2860+
OPENS[args[0]] = fd
2861+
cb(er, fd)
2862+
})
2863+
return open.call(fs, ...args)
2864+
}
2865+
t.teardown(mutateFS.mutateArgs('close', ([fd]) => {
2866+
CLOSES.push(fd)
2867+
return [fd]
2868+
}))
2869+
const WARNINGS = []
2870+
const dir = path.resolve(unpackdir, 'close-on-futimes-error')
2871+
mkdirp.sync(dir)
2872+
const unpack = new Unpack({
2873+
cwd: dir,
2874+
onwarn: (code, msg) => WARNINGS.push([code, msg]),
2875+
})
2876+
unpack.on('end', () => {
2877+
for (const [path, fd] of Object.entries(OPENS))
2878+
t.equal(CLOSES.includes(fd), true, 'closed fd for ' + path)
2879+
t.end()
2880+
})
2881+
unpack.end(data)
2882+
})

0 commit comments

Comments
 (0)