Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
zmandel opened this issue Jul 27, 2024 · 7 comments
Open
Labels
needs triage Issue / PR needs to be triaged. Stale

Comments

@zmandel
Copy link

zmandel commented Jul 27, 2024

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.

@zmandel zmandel added the needs triage Issue / PR needs to be triaged. label Jul 27, 2024
@kevinhammond
Copy link
Contributor

Thank you!

@erikd
Copy link
Contributor

erikd commented Aug 1, 2024

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
Copy link
Author

zmandel commented Aug 1, 2024 via email

@erikd
Copy link
Contributor

erikd commented Aug 1, 2024

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.

@zmandel
Copy link
Author

zmandel commented Aug 5, 2024

@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
Copy link
Contributor

coot commented Sep 3, 2024

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.

Copy link

github-actions bot commented Oct 4, 2024

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 120 days.

@github-actions github-actions bot added the Stale label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Issue / PR needs to be triaged. Stale
Projects
Status: No status
Development

No branches or pull requests

4 participants