alexpikalov / cassandra-proto

Cassandra binary protocol implementation

Hi there 👋

• 🔭 I’m currently working on:
  • Rasmus - standalone WebAssembly engine.
  • TeleMQ - MQTT broker

cassandra-proto's Issues

Reading on uninitialized buffer may cause UB

Hello 🦀 ,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

cassandra-proto/src/frame/parser_async.rs

Lines 46 to 51 in 9bdbc91

	let mut body_bytes = Vec::with_capacity(length);
	unsafe {
	body_bytes.set_len(length);
	}
	proceed_if_filled!(cursor.read(&mut body_bytes), length);

cassandra-proto/src/frame/parser_async.rs

Lines 65 to 69 in 9bdbc91

	let mut tracing_bytes = Vec::with_capacity(UUID_LEN);
	unsafe {
	tracing_bytes.set_len(UUID_LEN);
	}
	body_cursor.read_exact(&mut tracing_bytes)?;

We found two cases where an uninitialized buffer is created (body_bytes & tracing_bytes) and passed to user-provided Read implementation. This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).

This part from the Read trait documentation explains the issue:

It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

Suggested Fix

It is safe to zero-initialize the newly allocated u8 buffer before read(), in order to prevent user-provided Read from accessing old contents of the newly allocated heap memory.

Thank you for checking out this issue 👍

align parser and parser_async method signatures

parser and parser_async have different method signatures

frame_parse signature is:

pub fn parse_frame<E>(
    cursor_cell: &RefCell<dyn Read>,
    compressor: &dyn Compressor<CompressorError = E>,
) -> error::Result<Frame>
where
    E: std::error::Error,

frame_parse_async signature:

pub fn parse_frame_async<E, C>(
  cursor: &mut C,
  compressor: &dyn Compressor<CompressorError = E>,
) -> error::Result<Option<Frame>>
where
  E: std::error::Error,
  C: Read,

I think it makes sense to align these so that the async is a drop in replacement for the sync.

Does the async require the cursor or can the cursor be passed in a RefCell?

Document Needed!

I'm trying to use this crate with cdrs-async, but the document is so poor, is there any plan to write some document for basic usage.

add a license

Hi, thanks for your work on this project!

I assume this project has the same license as cdrs but it would be great if you could explicitly add the MIT and Apache license files to this project.

Option processing does not require the version to parse correctly.

all discussion is based on
https://github.com/apache/cassandra/blob/trunk/doc/native_protocol_v4.spec#L110

In some cases the initial OPTION message does not have an expected version. Specifically I have an example comprising 9 bytes:

0x42 0x00 0x00 0x00 0x05
0x00 0x00 0x00 0x00

0x42 = 66 = the code for the DSE v4 driver.

I think that the proper way to handle this is to skip the version verification until the STARTUP message is received. What I am unsure of is should the protocol 0x42 OPTIONS message be processed, silently ignored, or an ERROR returned.

Move cdrs-async Compression type into cassandra-proto

Although maybe not technically part of the protocol, I think it would make a lot of sense to move it here as people who want to use cassandra-proto for lower level cassandra access will still need access to this.

I'm happy to provide a PR if this is something you would like to do.

Maintainership

I am working on a project that makes extensive use of cassandra-proto.
We currently have a fork with various features and fixes that I am trying to upstream.
And then there is a lot of further improvements I want to make after that including:

• add comprehensive tests
• add missing features
• make the encoders/decoders allocate less
• improve the API.

However the current pace of reviews here is pretty slow.
This is of course fine, you are working on this on your own time and have no obligation to review my PRs.
But this is limiting my ability to get cassandra-proto where it needs to be both for use in the project im working on and for use in cdrs and other projects.

So that leaves a few options:

• I am added as a maintainer and can merge my own PRs - I have a co-worker who can review my PRs to ensure we get a second set of eyes on them.
• I fork cassanda-proto and release it under a new name cassandra-protocol or something. - in the future if you are happy with the forked project then you can start using it for cdrs
• I fork cassanda-proto and am given access to the cassandra-proto name on crates.io

Let me know what your plans are for cassandra-proto and how we should proceed.