Fix undefined behavor in GenericStringArray::from_iter_values #1145
Conversation
alamb
force-pushed
the
alamb/fix_undefined
branch
from
38a0af9
to
db22383
Compare
alamb
changed the title
|
I need to port my tests for this but I think the code is fine. I ran out of time today to keep working on this |
Codecov Report
@@ Coverage Diff @@
## master #1145 +/- ##
=======================================
Coverage 82.55% 82.56%
=======================================
Files 169 169
Lines 50456 50497 +41
=======================================
+ Hits 41655 41693 +38
- Misses 8801 8804 +3
Continue to review full report at Codecov.
|
arrow/src/array/array_string.rs
Outdated
|
|
||
| // iterator size hint may not be correct so compute the actual number of offsets | ||
| let actual_len = match offsets.len() { | ||
| 0 => 0, |
There was a problem hiding this comment.
We always push at least one offset (zero), so this match should not be needed. Looks good otherwise.
There was a problem hiding this comment.
Thanks @jhorstmann -- I'll change this to assert len > 0 and try and polish up this PR with tests tomorrow
| @@ -187,8 +187,13 @@ impl<OffsetSize: StringOffsetSizeTrait> GenericStringArray<OffsetSize> { | |||
| offsets.push(length_so_far); | |||
| values.extend_from_slice(s.as_bytes()); | |||
| } | |||
|
|
|||
| // iterator size hint may not be correct so compute the actual number of offsets | |||
| assert!(!offsets.is_empty()); // wrote at least one | |||
There was a problem hiding this comment.
here is the fix -- the rest of the PR is tests
Which issue does this PR close?
Closes #1144
Rationale for this change
If an iterator produces more values than its declared upper bound, it can result in undefined behavior
What changes are included in this PR?
Fix the bug by using the correct length