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: Fix data flow through variable defined in type switch guard #16120

Merged
merged 8 commits into from
Apr 11, 2024

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Apr 4, 2024

Reported here.

Consider a typical type switch statement.

	switch inputWithType := input.(type) {
	case Type1:
		output1 = f(inputWithType)
	case Type2:
		output1 = g(inputWithType)
	}

It looks like a new variable inputWithType is defined in the guard, and is used in the case clauses. But the go compiler can't represent it like that internally because inputWithType has a different type in each case clause. So it has to make a variable for each case clause. The CodeQL Go library is not currently dealing with this. We end up with an object for inputWithType which doesn't have a declaration, which means that there is no SsaDefinition relating to it. This causes data flow to not work.

I tried fixing this in the extractor, but it was a bit messy, and I realised it was better to accept that these variables don't have an explicit declaration and add an IR::Instruction for their implicit declaration. This causes SSA to work properly again. I have added tests for control flow and data flow.

@github-actions github-actions bot added the Go label Apr 4, 2024
go/extractor/extractor.go Outdated Show resolved Hide resolved
go/ql/lib/go.dbscheme Outdated Show resolved Hide resolved
go/ql/lib/semmle/go/Scopes.qll Outdated Show resolved Hide resolved
go/ql/lib/semmle/go/controlflow/IR.qll Outdated Show resolved Hide resolved
go/ql/lib/semmle/go/controlflow/IR.qll Outdated Show resolved Hide resolved
go/ql/lib/semmle/go/Scopes.qll Outdated Show resolved Hide resolved
@smowton
Copy link
Contributor

smowton commented Apr 4, 2024

Liking that we apparently don't even need to touch the SSA layer -- this becomes just another instruction that defines a fresh variable 👍

@owen-mc
Copy link
Contributor Author

owen-mc commented Apr 6, 2024

I found a way to identify the implicitly declared objects without a new table. This simplifies the PR a bit.

@owen-mc owen-mc force-pushed the go/fix/type-switch-control-flow branch from 8027f3a to 74711ec Compare April 8, 2024 23:11
@owen-mc owen-mc force-pushed the go/fix/type-switch-control-flow branch from 74711ec to a65b02e Compare April 9, 2024 10:04
@owen-mc owen-mc marked this pull request as ready for review April 9, 2024 10:26
@owen-mc owen-mc requested a review from a team as a code owner April 9, 2024 10:26
smowton
smowton previously approved these changes Apr 9, 2024
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

QL changes look good; trivial suggestion re: change note. Haven't examined the test expectations in detail.

Co-authored-by: Chris Smowton <[email protected]>
@owen-mc
Copy link
Contributor Author

owen-mc commented Apr 11, 2024

QA results all look good.

@owen-mc owen-mc merged commit d4bb4d4 into github:main Apr 11, 2024
14 checks passed
@owen-mc owen-mc deleted the go/fix/type-switch-control-flow branch April 11, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants