search

crypto-com / cosmos-sdk-codeql

A query suite for common bug patterns in Cosmos SDK-based applications
Apache License 2.0
20 stars 6 forks source link

Changes to beginendblock-panic query #4

Closed ivan-gavran closed 1 year ago

ivan-gavran commented 1 year ago

Connected to the question in the issue #3, in this PR I changed two things:

  1. replaced the mayPanic() with mustPanic()
  2. Changed the way parent-child relation was modelled
tomtau commented 1 year ago

@ivan-gavran I just tried on the ~latest~ https://github.com/cosmos/cosmos-sdk/commit/8076144 Cosmos SDK repo and isLikelyFalsePositive doesn't make a difference for mustPanic:

223 is still a bit too much though -- a maintainer would likely just ignore that, especially as the current query doesn't show them the ABCI context in which each panic call happens. @ivan-gavran would you take a stab at converting it to a path query: https://codeql.github.com/docs/writing-codeql-queries/creating-path-queries/#creating-path-queries , i.e. changing to * @kind path-problem so that the path from those ABCI methods to the panic calls can be visualized?

ivan-gavran commented 1 year ago

Gladly! Expect an update to the PR in the next days.

Regarding the discussion about the (large) number of reported result: I don't think that comes from using mustPanic vs mayPanic: indeed, mustPanic gives fewer results! But the difference comes from changing the parent-child relation. Previously, it was simply marking all the (direct!) function calls within an EndBlock/BeginBlock, whereas now it explores the full tree of function calls. What do you think, do you agree or is there something I am missing?

tomtau commented 1 year ago

Yes, I agree

ivan-gavran commented 1 year ago

I added the path version of the query and removed the isLikelyFalsePositive predicate.

I decided not to modify the existing query and add a new path one because there are many more potential paths to panics that just panics reachable from begin/end blockers.

The assumed workflow would be to first see what are the panics and use the path query for inspection, perhaps modifying it by narrowing down to a particular panic that one wants to inspect.

tomtau commented 1 year ago

@ivan-gavran thanks, I just tried and there are perhaps 10x more path results, so I think it makes sense