What do we really want here?

• Is it because we want to catch the following:

  void myFunc(void* p) { ... }
  free(p);
  myFunc(p);

  and we have to stop at the argument because we're doing local-only flow?

  If so, I think we should be a bit smarter here and identify "functions that always dereference a parameter". We can do that by checking if there's local flow from a parameter node to a use, and ensuring that the use always post-dominates the parameter (very much like what we're doing in the query already).

• Or is it because we want to catch this (which I doubt we'd actually see in the wild, and I think this would even be included in the PointerDereferenceExpr case):

  free(myFunc);
  myFunc();

The first case. The intention is flag passing a pointer pointing to de-allocated memory to a function, indeed

We could extend the analysis to be interprocedural, but I would argue that passing an invalid pointer to any function should be considered harmful, and I have not seen any FP's for that case.
The only point it would make sense is if the callee just ignores the argument.

It should be noted that we block flow though CallByReference.

Followup after a sync talk. This would avoid an FP in

void *a = ...
free(a);
printf("%p", a);

As we've discussed offline, this should be replaced with the IR versions. Sadly, the IR doesn't yet include delete so this may cause us to lose results 😭. If this is unacceptable (which it very well might be) it should at least be changed to use the basic block version of these predicates (since the control-flow node version of these won't scale to large databases).

We already do use the IR flow so we are already missing these results, so switchig should not cause harm. It would supposedly also remove one FP in the tests.

Oh, right. Good point. I can push a IR-ified version of this to the PR later today then 👍.

Yeah, this makes sense. delete x isn't in the IR, so there's no dataflow from that x to use(x).

So this case is for arguments that are always dereferenced by the function.

Why does DataFlow not find these results satisfactorily without a special case?

Normal interprocedural will find this result, but because we're adding the extra condition that either:

• The free dominates the use, or
• The use post-dominates the free

And domination/post-domination is only defined intraprocedurally. So this query is only finding cases of use-after-free when the use happens in the same function as the free (because neither of the above conditions will hold when the source and sink are in different functions). So we won't catch stuff like:

void use(char* p) { /* ... reading *p somewhere in here ... */ }
void f() {
  free(p);
  use(p);
}

since *p free doesn't dominate *p.

So to get a slightly better TP rate, we use local dataflow to identify the functions that will always dereference one of their parameters (in this case, the first parameter of use is one such function). And we then include use(p) as a sink in the configuration. And since use(p) is dominates by free(p) this free/use pair will satisfy one of the criteria (in fact, both of them in this case) above.

An alternative idea would be to define a concept of interproceudral domination/post-domination. That would probably amount to the same amount of code as this find-always-dereferenced-arguments strategy, though. It would give us slightly better paths, though 🤔 (since, with the current approach, we show the path up to the call to the always-dereferencing-function, and not actually all the way to the dereference operation)

Thanks for the explanation, makes sense I think.

Why do we need a FlowState to track the source?

Because we need to implement the extra condition we place on the source/sink pair:

• The source dominates the sink, or
• The sink post-dominates the source

So in the isSink definition in the FlowFromFree module, we use the flow state to check the domination/post-domination relationship between the source and the sink.

I think I'd have done that check in the query where we have access to both the source and sink without need of a FlowState ... but then this approach puts the code in the shared library and keeps the query code cleaner.

The good thing about doing it inside the configuration is that we only generate the dataflow paths that actually satisfy the criteria. If we move the check to the query we'd generate all paths from a source to a sink, and only then filter out the ones that don't satisfy any of the criteria.

Why do we check e != state? Perhaps an example would help me see this.

Yeah, that's a bit annoying. I was seeing some FPs where, for some reason, when there was a conversion on the free argument, we'd wind up with FPs where we claimed that there was a double-free in a case such as:

void free(void*);

void f(char* p) {
  free(p);
}

(I think, because dataflow goes (void*)p -> p -> (void*)p, which is clearly wrong). I'd like to investigate that in a PR after this, though. For now, the check that the source expression isn't the same as the sink expression does the job, though.

I know that's not a super satisfying answer 😂.

That's fine, it's a practical workaround that we hopefully won't need forever.

Feel free to ignore if deallocate is a word that's commonly used for this. (Just to let you know that I didn't find the term on Merriam Webster or the Microsoft style guide).

Thanks for the heads up. It's a commonly used word in this context, so I'll keep the terminology as-is :)

Should this be capitalized as we often refer to DoS?

Good question. A quick search in our repo shows that we're now quite consistent with the capitalization: https://github.com/search?q=repo%3Agithub%2Fcodeql%20%22Denial-of-Service%22&type=code. I think "denial-of-service" wins by a small margin, so I'll leave it as-is for now.

Should the new query be cpp/double-free?

This PR contains two new queries: A new query with the id cpp/double-free (whose change note is added here) and another query cpp/use-after-free (whose change note is added here). So I think this is correct.