C/C++ : memory may not be freed on loop

[Yonah125]

This query finds memory that might no be freed in a loop.

For exemple :

for(int i = 0; i < 10; i++){
     char* notfree = malloc(0x100);
     if(i == 5){
         break;
         }
      free(notfree)
 }

A resultat was found in netcdf-c : Unidata/netcdf-c#2339

Supervised by @catenacyber

[geoffw0]

This is a promising idea for a query, thank you for your contribution!

I think we should clean up the QL a bit - I've made some initial suggestions. In general adding \\ comments to blocks of code explaining what they do would help make the QL more readable and maintainable.

[geoffw0]

Again it may be worth calling out the predicates that are copies, as ideally at some point we would want to clean this up to have a single version of each in a .qll library somewhere.

[geoffw0]

Do you think we'd get good results if we included ReturnStmt as well?

[geoffw0]

I think this line is redundant, given the line below.

[geoffw0]

Is checking sameLoop here redundant (given the previous line bs.getChild(y) = is or...)?

[geoffw0]

I've run the tests on this PR, and it looks like you need to autoformat the QL file. Let me know if you don't know how to do this.

Error: ql/cpp/ql/src/experimental/Critical/MemoryMayNotBeFreedOnLoop.ql would change by autoformatting.

I also started an LGTM run to see what kinds of real world results we get from this query: https://lgtm.com/query/7023328987326235286/ . It looks like there are some good results, but sometimes a very large number of locations are reported.

[geoffw0]

Hi @Yonah125, please could you let us know if you intend to do any more work on this PR? I'm OK with merging this into experimental as it is - but there are some unaddressed comments, in particular the real world results that reference very large numbers of locations. I think this query would also benefit greatly from a good test (in cpp/ql/test/experimental).

[catenacyber]

Hi @geoffw0, I think @Yonah125 is done with this after his latest push. How does this look ?

[geoffw0]

I think @Yonah125 is done with this after his latest push. How does this look ?

Yes I think you're right. Its looks promising but a little unfinished to me. But for merging into experimental that's OK, it can potentially be built upon in future.

I've started the checks, will merge if they pass.

[geoffw0]

AllocAndFree.qll needs autoformatting.

[geoffw0]

Suggested change

	is.getThen().getAChild() = b or
	is.getThen().getAChild() = b

[catenacyber]

Friendly ping @Yonah125 could you do these changes ?