Skip to content

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented May 6, 2020

@aschackmull and @jbj : We will need a similar change note for Java and C++; can you either provide me the relevant examples, or create the PRs yourself, thanks?

@hvitved hvitved added the C# label May 6, 2020
@aschackmull
Copy link
Contributor

Java version:

class C1 {
    String f1;
    C1(String f1) { this.f1 = f1; }
}
class C2 {
    C1 f2;
    String getF2F1() {
        return this.f2.f1; // Nested field read
    }
    void m() {
        this.f2 = new C1("taint");
        sink(this.getF2F1()); // NEW: "taint" reaches here
    }
}

@hvitved hvitved changed the title C#: Add change note for #3110 C#/Java: Add change note for #3110 May 6, 2020
@hvitved hvitved added the Java label May 6, 2020
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.

Thanks for adding the change note @hvitved. Great stuff!

class C2
{
C1 F2;

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an extra line here.

through methods now takes nested field reads/writes into account. For example, the library is
able to track flow from `"taint"` to `Sink()` via the method `GetF2F1()` in
```csharp
class C1
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 the example might read better if C1 and F1 could be replaced by real-world examples like Person and Surname. Just an idea.


void M()
{
this.F2 = new C1() { F1 = "taint" };
Copy link
Contributor

Choose a reason for hiding this comment

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

this. is not really idiomatic in C# and could be removed.

@@ -24,5 +24,28 @@ The following changes in version 1.25 affect C# analysis in all applications.
have type parameters. This means that non-generic nested types inside construced types,
such as `A<int>.B`, no longer are considered unbound generics. (Such nested types do,
however, still have relevant `.getSourceDeclaration()`s, for example `A<>.B`.)
* The data-flow library has been improved, which affects and improves most security queries. Flow
Copy link
Contributor

Choose a reason for hiding this comment

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

affects and

I would also state exactly how it improves it, for example that it will find more data-flow steps and potentially more results.

@jbj
Copy link
Contributor

jbj commented May 7, 2020

I'll make a separate PR for the C++ version next week, integrating it with other data-flow improvements. I'll put this issue on our team board as a way to remember it.

@MathiasVP
Copy link
Contributor

I'll make a separate PR for the C++ version next week, integrating it with other data-flow improvements. I'll put this issue on our team board as a way to remember it.

If you haven't already written an example feel free to use this port of the Java one:

struct C {
    int f1;
};

struct C2
{
    C f2;

    int getf2f1() {
        return f2.f1; // Nested field read
    }

    void m() {
        f2.f1 = taint();
        sink(getf2f1()); // NEW: "taint" reaches here
    }
};

aschackmull
aschackmull previously approved these changes May 7, 2020
@jbj
Copy link
Contributor

jbj commented May 7, 2020

Thanks, @MathiasVP, for quickly porting the example. If you haven't done so already, will you also turn it into a qltest to check that it actually works?

@hvitved, please use that example for C++.

@MathiasVP
Copy link
Contributor

Thanks, @MathiasVP, for quickly porting the example. If you haven't done so already, will you also turn it into a qltest to check that it actually works?

I checked that the flow was missing prior to #3110, and that we get it after merging the PR. Here's a PR with the qltest: #3426

@hvitved hvitved changed the title C#/Java: Add change note for #3110 C#/Java/C++: Add change note for #3110 May 7, 2020
@hvitved hvitved added the C++ label May 7, 2020
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

I've suggested some C/C++-specific tweaks to the change note.

Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

Thanks, Tom! Ping @calumgrant, who has a negative review active.

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.

Thanks for the changes @hvitved.

@calumgrant calumgrant merged commit f5daeea into github:master May 13, 2020
@hvitved hvitved deleted the csharp/dataflow/change-note branch May 13, 2020 13:02
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.

5 participants