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

Fix #69: Coverity defects #70

Merged
merged 5 commits into from
Aug 19, 2024
Merged

Fix #69: Coverity defects #70

merged 5 commits into from
Aug 19, 2024

Conversation

51-code
Copy link

@51-code 51-code commented Jun 3, 2024

Fixed Coverity defects 460538 and 452917.

The first (460538) was a simple useless variable assignment in PayloadConfig.

The second (452917) was a possible NPE in HttpServerHandler. ChannelHandlerContext.remoteAddress() might return null if the channel is not connected. I noticed that there is a check for this in the class isActive() so I used that and logged the error and returned. This might not be the best way to deal with the situation, maybe an error should be thrown instead? Furthermore, Coverity might not see that the NPE is dealt with this fix so it needs to be solved manually from Coverity after merging.

In #69, fixing was specifically asked for 461131. This was a possible NullPointerException in RelpConversion's sendMessage(), but the function call is already surrounded by a try catch as per #7, so in my opinion a null check for this is a bit overkill and clutters the code more. There is also no threat of a null happening there yet with the current Supplier (RelpConnectionFactory), but this might of course change in the future.

Went over the rest of the defects in Coverity as well, and I don't think they are necessary to "fix", I think they are false positives mostly.

@51-code 51-code added the bug Something isn't working label Jun 3, 2024
@51-code 51-code requested a review from kortemik June 3, 2024 13:11
@51-code 51-code self-assigned this Jun 3, 2024
@51-code 51-code linked an issue Jun 4, 2024 that may be closed by this pull request
@MoonBow-1 MoonBow-1 self-requested a review August 12, 2024 12:14
Copy link

@MoonBow-1 MoonBow-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting one minor grammar issue and asking a question regarding a possible larger refactoring in the future

@51-code 51-code requested a review from MoonBow-1 August 14, 2024 05:51
@51-code 51-code requested a review from kortemik August 16, 2024 07:00
@51-code
Copy link
Author

51-code commented Aug 19, 2024

I traced the remoteAddress parameter all the way to the RelpConversion object which forwards the message. The parameter wasn't actually used at all. The address is only gotten from the RelpConfig object at the start of execution and placed in RelpConnectionFactory, which ultimately gives it to the RelpConversion instead of the message itself. That means that taking the remoteAddress from the message or the channel wasn't necessary at all, so I removed the whole thing.

@51-code 51-code requested a review from kortemik August 19, 2024 06:54
@kortemik kortemik merged commit 605ef1f into teragrep:main Aug 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix defects reported by coverity
3 participants