Repository: jorgecarleitao/arrow2
Pull request #1078 (2022-06-16 07:04:38)

Fixed panic in bitmap assign_mut
~The `length` taken into account to compute the remainder bytes could differ from the length of the buffer (e.g. in slicing operations).~

~This ensures we do the computations on the length of the buffer and we've added a test to ensure we don't regress.~

When calling `into_mut` on a `Bitmap` and the data has an `offset` we `memcpy` the bytes into a `MutableBitmap`. However this is incorrect, because a `MutableBitmap` does not have an offset.

This fixes that by iterating every `bit` and recreating the proper bytes for the `MutableBitmap`.

This is expensive, so maybe we should consider adding an `offset` on `MutableBitmap` as well.

codecov[bot]: # [Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) Report
> Merging [#1078](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (0640015) into [main](https://codecov.io/gh/jorgecarleitao/arrow2/commit/39db6fb7514364bfea08d594793b23e1ed5a7def?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) (39db6fb) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main    #1078      +/-   ##
==========================================
+ Coverage   81.01%   81.02%   +0.01%     
==========================================
  Files         366      366              
  Lines       35056    35060       +4     
==========================================
+ Hits        28399    28407       +8     
+ Misses       6657     6653       -4     
```


| [Impacted Files](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao) | Coverage Δ | |
|---|---|---|
| [src/bitmap/mutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC9tdXRhYmxlLnJz) | `97.51% <100.00%> (ø)` | |
| [src/bitmap/utils/chunks\_exact\_mut.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC91dGlscy9jaHVua3NfZXhhY3RfbXV0LnJz) | `75.86% <100.00%> (+0.86%)` | :arrow_up: |
| [src/array/primitive/mod.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2FycmF5L3ByaW1pdGl2ZS9tb2QucnM=) | `84.46% <0.00%> (-1.25%)` | :arrow_down: |
| [src/bitmap/utils/slice\_iterator.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC91dGlscy9zbGljZV9pdGVyYXRvci5ycw==) | `98.78% <0.00%> (+1.21%)` | :arrow_up: |
| [src/bitmap/immutable.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC9pbW11dGFibGUucnM=) | `83.11% <0.00%> (+1.29%)` | :arrow_up: |
| [src/bitmap/assign\_ops.rs](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao#diff-c3JjL2JpdG1hcC9hc3NpZ25fb3BzLnJz) | `87.09% <0.00%> (+3.22%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Last update [39db6fb...0640015](https://codecov.io/gh/jorgecarleitao/arrow2/pull/1078?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jorge+Leitao).

jorgecarleitao: Just to double check, before the test would panic (not trigger UB), right?
ritchie46: > Just to double check, before the test would panic (not trigger UB), right?

Yes

Patch:
diff --git a/src/bitmap/immutable.rs b/src/bitmap/immutable.rs
index 45d8e89e4e..b63310b07c 100644
--- a/src/bitmap/immutable.rs
+++ b/src/bitmap/immutable.rs
@@ -239,7 +239,12 @@ impl Bitmap {
     pub fn make_mut(self) -> MutableBitmap {
         match self.into_mut() {
             Either::Left(data) => {
-                MutableBitmap::from_vec(data.bytes.as_ref().to_vec(), data.length)
+                if data.offset > 0 {
+                    // we have to recreate the bytes because a MutableBitmap does not have an `offset`.
+                    data.iter().collect()
+                } else {
+                    MutableBitmap::from_vec(data.bytes.as_ref().to_vec(), data.length)
+                }
             }
             Either::Right(data) => data,
         }


Test patch:
diff --git a/tests/it/bitmap/assign_ops.rs b/tests/it/bitmap/assign_ops.rs
index bea2b0dcbf..ef01fdd208 100644
--- a/tests/it/bitmap/assign_ops.rs
+++ b/tests/it/bitmap/assign_ops.rs
@@ -18,6 +18,24 @@ fn basics() {
     );
 }
 
+#[test]
+fn binary_assign_oob() {
+    // this check we don't have an oob access if the bitmaps are size T + 1
+    // and we do some slicing.
+    let a = MutableBitmap::from_iter(std::iter::repeat(true).take(65));
+    let b = MutableBitmap::from_iter(std::iter::repeat(true).take(65));
+
+    let a: Bitmap = a.into();
+    let a = a.slice(10, 20);
+
+    let b: Bitmap = b.into();
+    let b = b.slice(10, 20);
+
+    let mut a = a.make_mut();
+
+    binary_assign(&mut a, &b, |x: u64, y| x & y);
+}
+
 #[test]
 fn fast_paths() {
     let b = MutableBitmap::from([true, false]);