Skip to content

Codacy/Flawfinder: Document a false positive finding#1084

Merged
hoffie merged 1 commit intojamulussoftware:masterfrom
hoffie:issue1081-ignore-flawfinder-signalhandler
Feb 23, 2021
Merged

Codacy/Flawfinder: Document a false positive finding#1084
hoffie merged 1 commit intojamulussoftware:masterfrom
hoffie:issue1081-ignore-flawfinder-signalhandler

Conversation

@hoffie
Copy link
Copy Markdown
Member

@hoffie hoffie commented Feb 21, 2021

Add an explaining comment and a special hint at Codacy/Flawfinder to ignore the (non-)problematic line.

Special comment effectiveness has been validated here: hoffie#3

Related: #1081

@hoffie hoffie requested a review from softins February 21, 2021 23:23
@hoffie hoffie mentioned this pull request Feb 21, 2021
Closed
@atsampson
Copy link
Copy Markdown
Contributor

atsampson commented Feb 22, 2021

In comments like this, it's a good idea to say which warning it's safe to ignore, just in case the tools detect a different problem here in the future.

@hoffie hoffie force-pushed the issue1081-ignore-flawfinder-signalhandler branch from 91027fc to 3f5b198 Compare February 22, 2021 07:32
@hoffie
Copy link
Copy Markdown
Member Author

hoffie commented Feb 22, 2021 •
edited
Loading

In comments like this, it's a good idea to say which warning it's safe to ignore, just in case the tools detect a different problem here in the future.

I though my comment was already rather elaborate and git blame could be used to end up in this PR/the related issue, but I guess you are right that it will be better to make this explicit. I have pushed an updated comment. Thanks for this suggestion! :)

@hoffie hoffie force-pushed the issue1081-ignore-flawfinder-signalhandler branch from 3f5b198 to f429148 Compare February 22, 2021 07:35
@hoffie
Codacy/Flawfinder: Document a false positive finding
e0e6401 
The read() from the signal handler socket triggers a Codacy/flawfinder finding:
"Check buffer boundaries if used in a loop including recursive loops (CWE-120, CWE-20)"
Investigation has shown that this is a false positive as the proper buffer
size is enforced and because the input is within the same security boundary.

This adds an explaining comment and a special hint at Codacy/Flawfinder to ignore
the (non-)problematic line.

Related: jamulussoftware#1081

Signed-off-by: Christian Hoffmann <mail@hoffmann-christian.info>
@hoffie hoffie force-pushed the issue1081-ignore-flawfinder-signalhandler branch from f429148 to e0e6401 Compare February 22, 2021 07:36
@softins softins linked an issue Feb 22, 2021 that may be closed by this pull request
Security: Investigate buffer boundary warning in src/signalhandler.cpp #1081
Closed
@pljones
Copy link
Copy Markdown
Collaborator

pljones commented Feb 22, 2021 •
edited
Loading

I don't like linters/analysers that don't let you mark clearly to the linter that you've accepted the warning and don't want to hear about it again (in addition to human-readable comments). (They should be explicit markers, as close to the code as possible: ..., /* Codacity(#1432, accept) */&sigNum, ... kind of thing.) Few and far between, unfortunately.

@hoffie
Copy link
Copy Markdown
Member Author

hoffie commented Feb 22, 2021

I don't like linters/analysers that don't let you mark clearly to the linter that you've accepted the warning and don't want to hear about it again (in addition to human-readable comments). (They should be explicit markers, as close to the code as possible: ..., /* Codacity(#1432, accept) */&sigNum, ... kind of thing.) Few and far between, unfortunately.

I fully agree. I don't think it's possible with Flawfinder though. I even checked its code.

@softins softins requested a review from pljones February 23, 2021 11:10
@hoffie hoffie merged commit a48affb into jamulussoftware:master Feb 23, 2021
@hoffie hoffie deleted the issue1081-ignore-flawfinder-signalhandler branch March 19, 2022 21:23
@pljones pljones added this to the Release 3.7.0 milestone Nov 24, 2024
@pljones pljones moved this from Triage to Done in Tracking Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pljones pljones pljones approved these changes

@softins softins softins approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Tracking
Status: Done

Milestone

Release 3.7.0

Development

Successfully merging this pull request may close these issues.

Security: Investigate buffer boundary warning in src/signalhandler.cpp

4 participants

@hoffie @atsampson @pljones @softins