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 UB and add Miri to Github CI #505

Merged
merged 5 commits into from Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/ci.yml
Expand Up @@ -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
2 changes: 1 addition & 1 deletion Cargo.toml
Expand Up @@ -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 }
Expand Down
13 changes: 7 additions & 6 deletions src/matrix_graph.rs
Expand Up @@ -853,15 +853,16 @@ fn extend_flat_square_matrix<T: Default>(
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();
Comment on lines -859 to -860

Choose a reason for hiding this comment

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

FYI my usual solution to this is

let ptr = node_adjacencies.as_mut_ptr();
let old = ptr.add(pos);
let new = ptr.add(new_pos);

This has zero overhead, and if anything is faster because it omits the bounds checks. Unclear if those are desired, but you could add them back in explicitly if you want.


// SAFE: new starts at least `old_node_capacity` positions after old.
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 {
std::ptr::swap_nonoverlapping(old, new, old_node_capacity);
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() {
Expand Down
11 changes: 8 additions & 3 deletions tests/iso.rs
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -465,15 +467,16 @@ fn iso_matching() {

#[test]
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 = "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));
}

Expand All @@ -489,6 +492,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)]);
Expand All @@ -497,6 +501,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)]);
Expand Down