Skip to content

Fix a unbound allocation - #47

Open
weiznich wants to merge 2 commits into
mainfrom
fix/unbound_allocation
Open

Fix a unbound allocation#47
weiznich wants to merge 2 commits into
mainfrom
fix/unbound_allocation

Conversation

@weiznich

Copy link
Copy Markdown
Member

This just caps the size of the preallocated vector to some reasonable limit. This prevents a potential out of memory error for malformed or otherwise malicious packages.

This issue was originally discovered by the RustFoundation based
scanning infrastructure created by their AI Security Engineer.

This PR also contains some minor CI improvements.

* Pin actions
* Restrict permissions
* Minimize dependencies
@weiznich
weiznich force-pushed the fix/unbound_allocation branch from 2a11d23 to dc67f65 Compare August 28, 2026 10:18
@weiznich
weiznich requested a review from a team August 28, 2026 10:23
Comment thread .github/workflows/ci.yml
- uses: taiki-e/install-action@cargo-hack
- name: Set environment variables
shell: bash
if: matrix.rust != 'nightly'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure that a "rust" key is set in the matrix, it might just work because it is "null". Maybe you wanted version here?

Suggested change
if: matrix.rust != 'nightly'
if: matrix.version != 'nightly'

Comment thread .github/workflows/ci.yml
RUST: ${{ matrix.version }}
run: |
rustup override set $RUST
rustup component add rustfmt

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I am not mistaken, nothing is running cargo fmt --check at this time. I suppose it still needs to be added?

Comment thread src/lib.rs
fn from_sql(
bytes: <Pg as diesel::backend::Backend>::RawValue<'_>,
) -> diesel::deserialize::Result<Self> {
let mut cursor = Cursor::new(bytes.as_bytes());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let bytes = bytes.as_bytes();
let mut cursor = Cursor::new(bytes);

Comment thread src/lib.rs
let num_lexemes = cursor.read_u32::<NetworkEndian>()?;

let mut entries = Vec::with_capacity(num_lexemes as usize);
let mut entries = Vec::with_capacity(std::cmp::min(num_lexemes as usize, 1000));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since each lexeme costs at least three bytes on the wire (null terminator plus its u16 position count) and the slice length is known (see review note above) before the cursor exists, which is exact, this needs no constant, and does not penalise a legitimately large vector.

Suggested change
let mut entries = Vec::with_capacity(std::cmp::min(num_lexemes as usize, 1000));
let mut entries = Vec::with_capacity((num_lexemes as usize).min(bytes.len() / 3));

Comment thread src/lib.rs
pub struct RegConfig;

impl FromSql<TsVector, Pg> for PgTsVector {
fn from_sql(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would suggest to move the body into fn decode_tsvector(bytes: &[u8]) -> deserialize::Result<PgTsVector> so this is testable, as PgValue::new is public-gated behind i-implement-a-third-party-backend-and-opt-into-breaking-changes, so no test can reach from_sql otherwise, and the two existing tests need DATABASE_URL.

Then something like assert!(decode_tsvector(&[0xFF; 4]).is_err()) covers this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants