From 25f84ab41e2f12bf5c97316e7390f48764178820 Mon Sep 17 00:00:00 2001 From: clubby789 Date: Thu, 28 Jul 2022 13:52:39 +0100 Subject: [PATCH 1/5] Fix UB reported by Miri by splitting a slice --- src/matrix_graph.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/matrix_graph.rs b/src/matrix_graph.rs index 8b80f26c3..a3f039758 100644 --- a/src/matrix_graph.rs +++ b/src/matrix_graph.rs @@ -853,15 +853,24 @@ fn extend_flat_square_matrix( for c in (1..old_node_capacity).rev() { let pos = c * old_node_capacity; let new_pos = c * new_node_capacity; - - // Move the slices directly if they do not overlap with their new position if pos + old_node_capacity <= new_pos { - let old = node_adjacencies[pos..pos + old_node_capacity].as_mut_ptr(); - let new = node_adjacencies[new_pos..new_pos + old_node_capacity].as_mut_ptr(); - - // SAFE: new starts at least `old_node_capacity` positions after old. + // first_chunk = node_adjacencies[pos..] + let (_, first_chunk) = node_adjacencies.split_at_mut(pos); + // first_chunk = node_adjacencies[pos..pos + old_node_capacity] + // second_chunk = node_adjacencies[old_node_capacity..] + let (first_chunk, second_chunk) = first_chunk.split_at_mut(old_node_capacity); + // second_chunk = node_adjacencies[new_pos..] + let (_, second_chunk) = second_chunk.split_at_mut(new_pos - (pos + old_node_capacity)); + // second_chunk = node_adjacencies[new_pos..new_pos + old_node_capacity] + let (second_chunk, _) = second_chunk.split_at_mut(old_node_capacity); + // SAFETY: Slices cannot overlap as they were created by splitting a slice + // The length is equal and valid as they were both created with the same `split_at_mut` argument unsafe { - std::ptr::swap_nonoverlapping(old, new, old_node_capacity); + std::ptr::swap_nonoverlapping( + first_chunk.as_mut_ptr(), + second_chunk.as_mut_ptr(), + old_node_capacity, + ); } } else { for i in (0..old_node_capacity).rev() { From ad1bda70beaeda9f768e21e9d13e88ffd4dc5120 Mon Sep 17 00:00:00 2001 From: clubby789 Date: Thu, 28 Jul 2022 13:56:30 +0100 Subject: [PATCH 2/5] Add Miri to GitHub CI Disables some tests that take too long to run while under Miri, and adds `features = ["std"]` to the `indexmap` dependency so it compiles correctly under Miri. --- .github/workflows/ci.yml | 9 +++++++++ Cargo.toml | 2 +- tests/iso.rs | 6 ++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8fb6487a4..fae7a44e1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,3 +72,12 @@ jobs: - name: Rustfmt if: matrix.rustfmt run: cargo fmt -- --check + + miri: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: dtolnay/rust-toolchain@nightly + with: + components: miri + - run: cargo miri test diff --git a/Cargo.toml b/Cargo.toml index e45220218..24ee6c57c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,7 +36,7 @@ debug = true [dependencies] fixedbitset = { version = "0.4.0", default-features = false } -indexmap = { version = "1.7" } +indexmap = { version = "1.7", features = ["std"] } quickcheck = { optional = true, version = "0.8", default-features = false } serde = { version = "1.0", optional = true } serde_derive = { version = "1.0", optional = true } diff --git a/tests/iso.rs b/tests/iso.rs index f808830e4..06e426b99 100644 --- a/tests/iso.rs +++ b/tests/iso.rs @@ -294,6 +294,7 @@ fn full_iso() { } #[test] +#[cfg_attr(miri, ignore = "Takes too long to run in Miri")] fn praust_dir_no_iso() { let a = str_to_digraph(PRAUST_A); let b = str_to_digraph(PRAUST_B); @@ -302,6 +303,7 @@ fn praust_dir_no_iso() { } #[test] +#[cfg_attr(miri, ignore = "Takes too long to run in Miri")] fn praust_undir_no_iso() { let a = str_to_graph(PRAUST_A); let b = str_to_graph(PRAUST_B); @@ -464,6 +466,7 @@ fn iso_matching() { } #[test] +#[cfg_attr(miri, ignore = "Can't open files with isolation")] fn iso_100n_100e() { let g0 = graph_from_file("tests/res/graph_100n_100e.txt"); let g1 = graph_from_file("tests/res/graph_100n_100e_iso.txt"); @@ -471,6 +474,7 @@ fn iso_100n_100e() { } #[test] +#[cfg_attr(miri, ignore = "Can't open files with isolation")] fn iso_large() { let g0 = graph_from_file("tests/res/graph_1000n_1000e.txt"); let g1 = graph_from_file("tests/res/graph_1000n_1000e_iso.txt"); @@ -489,6 +493,7 @@ fn iso_multigraph_failure() { } #[test] +#[cfg_attr(miri, ignore = "Takes too long to run in Miri")] fn iso_subgraph() { let g0 = Graph::<(), ()>::from_edges(&[(0, 1), (1, 2), (2, 0)]); let g1 = Graph::<(), ()>::from_edges(&[(0, 1), (1, 2), (2, 0), (2, 3), (0, 4)]); @@ -497,6 +502,7 @@ fn iso_subgraph() { } #[test] +#[cfg_attr(miri, ignore = "Takes too long to run in Miri")] fn iter_subgraph() { let a = Graph::<(), ()>::from_edges(&[(0, 1), (1, 2), (2, 0)]); let b = Graph::<(), ()>::from_edges(&[(0, 1), (1, 2), (2, 0), (2, 3), (0, 4)]); From 48cdcd078e94566f83af466694e6d576319e8b75 Mon Sep 17 00:00:00 2001 From: clubby789 Date: Thu, 28 Jul 2022 14:01:04 +0100 Subject: [PATCH 3/5] Re-add removed comment --- src/matrix_graph.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/matrix_graph.rs b/src/matrix_graph.rs index a3f039758..cf7c9fce3 100644 --- a/src/matrix_graph.rs +++ b/src/matrix_graph.rs @@ -853,6 +853,7 @@ fn extend_flat_square_matrix( for c in (1..old_node_capacity).rev() { let pos = c * old_node_capacity; let new_pos = c * new_node_capacity; + // Move the slices directly if they do not overlap with their new position if pos + old_node_capacity <= new_pos { // first_chunk = node_adjacencies[pos..] let (_, first_chunk) = node_adjacencies.split_at_mut(pos); From 5916f482b10091efffefcbe1a2e7b8583904ffff Mon Sep 17 00:00:00 2001 From: clubby789 Date: Thu, 28 Jul 2022 15:55:27 +0100 Subject: [PATCH 4/5] Remove unsafe version and enable a test in Miri --- src/matrix_graph.rs | 11 ++--------- tests/iso.rs | 9 ++++----- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/matrix_graph.rs b/src/matrix_graph.rs index cf7c9fce3..65586fa70 100644 --- a/src/matrix_graph.rs +++ b/src/matrix_graph.rs @@ -864,15 +864,8 @@ fn extend_flat_square_matrix( let (_, second_chunk) = second_chunk.split_at_mut(new_pos - (pos + old_node_capacity)); // second_chunk = node_adjacencies[new_pos..new_pos + old_node_capacity] let (second_chunk, _) = second_chunk.split_at_mut(old_node_capacity); - // SAFETY: Slices cannot overlap as they were created by splitting a slice - // The length is equal and valid as they were both created with the same `split_at_mut` argument - unsafe { - std::ptr::swap_nonoverlapping( - first_chunk.as_mut_ptr(), - second_chunk.as_mut_ptr(), - old_node_capacity, - ); - } + // This will not panic as the chunks were both created with the same length (old_node_capacity) + first_chunk.swap_with_slice(second_chunk); } else { for i in (0..old_node_capacity).rev() { node_adjacencies.as_mut_slice().swap(pos + i, new_pos + i); diff --git a/tests/iso.rs b/tests/iso.rs index 06e426b99..13eb1ca3d 100644 --- a/tests/iso.rs +++ b/tests/iso.rs @@ -466,18 +466,17 @@ fn iso_matching() { } #[test] -#[cfg_attr(miri, ignore = "Can't open files with isolation")] fn iso_100n_100e() { - let g0 = graph_from_file("tests/res/graph_100n_100e.txt"); - let g1 = graph_from_file("tests/res/graph_100n_100e_iso.txt"); + let g0 = str_to_digraph(include_str!("res/graph_100n_100e.txt")); + let g1 = str_to_digraph(include_str!("res/graph_100n_100e_iso.txt")); assert!(petgraph::algo::is_isomorphic(&g0, &g1)); } #[test] -#[cfg_attr(miri, ignore = "Can't open files with isolation")] +#[cfg_attr(miri, ignore = "Too large for Miri")] fn iso_large() { let g0 = graph_from_file("tests/res/graph_1000n_1000e.txt"); - let g1 = graph_from_file("tests/res/graph_1000n_1000e_iso.txt"); + let g1 = graph_from_file("tests/res/graph_1000n_1000e.txt"); assert!(petgraph::algo::is_isomorphic(&g0, &g1)); } From 0e4754b5e66a3b4071291745db3b7835fff741a6 Mon Sep 17 00:00:00 2001 From: clubby789 Date: Tue, 27 Sep 2022 14:21:09 +0100 Subject: [PATCH 5/5] Switch back to copy_nonoverlapping --- src/matrix_graph.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/matrix_graph.rs b/src/matrix_graph.rs index 65586fa70..06f37b5bd 100644 --- a/src/matrix_graph.rs +++ b/src/matrix_graph.rs @@ -855,17 +855,15 @@ fn extend_flat_square_matrix( let new_pos = c * new_node_capacity; // Move the slices directly if they do not overlap with their new position if pos + old_node_capacity <= new_pos { - // first_chunk = node_adjacencies[pos..] - let (_, first_chunk) = node_adjacencies.split_at_mut(pos); - // first_chunk = node_adjacencies[pos..pos + old_node_capacity] - // second_chunk = node_adjacencies[old_node_capacity..] - let (first_chunk, second_chunk) = first_chunk.split_at_mut(old_node_capacity); - // second_chunk = node_adjacencies[new_pos..] - let (_, second_chunk) = second_chunk.split_at_mut(new_pos - (pos + old_node_capacity)); - // second_chunk = node_adjacencies[new_pos..new_pos + old_node_capacity] - let (second_chunk, _) = second_chunk.split_at_mut(old_node_capacity); - // This will not panic as the chunks were both created with the same length (old_node_capacity) - first_chunk.swap_with_slice(second_chunk); + debug_assert!(pos + old_node_capacity < node_adjacencies.len()); + debug_assert!(new_pos + old_node_capacity < node_adjacencies.len()); + let ptr = node_adjacencies.as_mut_ptr(); + // SAFETY: pos + old_node_capacity <= new_pos, so this won't overlap + unsafe { + let old = ptr.add(pos); + let new = ptr.add(new_pos); + core::ptr::swap_nonoverlapping(old, new, old_node_capacity); + } } else { for i in (0..old_node_capacity).rev() { node_adjacencies.as_mut_slice().swap(pos + i, new_pos + i);