[
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 6:03 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 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?
For the record, there is absolutely no usage of InvalidLegacyProtocolMagic in
the whole codebase which makes me confident we can get rid of that safely.
EDIT: It would be cool if we also put there the IP address of the caller here.
Now if we just log, the log message is pretty non-telling. Who has caused that?
We have context here
{code}
void initiate(ChannelHandlerContext ctx, ByteBuf in) throws IOException
{
initiate = HandshakeProtocol.Initiate.maybeDecode(in);
if (initiate == null)
{code}
So we can pass address of the source from ctx into maybeDecode and log it with
it.
[~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 about to return
"null" 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?
For the record, there is absolutely no usage of InvalidLegacyProtocolMagic in
the whole codebase which makes me confident we can get rid of that safely.
[~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
