raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
732 stars 93 forks source link

`removeCallback` fails when a folder inside a tmp folder is read-only #255

Closed okcompute closed 4 years ago

okcompute commented 4 years ago

Operating System

NodeJS Version

Tmp Version

0.2.0 and +

Expected Behavior

I expect tmp.removeCallback to delete read-only folder when I set the unsafeCleanup flag to true when using a temporary directory created with tmp.dirSync. This was working using tmp version 0.1.0.

Experienced Behavior

With 0.2.0 and up, tmp.removeCallback throws an EACCESS error when read-only folder are present in a temporary directory:

/Users/pascall/Downloads/tmp-bug-repro/node_modules/rimraf/rimraf.js:310
        throw er
        ^

Error: EACCES: permission denied, unlink '/var/folders/bh/kklshxl15b70fs46w805zqxdf2__g6/T/tmp-59232-xIk3C0bCCGDE/folder/my-file.txt'
    at Object.unlinkSync (fs.js:1046:3)
    at rimrafSync (/Users/pascall/Downloads/tmp-bug-repro/node_modules/rimraf/rimraf.js:303:17)
    at /Users/pascall/Downloads/tmp-bug-repro/node_modules/rimraf/rimraf.js:336:39
    at Array.forEach (<anonymous>)
    at rmkidsSync (/Users/pascall/Downloads/tmp-bug-repro/node_modules/rimraf/rimraf.js:336:26)
    at rmdirSync (/Users/pascall/Downloads/tmp-bug-repro/node_modules/rimraf/rimraf.js:329:7)
    at rimrafSync (/Users/pascall/Downloads/tmp-bug-repro/node_modules/rimraf/rimraf.js:301:9)
    at /Users/pascall/Downloads/tmp-bug-repro/node_modules/rimraf/rimraf.js:336:39
    at Array.forEach (<anonymous>)
    at rmkidsSync (/Users/pascall/Downloads/tmp-bug-repro/node_modules/rimraf/rimraf.js:336:26) {
  errno: -13,
  syscall: 'unlink',
  code: 'EACCES',
  path: '/var/folders/bh/kklshxl15b70fs46w805zqxdf2__g6/T/tmp-59232-xIk3C0bCCGDE/folder/my-file.txt'

Here is the code I used to reproduce the problem:

const fs = require('fs')
const path = require('path')
const tmp = require('tmp')

function reproReadOnlyBug() {
  const tempFolder = tmp.dirSync({ unsafeCleanup: true })
  const folder = path.join(tempFolder.name, 'folder')
  const filePath = path.join(folder, 'my-file.txt')
  fs.mkdirSync(folder)
  fs.writeFileSync(filePath, 'abc')
  fs.chmodSync(folder, 0o400 | 0o100)
  tempFolder.removeCallback()
}

reproReadOnlyBug()
okcompute commented 4 years ago

This issue was first reported in an unrelated issue: https://github.com/raszi/node-tmp/issues/251

okcompute commented 4 years ago

Btw, the behaviour change I'm exposing may be a good thing. I was discussing with a colleague about this issue and he exposed that rimraf basically replicate rm -rf. And doing rm -rf on a read-only folder in the terminal will fail!

silkentrance commented 4 years ago

@okcompute if you set up a small test project and then run index.js, you will find that the tmp folder has not been cleaned up and still resides under e.g. /tmp.

This was actually a bug in 0.1.0 which used asynchronous code when it was expected to do synchronous cleanup. In addition, any exceptions will be caught.

I just ran the example and here is what is left in /tmp:

> find /tmp/tmp-26244oEcZlrGrKBrS/
/tmp/tmp-26244oEcZlrGrKBrS/
/tmp/tmp-26244oEcZlrGrKBrS/folder
/tmp/tmp-26244oEcZlrGrKBrS/folder/my-file.txt
silkentrance commented 4 years ago

https://github.com/isaacs/rimraf/pull/161

silkentrance commented 4 years ago

And rimraf does not handle readonly files or dirs at all.

So I would call this a rather specific use case and I am not so sure whether we would want to handle this as it requires changes to an external library, which we introduced in order to not having to deal with all the platform specific stuff by ourselves.

silkentrance commented 4 years ago

@okcompute maybe you can rethink your use case and why you would need the folder or files to be readonly in the first place. Maybe this might solve your problem then?

okcompute commented 4 years ago

@silkentrance Thank you for the explanation! I will update to version 0.2.1 and change my test to remove the fixture read-only flag before cleanup. I'm glad we now understand where this behaviour change is coming from 🙂️. Thanks again!