Dataflow: Deprecate BarrierGuard class #9618

aschackmull · 2022-06-20T13:48:33Z

Deprecates the BarrierGuard class for all languages using the shared data flow library, and replaces existing usage with the BarrierGuard parameterised module.

See initial Java implementation in #9294

go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll

+abstract deprecated class BarrierGuard extends Node {
+  /** Holds if this guard validates `e` upon evaluating to `branch`. */
+  abstract predicate checks(Expr e, boolean branch);
+
+  /** Gets a node guarded by this guard. */
+  final Node getAGuardedNode() {
+    result = BarrierGuard<barrierGuardChecks/3>::getABarrierNodeForGuard(this)
+  }
+}


RasmusWL · 2022-06-20T19:49:46Z

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll

+ */
+module BarrierGuard<guardChecksSig/3 guardChecks> {
+  /** Gets a node that is safely guarded by the given guard check. */
+  ExprNode getABarrierNode() {


@yoff should this be ExprNode or CfgNode? It seems the only difference is that CfgNodes can also be patterns from a match statement. I simply don't know whether such patterns should be eligible for being the result of a guards check 🤔 Anyway, I was just a bit suspicious about this since we use ExprNode very little in our code, and thought it might just be a leftover from @aschackmull normally using that for Java.

codeql/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll

Lines 27 to 32 in 02614e1

/** A node corresponding to a control flow node. */

TCfgNode(ControlFlowNode node) {

isExpressionNode(node)

or

node.getNode() instanceof Pattern

} or

codeql/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll

Lines 267 to 268 in 02614e1

class ExprNode extends CfgNode {

ExprNode() { isExpressionNode(node) }

Feel free to change it as a follow-up. I just kept the status quo in this case.

atorralba

Java 👍

michaelnebel

C# Looks plausible to me!

smowton

Directly resolved some ql-for-ql doc complaints; with that Go 👍

nickrolfe

The Ruby changes look good to me.

Are you running DCA comparisons?

ruby/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md

aschackmull · 2022-06-21T12:07:03Z

Are you running DCA comparisons?

I didn't plan to.

cpp/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md

csharp/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md

go/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md

java/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md

python/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md

ruby/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md

RasmusWL

Python: Besides the point I already raised for @yoff, this LGTM 👍

RasmusWL · 2022-06-22T07:49:33Z

python/ql/lib/semmle/python/dataflow/new/BarrierGuards.qll

+
 /** A validation of unknown node by comparing with a constant string value. */
-class StringConstCompare extends DataFlow::BarrierGuard, CompareNode {
+class StringConstCompareBarrier extends DataFlow::Node {


I wondered for a second whether this should be called StringConstCompareSanitizer, but I think using StringConstCompareBarrier is fine 👍 (and consistent with the new name in Ruby at least)

Sounds like this is good to merge, then.

michaelnebel

C# Looks plausible to me!

hvitved · 2022-08-09T09:46:22Z

cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImpl2.qll

 }

+/** A bridge class to access the deprecated `isBarrierGuard`. */
+private class BarrierGuardGuardedNodeBridge extends Unit {


Neat trick for accessing a deprecated predicate without getting a deprecation warning.

aschackmull added 11 commits June 16, 2022 11:25

Java: Deprecate BarrierGuard class.

33deff9

Dataflow: Sync.

6518a01

C#: Add BarrierGuard parameterised module.

456f02f

C#: Replace deprecated barrier guards.

1b374e2

C/C++: Deprecate BarrierGuard class.

bbb8d29

Go: Deprecate and replace BarrierGuard class

406f5b5

Go: Ad-hoc patch the shared libs.

87d5305

Python: Deprecate and replace BarrierGuard class.

f473a0a

Ruby: Deprecate and replace BarrierGuard class.

1b13790

Python: adjust test.

a7c268f

Swift: Deprecate BarrierGuard.

730871c

github-actions bot added C# C++ Go Java Python Ruby Swift labels Jun 20, 2022

This was referenced Jun 20, 2022

Dataflow: Deprecate BarrierGuard class #9590

Closed

Ruby: Make StringArrayInclusion more sensitive #9138

Merged

github-advanced-security bot found potential problems Jun 20, 2022

View reviewed changes

RasmusWL reviewed Jun 20, 2022

View reviewed changes

aschackmull added 3 commits June 21, 2022 09:19

Python: one more fix

a6c0a9e

Add change notes.

a4796e1

Ruby: Remove test.

736372f

aschackmull marked this pull request as ready for review June 21, 2022 09:19

aschackmull requested review from a team as code owners June 21, 2022 09:19

github-actions bot added the documentation label Jun 21, 2022

MathiasVP previously approved these changes Jun 21, 2022

View reviewed changes

atorralba previously approved these changes Jun 21, 2022

View reviewed changes

michaelnebel previously approved these changes Jun 21, 2022

View reviewed changes

Update doc for divideByZeroSanitizerGuard

8ae4c21

smowton dismissed stale reviews from michaelnebel, atorralba, and MathiasVP via 8ae4c21 June 21, 2022 11:11

Update comparisonBarrierGuard qldoc

7bb0d62

smowton previously approved these changes Jun 21, 2022

View reviewed changes

nickrolfe reviewed Jun 21, 2022

View reviewed changes

ruby/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md Outdated Show resolved Hide resolved

aschackmull commented Jun 21, 2022

View reviewed changes

Apply suggestions from code review

f8f9b7d

aschackmull dismissed smowton’s stale review via f8f9b7d June 21, 2022 12:11

RasmusWL reviewed Jun 22, 2022

View reviewed changes

michaelnebel self-requested a review June 22, 2022 08:39

michaelnebel approved these changes Jun 22, 2022

View reviewed changes

aschackmull merged commit df6d68b into github:main Jun 22, 2022

aschackmull deleted the dataflow/deprecate-barrierguard-class branch June 22, 2022 08:44

erik-krogh mentioned this pull request Jun 23, 2022

Refactorizations of the ReDoS libraries #9422

Merged

hvitved reviewed Aug 9, 2022

View reviewed changes

owen-mc added the DataFlow Library label Nov 10, 2022

	/** A node corresponding to a control flow node. */
	TCfgNode(ControlFlowNode node) {
	isExpressionNode(node)
	or
	node.getNode() instanceof Pattern
	} or

	class ExprNode extends CfgNode {
	ExprNode() { isExpressionNode(node) }

Dataflow: Deprecate BarrierGuard class #9618

Dataflow: Deprecate BarrierGuard class #9618

Uh oh!

Conversation

aschackmull commented Jun 20, 2022 • edited by RasmusWL Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Check failure

RasmusWL Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

aschackmull Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

atorralba left a comment

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

nickrolfe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aschackmull commented Jun 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

RasmusWL Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

aschackmull Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

hvitved Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

aschackmull commented Jun 20, 2022 •

edited by RasmusWL

Loading