teragrep / lsh_01

logstash-http-input to syslog bridge
https://teragrep.com
Apache License 2.0
0 stars 3 forks source link

Fix #69: Coverity defects #70

Closed 51-code closed 3 months ago

51-code commented 5 months ago

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 commented 3 months ago

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.