Skip to content

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Jun 10, 2020

Allowing field flow through ChiInstructions that are conflated into {AllAliasedMemory} gives us quite a few good results in qltest (see the comments in https://github.com/github/codeql-c-analysis-team/issues/64#issuecomment-640606009).
It does, however, also opens the door for some false positives. I've added one such false positive in simple.cpp.

The question now is: how many false positives will this actually result in. I've started a CPP-difference to check for this: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1187/

One option (as suggested by @jbj) if we get a lot of false positives is to start using the type pruning feature from the shared dataflow library.

@MathiasVP MathiasVP added the C++ label Jun 10, 2020
@MathiasVP
Copy link
Contributor Author

MathiasVP commented Jun 12, 2020

Removing the restriction on non-conflatedness on the flow from WriteSideEffectInstruction to ChiInstruction in simpleInstructionLocalFlowStep gave lots of new results. Most of them were false positives, but the following looks like true positives:

Git
  cpp/uncontrolled-process-operation (shell.c:189)
  cpp/path-injection (builtin/init-db.c:127)
  cpp/path-injection (merge-recursive.c:3356)

g/openjdk/jdk
  cpp/unbounded-write (src/hotspot/os/linux/os_linux.cpp:401)
  cpp/unbounded-write (src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:230)
  cpp/unbounded-write (src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:239)
  cpp/unbounded-write (src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:242)
  cpp/unbounded-write (src/hotspot/share/runtime/arguments.cpp:134)

Unfortunately, when we remove the non-conflatedness only from the PostUpdateNode (but keep it for the flow in simpleInstructionLocalFlowStep I described above) we lose all of these results. I've start a new CPP-difference to check what happens when we merge the latest changes from master (which includes #3419).

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Jun 15, 2020

Sadly removing the non-conflatedness restriction in only a few places results in another false positive on php in cpp-differences, and without removing the restriction in several places we don't get any new true positives "for free".

If we are to continue along the route of allowing field flow through conflated chi instructions I think we need to start doing type pruning. I'm closing this PR until we have a more clear idea about what we need to do.

@MathiasVP MathiasVP closed this Jun 15, 2020
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.

1 participant