Switch from using mustPanic to mayReturnNormally to construct a call-expression's CFG by smowton · Pull Request #239 · github/codeql-go

smowton · 2020-06-30T16:48:55Z

We also use this to note that user-defined functions can only return normally if their CFG normal exit node is reachable, and annotate some well-known functions as noreturn.

For example, this will by fiat declare os.Exit noreturn (never returns normally), and will also notice that a user function func myExit() { os.Exit(1) } is also noreturn, because it doesn't have any control-flow edges that reach the normal return node.

Note that the test diff shows some expected changes to other functions' CFGs, which happens because stmts.go / test5 and a few other test functions cannot return.

There are also a couple of apparently-unrelated changes, but these turn out to be because most of test8 is in fact unreachable due to unconditionally calling test5 early on. A partial, unconnected CFG is generated for the unreachable statements before the absence of a normal exit edge is noticed. This may constitute a bug if we care about the internal consistency of the CFG of unreachable blocks.

intrigus-lgtm · 2020-06-30T16:56:19Z

+}
+
+/**
+ * A fatal log function, which calls os.Exit.


Suggested change

* A fatal log function, which calls os.Exit.

* A fatal log function, which calls `os.Exit`.

intrigus-lgtm · 2020-06-30T16:56:34Z

+}
+
+/**
+ * The os.Exit function, which ends the process.


Suggested change

* The os.Exit function, which ends the process.

* The `os.Exit` function, which ends the process.

owen-mc · 2020-07-01T08:36:16Z

+/**
+ * The Ginkgo Fail function, which always panics.
+ */


Suggested change

/**

* The Ginkgo Fail function, which always panics.

*/

/** The Ginkgo Fail function, which always panics. */

(and several other places)

owen-mc · 2020-07-01T08:38:29Z

I think you forgot to commit your .expected file

max-schaefer

Some suggestions for how to rearrange things and a few questions. Let's discuss how to evaluate this.

max-schaefer · 2020-07-01T08:37:14Z

  /** Holds if this function has no observable side effects. */
  predicate mayHaveSideEffects() { none() }

+  predicate mayReturnNormally() { any() }


Needs qldoc.

max-schaefer · 2020-07-01T08:38:14Z

+    not builtinFunction(getName(), _, _, _) or
+    builtinFunction(getName(), _, _, false)


Suggested change

not builtinFunction(getName(), _, _, _) or

builtinFunction(getName(), _, _, false)

not builtinFunction(getName(), _, _, true)

max-schaefer · 2020-07-01T08:39:00Z

