Skip to content

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Mar 6, 2025

Converts the tests to use be inline expectation tests and adds a test (marked SPURIOUS) for an FP when there is a deferred call to os.File.Close and then later a call to os.File.Sync.

What we should probably do in this case is to make sure that every control flow path from the deferred call to os.File.Close to the end of the function either includes a call to os.File.Sync or involves returning a non-nil error. (I thought about how to do this and I think you would define some predicates like this:

private BasicBlock allowedSuccessor(BasicBlock b) {
  result = b.getASuccessor()
  // and result doesn't contain a handled sync call
  // and result doesn't end by returning an non-nil error
}

private BasicBlock maximalAllowedSuccessor(BasicBlock b) {
  result = maximalAllowedSuccessor(allowedSuccessor(b))
  or
  not exists(allowedSuccessor(b)) and result = b
}

Then isCloseSink should require that when closeCall is deferred, maximalAllowedSuccessor(closeCall) does not include the exit block of the function

@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Mar 6, 2025
@Copilot Copilot AI review requested due to automatic review settings March 6, 2025 10:28
@owen-mc owen-mc requested a review from a team as a code owner March 6, 2025 10:28
@github-actions github-actions bot added the Go label Mar 6, 2025
@owen-mc owen-mc merged commit 22b36a8 into github:main Mar 11, 2025
14 checks passed
@owen-mc owen-mc deleted the go/unhandled-close-writable-handle branch March 11, 2025 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants