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

fix: array_slice panics #10547

Merged
merged 6 commits into from
May 17, 2024
Merged

fix: array_slice panics #10547

merged 6 commits into from
May 17, 2024

Conversation

jonahgao
Copy link
Member

Which issue does this PR close?

Closes #10425.

Rationale for this change

Note: Negative from or to implies counting from the end of the list and both are standardized to the corresponding positive index.

When from is greater than to and stride is a positive, the following loop becomes infinite, until the index becomes excessively large and panics.

let mut index = start + from;
let mut cnt = 0;
while index >= start + to {
mutable.extend(
0,
index.to_usize().unwrap(),
index.to_usize().unwrap() + 1,
);
index += stride;
cnt += 1;
}

The fix is to return an empty list directly in this situation.

What changes are included in this PR?

Fix panic.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@jonahgao jonahgao marked this pull request as draft May 16, 2024 14:30
let stride = stride.unwrap_or(1);
if stride.is_zero() {
return exec_err!(
"array_slice got invalid stride: {:?}, it cannot be 0",
stride
);
} else if from <= to && stride.is_negative() {
} else if (from <= to && stride.is_negative())
Copy link
Member Author

Choose a reason for hiding this comment

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

When from is equal to to, it should not return an empty list.
The fix for it is not included in this PR, it has been added to issue #10548.

@jonahgao jonahgao marked this pull request as ready for review May 16, 2024 14:48
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @jonahgao

@jonahgao
Copy link
Member Author

Thank you for reviewing @alamb

@jonahgao jonahgao merged commit 06c8f5a into apache:main May 17, 2024
23 checks passed
@jonahgao jonahgao deleted the fix_panic branch May 17, 2024 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

array_slice panics with stride=1
2 participants