Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data flow: Fix a bad join order #18457

Merged
merged 1 commit into from
Jan 10, 2025
+7 −1
Merged

Data flow: Fix a bad join order #18457

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,12 +595,18 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
}

pragma[nomagic]
private ReturnKindExtOption getDisallowedReturnKind(ParamNodeEx p) {
private ReturnKindExtOption getDisallowedReturnKind0(ParamNodeEx p) {
if allowParameterReturnInSelfEx(p)
then result.isNone()
else p.isParameterOf(_, result.asSome().(ParamUpdateReturnKind).getPosition())
}

bindingset[p]
pragma[inline_late]
private ReturnKindExtOption getDisallowedReturnKind(ParamNodeEx p) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going wrong? This predicate is binary, nomagic'ed and only used in two (nomagic'ed) contexts where the result isn't otherwise bound, so I fail to see what this transformation achieves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is needed because fwdFlowIn is inlined inside fwdFlowIsEntered. Without this change, each recursive iteration of fwdFlowIsEntered starts at getDisallowedReturnKind, instead of the delta. The DIL before is

noinline
nomagic
incremental
`DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowIsEntered/3#137febc2`(
  /* DataFlowDispatch::DataFlowCall */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCall call,
  /* Option::Option<DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::ReturnKindExt>::Option */ `Option#8eb11f23::Option<DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::ReturnKindExt>::TOption` disallowReturnKind,
  boolean cc
)
{
  [base_case] false()
  [recursive_case]
  exists(
    /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::ParamNodeEx */ `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` p
   |
    (
      (
        exists(
          /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` dummyField
         |
          delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlow/2#65dc322b`(dummyField,
            cc) and
          `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::viableParamArgEx/3`(call,
            p, dummyField)
        ) and
        (
          cc = false
          or
          (
            cc = true and
            not(
              `project#DataFlowImplCommon::Cached::CachedCallContextSensitivity::reducedViableImplInCallContext/3#f0f91b5d`(call)
            )
          )
        ) and
        not(
          `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::fullBarrier/1#e25deefa`(p)
        )
      )
      or
      exists(
        /* DataFlowDispatch::DataFlowCallable */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCallable target
       |
        cc = true and
        delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::viableImplInSomeFwdFlowCallContextExt/1#23a13296`(call,
          target) and
        exists(
          /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ dontcare `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` _
         |
          previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowInReducedViableImplInSomeCallContext/4#ccb02962`(call,
            _, p, target)
        )
      )
      or
      exists(
        /* DataFlowDispatch::DataFlowCallable */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCallable target
       |
        cc = true and
        exists(
          /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ dontcare `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` _
         |
          delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowInReducedViableImplInSomeCallContext/4#ccb02962`(call,
            _, p, target)
        ) and
        previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::viableImplInSomeFwdFlowCallContextExt/1#23a13296`(call,
          target)
      )
    ) and
    `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::getDisallowedReturnKind0/1#0f59df6c`(p,
      disallowReturnKind)
  ) and
  not(
    previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowIsEntered/3#137febc2`(call,
      disallowReturnKind, cc)
  )
}

and afterwards

noinline
nomagic
incremental
`DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowIsEntered/3#137febc2`(
  /* DataFlowDispatch::DataFlowCall */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCall call,
  /* Option::Option<DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::ReturnKindExt>::Option */ `Option#8eb11f23::Option<DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::ReturnKindExt>::TOption` disallowReturnKind,
  boolean cc
)
{
  [base_case] false()
  [recursive_case]
  exists(
    /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::ParamNodeEx */ `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` p
   |
    `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::getDisallowedReturnKind/1#a2c82816`(p,
      disallowReturnKind) and
    (
      (
        exists(
          /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` dummyField
         |
          not(
            `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::fullBarrier/1#e25deefa`(p)
          ) and
          `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::viableParamArgEx/3`(call,
            p, dummyField) and
          delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlow/2#65dc322b`(dummyField,
            cc)
        ) and
        (
          cc = false
          or
          (
            cc = true and
            not(
              `project#DataFlowImplCommon::Cached::CachedCallContextSensitivity::reducedViableImplInCallContext/3#f0f91b5d`(call)
            )
          )
        )
      )
      or
      (
        exists(
          /* DataFlowDispatch::DataFlowCallable */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCallable target
         |
          exists(
            /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ dontcare `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` _
           |
            previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowInReducedViableImplInSomeCallContext/4#ccb02962`(call,
              _, p, target)
          ) and
          delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::viableImplInSomeFwdFlowCallContextExt/1#23a13296`(call,
            target)
        ) and
        cc = true
      )
      or
      (
        exists(
          /* DataFlowDispatch::DataFlowCallable */ DataFlowDispatch#2409fd0c::Cached::TDataFlowCallable target
         |
          exists(
            /* DataFlowImplCommon::MakeImplCommon<Location::Location, DataFlowImplSpecific::CsharpDataFlow>::NodeEx */ dontcare `DataFlowImplCommon#f7de413b::MakeImplCommon<Location#a178a2aa::Location,DataFlowImplSpecific#b13302e2::CsharpDataFlow>::Cached::TNodeEx` _
           |
            delta previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowInReducedViableImplInSomeCallContext/4#ccb02962`(call,
              _, p, target)
          ) and
          previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::viableImplInSomeFwdFlowCallContextExt/1#23a13296`(call,
            target)
        ) and
        cc = true
      )
    )
  ) and
  not(
    previous rec `DataFlowImpl::MakeImpl<Location::Location,DataFlowImplSpecific::CsharpDataFlow>::Impl<XSSQuery::XssTracking::C>::Stage1::fwdFlowIsEntered/3#137febc2`(call,
      disallowReturnKind, cc)
  )
}

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a pretty clear optimiser bug, I think: putting prev before delta like that.

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 actually hadn't spotted that, only that getDisallowedReturnKind was pushed to the top, good spot.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the actual reason why this join-order turns out poorly. The bindingset that we add in this PR isn't actually restricting the join-orderer in any way for the reason I stated, and that's what confused me - it's only giving a slight nudge that happens to cause the opimiser to dodge its bug.

result = getDisallowedReturnKind0(p)
}

/**
* Holds if an argument to `call` is reached in the flow covered by `fwdFlow`.
*/
Expand Down
Loading