Skip to content

Don't panic when null pointer is provided - log error and return instead #300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
May 15, 2025

Conversation

muzarski
Copy link
Collaborator

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 wprzytula and Lorak-mmk May 14, 2025 11:27
Copy link
Collaborator

@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
Collaborator

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
Collaborator 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
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