+/**
+ * A function that cannot return normally, either because it panics, exits the process, or loops forever.
+ */
+abstract class NoreturnAnnotatedFunction extends Function {


Why do we need a class for this?

max-schaefer · 2020-07-01T08:39:27Z

+/**
+ * A fatal log function, which calls os.Exit.
+ */
+class FatalFunction extends NoreturnAnnotatedFunction {


This should go into Stdlib.qll, somewhere close to the other models of the log library.

max-schaefer · 2020-07-01T08:39:39Z

+/**
+ * The os.Exit function, which ends the process.
+ */
+class OsExitFunction extends NoreturnAnnotatedFunction {


Similarly, should go into Stdlib.qll.

max-schaefer · 2020-07-01T08:40:44Z

+/**
+ * The Ginkgo Fail function, which always panics.
+ */
+class GinkgoFailFunction extends NoreturnAnnotatedFunction {


This should go into Testing.qll.

max-schaefer · 2020-07-01T08:40:57Z

+ * A function for which we have source code.
+ */
+class UserDefinedFunction extends DeclaredFunction {
+  UserDefinedFunction() { not this instanceof NoreturnAnnotatedFunction and exists(getBody()) }


Why the not instanceof?

Allows one to extend NoreturnAnnotatedFunction in a query to mark a function noreturn which we do have a body for, but where the noreturn-ness is not obvious enough to be detected automatically.

OK, but couldn't that be achieved by simply extending UserDefinedFunction and overriding mayReturnNormally?

That does work, but you have to remember to do that rather than directly extend Function, otherwise you will match both UserDefinedFunction and MyFunctionExtension, and your override of mayReturnNormally has no effect

But I guess now you'd have to remember to override NoreturnAnnotatedFunction, so maybe that doesn't help. Basically I want to allow people to use class MyFn extends Function { override predicate mayReturnNormally ... } with the expected behaviour, rather than having to know that UserDefinedFunction exists and will be providing a competing definition

max-schaefer · 2020-07-01T08:41:56Z

@@ -0,0 +1,23 @@
+package main
+
+import ("os"; "log")


Have you run go fmt over this file? We generally do that for consistency.

smowton · 2020-07-01T11:32:08Z

@max-schaefer all comments applied

max-schaefer

LGTM, modulo a few minor comments and the outcome of the evaluation.

max-schaefer · 2020-07-01T13:44:55Z

  predicate mayHaveSideEffects() { none() }

+  /**
+   * Holds if this function may return without panicking, exiting the process or looping forever.


Suggested change

* Holds if this function may return without panicking, exiting the process or looping forever.

* Holds if this function may return without panicking, exiting the process, or looping forever.

max-schaefer · 2020-07-01T13:45:14Z

+   * return normally, but it never fails to hold for functions that can.
+   *
+   * Note this is declared here and not in DeclaredFunction so that queries can override this by
+   * extending Function rather than having to remember to extend DeclaredFunction.


Suggested change

* extending Function rather than having to remember to extend DeclaredFunction.

* extending `Function` rather than having to remember to extend `DeclaredFunction`.

max-schaefer · 2020-07-01T13:45:44Z

+   * This predicate is an over-approximation: it may hold for functions that can never
+   * return normally, but it never fails to hold for functions that can.
+   *
+   * Note this is declared here and not in DeclaredFunction so that queries can override this by


Suggested change

* Note this is declared here and not in DeclaredFunction so that queries can override this by

* Note this is declared here and not in `DeclaredFunction` so that library models can override this by

max-schaefer · 2020-07-01T13:47:27Z

+  class FailFunction extends Function {
+    FailFunction() { hasQualifiedName("github.com/onsi/ginkgo", "Fail") }
+
+    override predicate mayReturnNormally() { none() }


Do we need this? Doesn't the override of mustPanic() already entail this?

smowton · 2020-07-02T11:07:06Z

Eval findings:

Total time change for default.slugs negligible (<0.1%)
Best project -8%, worst +5%, both <30s runs
Long-runners cockroach -2.5%, aws-sdk +3.3%
The new stage including mayReturnNormally is 8% slower than the old one
43 new alerts from unreachable-statement, all correct
To reduce noise we may want to stop flagging unreachable code that consists solely of a return or panic statement, since Go doesn't have an explicit unreachable instruction. This would remove about 40 of the new alerts; the remainder really do feature unreachable attempts to dump more debug info or issue an HTTP response.

max-schaefer · 2020-07-02T13:12:19Z

Great! Would you happen to have a link to the report?

smowton · 2020-07-02T13:25:31Z

https://github.com/smowton/private-scratch/blob/master/pr-239-report.md

max-schaefer · 2020-07-02T14:29:50Z

Thanks! Perhaps you could take a stab at updating UnreachableStatement to not flag trivial return statements (i.e., all returned values are constants or something like that)? And similar for break and continue, I suppose.

smowton · 2020-07-07T08:34:32Z

@max-schaefer mergeable now that #246 is in?

max-schaefer

Basically LGTM, modulo some very minor nits.

…expression's CFG We also use this to note that user-defined functions can only return normally if their CFG normal exit node is reachable, and annotate some well-known functions as noreturn. For example, this will by fiat declare os.Exit noreturn (never returns normally), and will also notice that a user function `func myExit() { os.Exit(1) }` is also noreturn, because it doesn't have any control-flow edges that reach the normal return node.

smowton · 2020-07-07T10:46:25Z

@max-schaefer nits picked

Call-graph API cleanup

smowton requested a review from a team June 30, 2020 16:48

intrigus-lgtm reviewed Jun 30, 2020

View reviewed changes

owen-mc reviewed Jul 1, 2020

View reviewed changes

max-schaefer suggested changes Jul 1, 2020

View reviewed changes

smowton force-pushed the smowton/feature/find-noreturn-user-functions branch 2 times, most recently from 6823de9 to bd2ef51 Compare July 1, 2020 11:31

max-schaefer suggested changes Jul 1, 2020

View reviewed changes

smowton force-pushed the smowton/feature/find-noreturn-user-functions branch from bd2ef51 to 7676101 Compare July 1, 2020 13:52

max-schaefer suggested changes Jul 7, 2020

View reviewed changes

smowton force-pushed the smowton/feature/find-noreturn-user-functions branch from 7676101 to 6e5ee47 Compare July 7, 2020 10:40

max-schaefer approved these changes Jul 7, 2020

View reviewed changes

max-schaefer merged commit 47a8586 into github:master Jul 7, 2020

ceh pushed a commit to ceh-forks/codeql-go that referenced this pull request Jul 22, 2020

Merge pull request github#239 from max/virtual-dispatch

ae96bd8

Call-graph API cleanup

owen-mc added the no-change-note-required label Aug 26, 2020

	* A fatal log function, which calls os.Exit.
	* A fatal log function, which calls `os.Exit`.

	* The os.Exit function, which ends the process.
	* The `os.Exit` function, which ends the process.

		not builtinFunction(getName(), _, _, _) or
		builtinFunction(getName(), _, _, false)

	not builtinFunction(getName(), _, _, _) or
	builtinFunction(getName(), _, _, false)
	not builtinFunction(getName(), _, _, true)

	* Holds if this function may return without panicking, exiting the process or looping forever.
	* Holds if this function may return without panicking, exiting the process, or looping forever.

	* extending Function rather than having to remember to extend DeclaredFunction.
	* extending `Function` rather than having to remember to extend `DeclaredFunction`.

	* Note this is declared here and not in DeclaredFunction so that queries can override this by
	* Note this is declared here and not in `DeclaredFunction` so that library models can override this by

Conversation

smowton commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc commented Jul 1, 2020

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smowton commented Jul 1, 2020

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Jul 2, 2020

Uh oh!

max-schaefer commented Jul 2, 2020

Uh oh!

smowton commented Jul 2, 2020

Uh oh!

max-schaefer commented Jul 2, 2020

Uh oh!

smowton commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smowton commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

smowton commented Jul 7, 2020 •

edited

Loading