Skip to content

Conversation

muzarski
Copy link
Contributor

Fixes: #253

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have implemented Rust unit tests for the features/changes introduced.
  • [ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • [ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

@muzarski muzarski added this to the 0.5 milestone May 14, 2025
@muzarski muzarski self-assigned this May 14, 2025
@muzarski muzarski added the P1 P1 priority item - very important label May 14, 2025
@muzarski muzarski requested review from Lorak-mmk and wprzytula May 14, 2025 11:27
Copy link
Contributor

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

Phew! This was one of the most boring PRs to review ;P

Comment on lines 1067 to 1079
cluster_raw: CassBorrowedExclusivePtr<CassCluster, CMut>,
enable: cass_bool_t,
) -> CassError {
let cluster = BoxFFI::as_mut_ref(cluster_raw).unwrap();
let Some(cluster) = BoxFFI::as_mut_ref(cluster_raw) else {
tracing::error!(
"Provided null cluster pointer to cass_cluster_set_use_beta_protocol_version!"
);
return CassError::CASS_ERROR_LIB_BAD_PARAMS;
};

cluster.use_beta_protocol_version = enable == cass_true;

CassError::CASS_OK
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Does cass_cluster_set_use_beta_protocol_version() do anything? AFAIK Rust driver supports just one protocol version, so this is a no-op. Shouldn't we rather mark this as deprecated and/or remove its implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is read here:

#[unsafe(no_mangle)]
pub unsafe extern "C" fn cass_cluster_set_protocol_version(
    cluster_raw: CassBorrowedExclusivePtr<CassCluster, CMut>,
    protocol_version: c_int,
) -> CassError {
    let Some(cluster) = BoxFFI::as_mut_ref(cluster_raw) else {
        tracing::error!("Provided null cluster pointer to cass_cluster_set_protocol_version!");
        return CassError::CASS_ERROR_LIB_BAD_PARAMS;
    };

    if protocol_version == 4 && !cluster.use_beta_protocol_version {
        // Rust Driver supports only protocol version 4
        CassError::CASS_OK
    } else {
        CassError::CASS_ERROR_LIB_BAD_PARAMS
    }
}

@muzarski muzarski merged commit b1ff429 into scylladb:master May 15, 2025
12 checks passed
@muzarski muzarski deleted the verify-null-ptr branch May 15, 2025 11:51
@wprzytula wprzytula mentioned this pull request Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 P1 priority item - very important

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't panic when user provides null pointers to methods

3 participants