Skip to content

ARROW-10448: [Rust] Remove PrimitiveArray::new that can cause UB#8560

Closed
jorgecarleitao wants to merge 1 commit intoapache:masterfrom
jorgecarleitao:remove_new
Closed

ARROW-10448: [Rust] Remove PrimitiveArray::new that can cause UB#8560
jorgecarleitao wants to merge 1 commit intoapache:masterfrom
jorgecarleitao:remove_new

Conversation

@jorgecarleitao
Copy link
Member

@jorgecarleitao jorgecarleitao commented Oct 31, 2020

This PR removes PrimitiveArray::new. PrimitiveArray::new is pub, but it is dangerous because:

  • when the buffer's content is not aligned with T, many of its methods cause UB
  • when used with null_count > 0, many calls panic as the null bitmap is None, but the null_count != 0
  • when used with null_count > 0, it creates an array out of spec (as the buffer for the null bitmap is None but the null count is not zero)

Since:

  • a change in this method's signature (to either add the bitmap or remove null_count) requires a backward incompatible change
  • it is only used in tests
  • we have good offers to create primitive arrays:
    • from an ArrayData,
    • from a vector or vector of optionals
    • from an iterator

This PR removes it.

@jorgecarleitao jorgecarleitao changed the title ARROW-10448: [Rust] Remove PrimitiveArray::new. ARROW-10448: [Rust] Remove PrimitiveArray::new that is out of spec Oct 31, 2020
@jorgecarleitao jorgecarleitao changed the title ARROW-10448: [Rust] Remove PrimitiveArray::new that is out of spec ARROW-10448: [Rust] Remove PrimitiveArray::new that causes UB Oct 31, 2020
@github-actions
Copy link

github-actions bot commented Oct 31, 2020

https://issues.apache.org/jira/browse/ARROW-10448

@jorgecarleitao jorgecarleitao changed the title ARROW-10448: [Rust] Remove PrimitiveArray::new that causes UB ARROW-10448: [Rust] Remove PrimitiveArray::new that can cause UB Nov 1, 2020
@nevi-me nevi-me self-requested a review November 3, 2020 22:24
@nevi-me
Copy link
Contributor

nevi-me commented Nov 3, 2020

I think we have a similar problem with building arrays from ArrayDataBuilder because when building ArrayData, we don't check if the length of the array corresponds with the buffer's length (or if it's even specified).

So something like the below gets created, but ends up as a 0-len array

// try build array data without specifying length
ArrayData::builder(DataType::_).buffers(vec![buffer1, buffer2]).build()

@jorgecarleitao
Copy link
Member Author

jorgecarleitao commented Nov 4, 2020

I think we have a similar problem with building arrays from ArrayDataBuilder because when building ArrayData, we don't check if the length of the array corresponds with the buffer's length (or if it's even specified).

So something like the below gets created, but ends up as a 0-len array

// try build array data without specifying length
ArrayData::builder(DataType::_).buffers(vec![buffer1, buffer2]).build()

That is a very good point.

I think that we have a trade-off here: do we check that the ArrayData is according to spec (and incur the testing cost), or do we not check and live with potential UB? We can have APIs for a safe and unsafe, but we still need to decide what is public and what is not :P

This could be a good topic for the mailing list, as it is a general question, specially for IPC and the c data interface.

@alamb
Copy link
Contributor

alamb commented Nov 6, 2020

Is it the case that "UB" means "Undefined Behavior"?

alamb
alamb approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like a good improvement to me. 👍 It also makes it clearer to people how to construct arrays (aka use the Builders) rather than also potentially using PrimitiveArray::new directly

nevi-me
nevi-me approved these changes Nov 7, 2020
View reviewed changes
Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

LGTM

@nevi-me nevi-me closed this in 4dfbc8b Nov 7, 2020
@jorgecarleitao jorgecarleitao deleted the remove_new branch November 7, 2020 13:14
@asfimport asfimport mentioned this pull request Nov 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@nevi-me nevi-me nevi-me approved these changes

Assignees

No one assigned

Labels

Component: Rust

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jorgecarleitao @nevi-me @alamb