C#: Include runtime target in TTransitiveCaptureCall

[lcartey]

TTransitiveCaptureCall represents a control flow node that may transitively call many different callables which capture a variable from the current scope. Captured variables are represented as synthetic parameters to the callable, at negative indices. However, each of the different targets may capture a different subset of variables from the enclosing scope, so we must include the target callable in the TTransitiveCaptureCall in order to prevent incorrect capture flow between unrelated capture variables.

For example, in the included test case:

        void CaptureTest(string nonSink0, string sink39)
        {
            RunAction(() =>       // Check each lambda captures the correct arguments
            {
                Check(nonSink0);
                RunAction(() =>
                {
                    Check(sink39);
                });
            });
        }
        CaptureTest("not tainted", tainted);

nonSink0 is captured in the outer lambda as a parameter at index -2. However, the parameter at index -2 in the nested lambda is sink39. Without the runtime target context, we get flow from the parameter sink39 to the access of nonSink0 in Check(nonSink0).

Tests are added in two commits; the first adds the new test cases, but commented out, and adjusts the expected result files to use the new locations. The second commit uncomments the new tests, and adds the new rows to the expected result files

@hvitved I've submitted this against rc/1.22, as this issue is affecting a customer.

[hvitved]

Thanks a lot for fixing this, Luke!

[hvitved]

I believe this should be TTransitiveCapturedCall(cfn, n.getEnclosingCallable())

[hvitved]

And would be nice with a test for that case as well, i.e., two nested lambdas that both write to a captured variable, but only one of them writes a tainted value.

[lcartey]

In theory, I think you're right. In practice, I was unable to find a test case where this actually made a difference. I think this is because OutNode.getCall() is not used in practice in the dataflow libraries. Instead DataFlowDispatch::Cached::getAnOutNode(DataFlowCall call, ReturnKind kind) associates OutNodes with DataFlowCalls in such a way that the runtime target is respected. I could be wrong, though! 🙂

In any case, I've made the change as suggested, and added a test case for captured out variables (whose results did not change after this line was modified).

[hvitved]

You are right; in fact it is because the return kind, ImplicitCapturedReturnKind, explicitly carries the captured variable that the two updates do not get mixed up.

[felicitymay]

Is this something that should be mentioned in the release notes for 1.22.1?

[lcartey]

@felicitymay yes, sorry - I remembered only after Tom merged. I will open a separate PR to add a release note for this.

[felicitymay]

Actually, @lcartey - at this point, you can simply add the change to the draft release notes page in the wiki.

[felicitymay]

Brief change note added to: https://wiki.semmle.com/display/SDmaster/Semmle+1.22+-+release+notes following advice from Tom.