[jira] [Commented] (CASSANDRA-20842) Fix version range check in MessagingService.getVersionOrdinal

["Dmitry Konstantinov (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-20842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18014289#comment-18014289 ] 

Dmitry Konstantinov commented on CASSANDRA-20842:
-------------------------------------------------

* I think 1-2 statements are correct, good catch by Claude :).
 * Regarding #3 - it is inherited from the old logic but it makes sense to fix it as well
 * Regarding #4 checkUnknownBigVersion - here I relied on the fact that the version is 8bit (but probably I should multiple MAX_VALUE by 2 due to sign/unsigned difference..):
{code:java}
// 8 bits version, so don't waste versions 
public enum Version
{code}
and org.apache.cassandra.net.HandshakeProtocol.Initiate#maybeDecode:
{code:java}
int minMessagingVersion = getBits(flags, 16, 8); int maxMessagingVersion = getBits(flags, 24, 8);
{code}
1000 suggested here by AI I do not think very good as well. 
Taking in account the discovered bug I think it would be better to adjust the test and use the first out of range value instead 

> Fix version range check in MessagingService.getVersionOrdinal
> -------------------------------------------------------------
>
>                 Key: CASSANDRA-20842
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-20842
>             Project: Apache Cassandra
>          Issue Type: Bug
>          Components: Messaging/Internode
>            Reporter: Dmitry Konstantinov
>            Assignee: Dmitry Konstantinov
>            Priority: Normal
>             Fix For: 5.x
>
>
> Thank you to [~maedhroz] and Claude :) to highlight a minor issue in the recent changes for CASSANDRA-20816:
> {code:java}
> ## Issues Found
> ### 1. Incorrect bounds check condition
> if (result < 0 || result > maxVersion)
> This condition is wrong. The check should be against the array length, not `maxVersion`. Currently:
> - `maxVersion` is the actual version value (e.g., 15)
> - The check should be against `Version.values().length - 1` (the maximum valid ordinal)
> ### 2. Misleading variable name
> `maxVersion` actually contains the version value, not the maximum ordinal index.
> ## Corrected Implementation
> static final int minVersion = Version.values()[0].value;
> static final int maxVersionOrdinal = Version.values().length - 1;
> /**
>  * This is an optimisation to speed up the translation of the serialization
>  * version to the {@link Version} enum ordinal.
>  * We expect values for new versions to be incremented sequentially
>  *
>  * @param version the serialization version
>  * @return a {@link Version} ordinal value
>  */
> public static int getVersionOrdinal(int version)
> {
>     int result = version - minVersion;
>     if (result < 0 || result > maxVersionOrdinal)
>         throw new IllegalStateException("Unknown serialization version: " + version);
>     return result;
> }
> ## Additional Issues
> ### 3. Typo in exception message
> "Unkown" should be "Unknown"
> ### 4. Test case issue
> The test `checkUnknownBigVersion()` uses `Byte.MAX_VALUE + 1` (128), but this assumes version values are byte-sized. A more robust test would use a value that's guaranteed to be out of range regardless of the actual version values.
> ## Recommended Test Fix
> @Test(expected = IllegalStateException.class)
> public void checkUnknownBigVersion()
> {
>     // Use a value that's definitely larger than any reasonable version
>     MessagingService.getVersionOrdinal(1000);
> }
> {code}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org