Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go: Switch from def-use flow to use-use flow #14751

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Nov 11, 2023

This is a resurrection of github/codeql-go#460. Rebasing seems to have gone okay. There are more test changes. I am currently going through them to see if they are expected or if they indicate that something need to change.

…implement.

Queries / tests that required changes:
* The CleartextLogging and MissingErrorCheck queries are updated because they assumed def-use flow
* The CommandInjection query works around the shortcomings of use-use flow by essentially reintroducing def-use flow when it applies a sanitizer
* The OpenUrlRedirect query currently just accepts its fate; the tests are updated to avoid excess sanitization while the query comments on the problem. We should choose this approach or the CommandInjection one.
@github-actions github-actions bot added the Go label Nov 11, 2023
@owen-mc
Copy link
Contributor Author

owen-mc commented Nov 11, 2023

There are two lost results for go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql on go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go. They have the exact same cause: in the following function

func closeFileDeferredIndirect(f *os.File) {
	var cont = func() {
		f.Close() // NOT OK, if `f` is writable
	}

	defer cont()
}

we don't have flow from capture variable f in the function literal to the use of f in f.Close(). This flow used to be caught by this disjunct in basicLocalFlowStep:

  // SSA -> Instruction
  exists(SsaDefinition pred, IR::Instruction succ |
    succ = pred.getVariable().getAUse() and
    nodeFrom = ssaNode(pred) and
    nodeTo = instructionNode(succ)
  )

but now that has changed to:

  // SSA defn -> first SSA use
  exists(SsaExplicitDefinition pred, IR::Instruction succ | succ = pred.getAFirstUse() |
    nodeFrom = ssaNode(pred) and
    nodeTo = instructionNode(succ)
  )
  or
  // SSA use -> successive SSA use
  // Note this case includes Phi node traversal
  exists(IR::Instruction pred, IR::Instruction succ | succ = getAnAdjacentUse(pred) |
    nodeFrom = instructionNode(pred) and
    nodeTo = instructionNode(succ)
  )

capture variable f is an SsaImplicitDefinition rather than an SsaExplicitDefinition, so the first of these disjuncts doesn't catch it. I am not sure which of these disjuncts should be amended to capture this flow.

@smowton
Copy link
Contributor

smowton commented Nov 13, 2023

Presumably this is because of switching from SsaDefinition to SsaExplicitDefintion, I suppose with intent to exclude phi nodes, but also excluding SsaVariableCapture by mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants