IntersectMBO/cardano-node

[BUG] - Several potential bugs found using an automated code analysis tool

zmandel opened this issue · 7 comments

zmandel commented

Internal/External
External

Area
Other

Summary
I ran a novel AI-based code-analysis tool on the entire cardano node directory (I'm the tool maker, its not released yet) and found 14 potential issues.

Steps to reproduce
I didn't want to create an issue for each. The issue list is in this Google Sheet.
I also uploaded it as issues cardano-node.csv but the CSV could be outdated as the Google Sheet is editable.

Scan Date: 7/27/2024
Scanned code: https://github.com/IntersectMBO/cardano-node/tree/1e6f75101b5a637169efd8cb29026ad23ba4a427/cardano-node/src/Cardano/Node

If this feedback is useful, I can gladly run the tool on other directories, even on the entire repository.

kevinhammond commented

Thank you!

erikd commented

One comment from our internal Slack channel that may be of interest to you:

I can’t comment on other items, as I have not looked at them, but item 4 seems bogus, unfortunately: The AI didn’t recognize bracket.

zmandel commented
  • 👍1
erikd commented

The bracket family of functions in Haskell are of the form:

bracket setup shutdown action

Where the bracket function runs setup , then action and then shutdown (regardless of whether action threw an exception). Ie, shutdown is always run.

  • 👍1
zmandel commented

@erikd @kevinhammond ahh, It seems you have confused the meaning of the last column "More Details" on that issue in line number 4. That code that uses "bracket" is actually the suggestion that my AI came up with. The actual Cardano code does not use a bracket and leaks the handle. I also updated the file and line number where it occurs (it was on shutdown.hs)

coot commented

For the network related things:

  • Ad 2: setBlockForging :: [BlockForging m blk] -> m () - it doesn't return any meaningful result. The function does not raises any meaningful exception either, see - it could raise an IOError (e.g. out of memory, etc), but such exception would also be thrown to other threads.

  • Ad 5: This is correct: if the node cannot get information about IPv{4,6} addresses it should fail, hence no special error handling is required.

  • Ad 10: this is about this code snippet. The code is correct, the function returns error if both configurations couldn't be parsed - although it only reports the new formatting error.

  • Ad 14: the AI is right that this might lead to different behaviour if the exception is modified or hierarchy is changed. However this is in NonP2P code path, which will be retired. Only for that reason I'd mark it as no-op.

  • Ad 15: I don't see any type casting in the Cardano.Node.Tracing.Tracers.P2P module.