[
https://issues.apache.org/jira/browse/CASSANDRA-19483?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17884394#comment-17884394
]
Stefan Miklosovic edited comment on CASSANDRA-19483 at 9/24/24 5:53 PM:
------------------------------------------------------------------------
https://github.com/apache/cassandra/pull/3249/files#r1773768271
Somebody might it find handy that it throws but when we are about to return
"null" there so there would be no way to tell the difference because the logic
in try block in maybeDecode might throw as well and there we return null in
catch block. InvalidLegacyProtocolMagic is not an instance of EOFException so
it will throw from that maybeDecode method but the logic in try might throw and
return null. So before we either threw or we returned null, now we always
return null. If we do not throw InvalidLegacyProtocolMagic then we can not
react on that in InboundMessageHandler.
So the ultimate question is what is the difference between returning null and
throwing InvalidLegacyProtocolMagic. There is this in InboudConnectionInitiator:
{code}
initiate = HandshakeProtocol.Initiate.maybeDecode(in);
if (initiate == null)
return;
{code}
which tells me that it kind of already counts with a situation when it returned
null (e.g. when it throws EOFException and returns null) so returning null from
maybeDecode seems pretty innocent. I think that as long as there is no spam
logger it is just fine.
If the goal is to just not constantly throw and log, we can just use no spam
logger in src/java/org/apache/cassandra/net/InboundMessageHandler.java and save
ourselves from all the related changes. The result would be functionally-wise
same. Hence, do we go to remove the exception and change the code or do we just
log with no spam logger?
[~brandon.williams] thoughts?
was (Author: smiklosovic):
https://github.com/apache/cassandra/pull/3249/files#r1773768271
Somebody might it find handy that it throws but when we are abotu to return
"null" there so there would be no way to tell the difference because the logic
in try block in maybeDecode might throw as well and there we return null in
catch block. InvalidLegacyProtocolMagic is not an instance of EOFException so
it will throw from that maybeDecode method but the logic in try might throw and
return null. So before we either threw or we returned null, now we always
return null. If we do not throw InvalidLegacyProtocolMagic then we can not
react on that in InboundMessageHandler.
So the ultimate question is what is the difference between returning null and
throwing InvalidLegacyProtocolMagic. There is this in InboudConnectionInitiator:
{code}
initiate = HandshakeProtocol.Initiate.maybeDecode(in);
if (initiate == null)
return;
{code}
which tells me that it kind of already counts with a situation when it returned
null (e.g. when it throws EOFException and returns null) so returning null from
maybeDecode seems pretty innocent. I think that as long as there is no spam
logger it is just fine.
If the goal is to just not constantly throw and log, we can just use no spam
logger in src/java/org/apache/cassandra/net/InboundMessageHandler.java and save
ourselves from all the related changes. The result would be functionally-wise
same. Hence, do we go to remove the exception and change the code or do we just
log with no spam logger?
[~brandon.williams] thoughts?
> Catch InvalidLegacyProtocolMagic exceptions
> -------------------------------------------
>
> Key: CASSANDRA-19483
> URL: https://issues.apache.org/jira/browse/CASSANDRA-19483
> Project: Cassandra
> Issue Type: Improvement
> Components: Observability/Logging
> Reporter: Brad Schoening
> Assignee: Ilya Zakharov
> Priority: Low
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Similar to CASSANDRA-18839, we frequently see the exception
> InvalidLegacyProtocolMagic with full stack traces flooding our logs. These
> are due to Qualys vulnerability scans.
> It seem to be a simple check in Message.java and would be better handled by:
> a) returning a boolean from validateLegacyProtocolMagic() ->
> hasValidLegacyProtocolMagic() instead of creating a custom exception class or
> b) adding a catch block in HandshakeProtocol.java and return null as is done
> for messagingVersion
> {code:java}
> static Initiate maybeDecode(ByteBuf buf) throws IOException
> {
> ...
> try (DataInputBuffer in = new DataInputBuffer(nio, false))
> {
> validateLegacyProtocolMagic(in.readInt()); // throws
> exception
> int flags = in.readInt();
> // legacy pre40 messagingVersion flag
> if (getBits(flags, 8, 8) < VERSION_40)
> return null;
> int minMessagingVersion = getBits(flags, 16, 8);
> int maxMessagingVersion = getBits(flags, 24, 8);
> // 5.0+ does not support pre40
> if (maxMessagingVersion < MessagingService.VERSION_40)
> return null;
> ....
> }
> catch (EOFException e)
> {
> return null;
> }
> }
> {code}
> {code:java}
> static void validateLegacyProtocolMagic(int magic) throws
> InvalidLegacyProtocolMagic
> {
> if (magic != PROTOCOL_MAGIC)
> throw new InvalidLegacyProtocolMagic(magic);
> }
> {code}
> {code:java}
> {{2024-03-20 03:47:27,380 [ERROR] [Messaging-EventLoop-3-2] cluster_id=9
> ip_address=10.0.0.1 InboundConnectionInitiator.java:360 - Failed to properly
> handshake with peer /10.0.2:33356. Closing the channel.}}
> {{io.netty.handler.codec.DecoderException:
> org.apache.cassandra.net.Message$InvalidLegacyProtocolMagic: Read 1431520594,
> Expected -900387334}}
> {{ at
> io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:478)}}
> {{ at
> io.netty.handler.codec.ByteToMessageDecoder.channelInputClosed(ByteToMessageDecoder.java:404)}}
> {{ at
> io.netty.handler.codec.ByteToMessageDecoder.channelInputClosed(ByteToMessageDecoder.java:371)}}
> {{ at
> io.netty.handler.codec.ByteToMessageDecoder.channelInactive(ByteToMessageDecoder.java:354)}}
> {{ at
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:262)}}
> {{ at
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:248)}}
> {{ at
> io.netty.channel.AbstractChannelHandlerContext.fireChannelInactive(AbstractChannelHandlerContext.java:241)}}
> {{ at
> io.netty.channel.DefaultChannelPipeline$HeadContext.channelInactive(DefaultChannelPipeline.java:1405)}}
> {{ at
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:262)}}
> {{ at
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:248)}}
> {{ at
> io.netty.channel.DefaultChannelPipeline.fireChannelInactive(DefaultChannelPipeline.java:901)}}
> {{ at
> io.netty.channel.AbstractChannel$AbstractUnsafe$8.run(AbstractChannel.java:819)}}
> {{ at
> io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:164)}}
> {{ at
> io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:472)}}
> {{ at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:384)}}
> {{ at
> io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)}}
> {{ at
> io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)}}
> {{ at
> io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)}}
> {{ at java.base/java.lang.Thread.run(Thread.java:834)}}
> {{Caused by: org.apache.cassandra.net.Message$InvalidLegacyProtocolMagic:
> Read 1431520594, Expected -900387334}}
> {{ at
> org.apache.cassandra.net.Message.validateLegacyProtocolMagic(Message.java:343)}}
> {{ at
> org.apache.cassandra.net.HandshakeProtocol$Initiate.maybeDecode(HandshakeProtocol.java:167)}}
> {{ at
> org.apache.cassandra.net.InboundConnectionInitiator$Handler.initiate(InboundConnectionInitiator.java:242)}}
> {{ at
> org.apache.cassandra.net.InboundConnectionInitiator$Handler.decode(InboundConnectionInitiator.java:235)}}
> {{ at
> io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:508)}}
> {{ at
> io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:447)}}
> {{ ... 18 common frames omitted}}
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
