Skip to content

Fix file descriptor leak in utimesMillisSync#1066

Merged
RyanZim merged 2 commits into
masterfrom
copilot/mirror-error-handling-utimesmillissync
Apr 21, 2026
Merged

Fix file descriptor leak in utimesMillisSync#1066
RyanZim merged 2 commits into
masterfrom
copilot/mirror-error-handling-utimesmillissync

Conversation

Copilot AI commented Apr 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

utimesMillisSync did not wrap futimesSync in a try/finally block, so a thrown error would cause the opened file descriptor to never be closed. utimesMillis (the async counterpart) already handled this correctly.

Changes

  • lib/util/utimes.js: Rewrote utimesMillisSync to mirror the try/catch/finally pattern from utimesMillis — closeSync is now always called in the finally block regardless of whether futimesSync throws.
  • lib/util/__tests__/utimes.test.js: Added a test for utimesMillisSync that verifies closeSync is called even when futimesSync throws, confirming no file descriptor leak occurs.
// Before — fd leaks if futimesSync throws
function utimesMillisSync (path, atime, mtime) {
  const fd = fs.openSync(path, 'r+')
  fs.futimesSync(fd, atime, mtime)
  return fs.closeSync(fd)
}

// After — fd always closed
function utimesMillisSync (path, atime, mtime) {
  const fd = fs.openSync(path, 'r+')
  let error = null
  try {
    fs.futimesSync(fd, atime, mtime)
  } catch (futimesErr) {
    error = futimesErr
  } finally {
    try {
      fs.closeSync(fd)
    } catch (closeErr) {
      error = closeErr
    }
  }
  if (error) throw error
}

@RyanZim RyanZim marked this pull request as ready for review April 20, 2026 16:34
@RyanZim RyanZim requested review from JPeer264 and manidlou April 20, 2026 16:34
@RyanZim RyanZim mentioned this pull request Apr 20, 2026
Merged
@RyanZim

RyanZim commented Apr 20, 2026

Copy link
Copy Markdown
Collaborator

After this is merged, should take care of #1065 (comment)

JPeer264
JPeer264 reviewed Apr 21, 2026
View reviewed changes

@JPeer264 JPeer264 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Could you write a test case that it won't leak any further?

The new code makes sense actually.

Copilot AI commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Could you write a test case that it won't leak any further?

The new code makes sense actually.

Added a test in commit 90cf44f. It stubs openSync/futimesSync/closeSync via proxyquire (same pattern as the existing async test), makes futimesSync throw, and asserts that the error is propagated and closeSync was still called.

Copilot AI requested a review from JPeer264 April 21, 2026 07:36
JPeer264
JPeer264 approved these changes Apr 21, 2026
View reviewed changes

@JPeer264 JPeer264 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the test is actually correct - as it only checks if closeSync is called, would be nice if someone could verify that.

The changes actually look good (except this one: #1065 (comment))

@RyanZim

RyanZim commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

@JPeer264 Copilot's new test is a mirror of our existing test for utimesMillis:

node-fs-extra/lib/util/__tests__/utimes.test.js

Lines 70 to 98 in a4000d6

it('should close open file desciptors after encountering an error', done => {
const fakeFd = Math.random()
gracefulFsStub.open = u((pathIgnored, flagsIgnored, modeIgnored, callback) => {
if (typeof modeIgnored === 'function') callback = modeIgnored
process.nextTick(() => callback(null, fakeFd))
})
let closeCalled = false
gracefulFsStub.close = u((fd, callback) => {
assert.strictEqual(fd, fakeFd)
closeCalled = true
if (callback) process.nextTick(callback)
})
let testError
gracefulFsStub.futimes = u((fd, atimeIgnored, mtimeIgnored, callback) => {
process.nextTick(() => {
testError = new Error('A test error')
callback(testError)
})
})
utimes.utimesMillis('ignored', 'ignored', 'ignored', err => {
assert.strictEqual(err, testError)
assert(closeCalled)
done()
})
})

This seems correct; as long as close is called in the error case, it shouldn't leak. Is there something I'm missing?

@RyanZim RyanZim merged commit 1c248ed into master Apr 21, 2026
18 checks passed
@RyanZim RyanZim deleted the copilot/mirror-error-handling-utimesmillissync branch April 21, 2026 14:34
@RyanZim

RyanZim commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

Merged to move on to #1066 (comment); if there's a problem with the test, we can fix in a follow-up PR.

@RyanZim RyanZim added the bug label Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JPeer264 JPeer264 JPeer264 approved these changes

@manidlou manidlou Awaiting requested review from manidlou

Assignees

@RyanZim RyanZim

Copilot code review Copilot

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@RyanZim @JPeer264