Skip to content

C#: Introduce BarrierGuards#1807

Merged
semmle-qlci merged 3 commits intogithub:masterfrom
hvitved:csharp/dataflow/barrier-guard
Aug 30, 2019
Merged

C#: Introduce BarrierGuards#1807
semmle-qlci merged 3 commits intogithub:masterfrom
hvitved:csharp/dataflow/barrier-guard

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 23, 2019

The equivalent of Java's #1718.

@hvitved hvitved added the C# label Aug 23, 2019
@hvitved hvitved requested a review from a team as a code owner August 23, 2019 11:20

/** A guard that checks if this expression is non-`null`. */
class NullGuard extends DataFlow::BarrierGuards::ValueBarrierGuard {
NullGuard() { this.getCheckedValue() = any(AbstractValues::NullValue nv | not nv.isNull()) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work as all BarrierGuards must be disjoint. In this case there won't be a difference between NullGuard and AntiNullGuard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point; BarrierGuards are a bit fragile...

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed they are. Until we get extensible IPA types with an agreed-upon syntax, I don't think there's much to do about that, though.

override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }

override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
guard instanceof BarrierGuard
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is confusing. I read it as if you were adding all BarrierGuards until I saw that you had defined another abstract class with the same name.

Copy link
Contributor

Choose a reason for hiding this comment

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

The difference being that BarrierGuard is an already existing class. Having abstract class Sink extends DataFlow::Node is fine as is quite clear that there's an important distinction between Sink and Node, but doing abstract class BarrierGuard extends DataFlow::BarrierGuard is confusing since then you suddenly have a very important distinction between two classes with identical names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I think my comment was lost, it was: Yeah, but this is the general pattern in C# queries, for extensibility.)

@hvitved hvitved force-pushed the csharp/dataflow/barrier-guard branch from bc9a16b to 5f66b9e Compare August 28, 2019 11:21
@hvitved
Copy link
Contributor Author

hvitved commented Aug 28, 2019

Rebased to resolve conflict in change note.

@hvitved hvitved force-pushed the csharp/dataflow/barrier-guard branch from 5f66b9e to 6e7ef66 Compare August 30, 2019 07:37
@hvitved
Copy link
Contributor Author

hvitved commented Aug 30, 2019

Rebased to resolve conflict in change note (again).

Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

Very nice.

@semmle-qlci semmle-qlci merged commit 394563d into github:master Aug 30, 2019
@hvitved hvitved deleted the csharp/dataflow/barrier-guard branch August 30, 2019 11:40
@hvitved
Copy link
Contributor Author

hvitved commented Sep 1, 2019

Here is the dist-compare report: https://git.semmle.com/gist/tom/75b4bdb0e981aa92f69770f64912116e.

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.

4 participants