Skip to content
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

[C++] light_array buffer boundary assertion? #36949

Closed
Light-City opened this issue Jul 31, 2023 · 0 comments · Fixed by #36966
Closed

[C++] light_array buffer boundary assertion? #36949

Light-City opened this issue Jul 31, 2023 · 0 comments · Fixed by #36966
Assignees
Milestone

Comments

@Light-City
Copy link
Contributor

Light-City commented Jul 31, 2023

light_array KeyColumnArray assert

  uint8_t* mutable_data(int i) {
    ARROW_DCHECK(i >= 0 && i <= kMaxBuffers);
    return mutable_buffers_[i];
  }
  /// \brief Return one of the underlying read-only buffers
  const uint8_t* data(int i) const {
    ARROW_DCHECK(i >= 0 && i <= kMaxBuffers);
    return buffers_[i];
  }

ARROW_DCHECK(i >= 0 && i <= kMaxBuffers); may be wrong?

static constexpr int kMaxBuffers = 3;
  const uint8_t* buffers_[kMaxBuffers];
  uint8_t* mutable_buffers_[kMaxBuffers];

when data(i) i equl kMaxBuffers, will the buffers_boundary be exceeded?

So,this is a bug?

index should [0,1, 2] but not include 3

Component(s)

C++

pitrou pushed a commit that referenced this issue Aug 10, 2023
…36966)

### Rationale for this change

file: light_array.h

The length of the buffers array is 3 (kMaxBuffers).

- buffers_[kValidityBuffer]
- buffers_[kFixedLengthBuffer]
- buffers_[kVariableLengthBuffer]

So when we do the check, the asserted index should be less than 3, not equal to.

In addition, fix the comment line break format problem.

### What changes are included in this PR?

- KeyColumnArray

data and mutable_data ARROW_DCHECK.

- ResizableArrayData class 

Comment line break formatting for mutable_data functions

### Are these changes tested?

no, just change assert bound.

### Are there any user-facing changes?

no
* Closes: #36949

Authored-by: light-city <455954986@qq.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@pitrou pitrou added this to the 14.0.0 milestone Aug 10, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…tion. (apache#36966)

### Rationale for this change

file: light_array.h

The length of the buffers array is 3 (kMaxBuffers).

- buffers_[kValidityBuffer]
- buffers_[kFixedLengthBuffer]
- buffers_[kVariableLengthBuffer]

So when we do the check, the asserted index should be less than 3, not equal to.

In addition, fix the comment line break format problem.

### What changes are included in this PR?

- KeyColumnArray

data and mutable_data ARROW_DCHECK.

- ResizableArrayData class 

Comment line break formatting for mutable_data functions

### Are these changes tested?

no, just change assert bound.

### Are there any user-facing changes?

no
* Closes: apache#36949

Authored-by: light-city <455954986@qq.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants