Skip to content

StringArrayBuilder and LargeStringArrayBuilder don't check null buffer length #13759

@alamb

Description

@alamb

Describe the bug

(This was found by a security audit performed by InfluxData)

The public method finish in datafusion_functions::strings::StringArrayBuilder takes an argument, null_buffer, which is unconditionally assigned to an internal member; however, the contract for this data type requires that the three buffers (value_buffer, offsets_buffer, and nulls) are of the same length.

The argument to finish can be Option with a buffer of arbitrary length, but the length of it is not checked; therefore, the contract might be violated and subsequent usage of the resulting StringArray might lead to out-of-bounds reads or writes.

pub fn finish(self, null_buffer: Option<NullBuffer>) -> LargeStringArray {
let array_builder = ArrayDataBuilder::new(DataType::LargeUtf8)
.len(self.offsets_buffer.len() / size_of::<i64>() - 1)
.add_buffer(self.offsets_buffer.into())
.add_buffer(self.value_buffer.into())
.nulls(null_buffer);
// SAFETY: all data that was appended was valid Large UTF8 and the values
// and offsets were created correctly
let array_data = unsafe { array_builder.build_unchecked() };
LargeStringArray::from(array_data)
}

I (@alamb ) analyzed the code and I do think there is a problem, but I do not think it is exploitable from SQL or other downstream applications. It would only affect someone using the StringArrayBuilder or LargeStringArrayBuilder APIs directly (though they are pub, see doc links)

The reason I don't think it is exploitable is that it is only called in in two locations:

And the size of the null buffer is correct in both cases

To Reproduce

No response

Expected behavior

No response

Additional context

No response

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions