bitflicker64 commented on code in PR #359: URL: https://github.com/apache/hugegraph-computer/pull/359#discussion_r3911923847
########## computer-rust/src/lib.rs: ########## @@ -0,0 +1,25 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +pub mod ffi; +pub mod fixtures; +pub mod kernel; + +pub use kernel::csr::CsrGraph; +pub use kernel::pagerank::PageRankKernel; + +pub const RUST_KERNEL_VERSION: &str = "1.5.0"; Review Comment: 🧹 This hand-copies `version = "1.5.0"` from `Cargo.toml:18`, and it is the string that `computer_kernel_version()` (`src/ffi/c_api.rs:128`) exports across the C ABI. `docs/rust-modernization-roadmap.md:77` documents that export as part of the stable boundary, and the Phase 3 JNI and CGO hosts (lines 33, 91-92) are the consumers. A routine version bump touches `Cargo.toml` only, so the exported version goes stale silently. The test at `c_api.rs:215` cannot catch the drift either: it asserts the exported string equals this same constant. Please derive it from the manifest: ```rust pub const RUST_KERNEL_VERSION: &str = env!("CARGO_PKG_VERSION"); ``` ########## computer-rust/src/kernel/csr.rs: ########## @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#[derive(Debug, Clone, Default)] +pub struct Edge { + pub target: u32, + pub weight: f64, +} + +#[derive(Debug, Clone)] +pub struct CsrGraph { + num_vertices: u32, + row_offsets: Vec<usize>, + column_indices: Vec<u32>, + edge_weights: Vec<f64>, +} + +impl CsrGraph { + pub fn new(num_vertices: u32) -> Self { + Self { + num_vertices, + row_offsets: vec![0; (num_vertices + 1) as usize], + column_indices: Vec::new(), + edge_weights: Vec::new(), + } + } + + pub fn from_edges(num_vertices: u32, edges: &[(u32, u32, f64)]) -> Self { + let mut degree = vec![0; num_vertices as usize]; + for &(src, dst, _weight) in edges { + if src < num_vertices && dst < num_vertices { + degree[src as usize] += 1; + } + } + + let mut row_offsets = vec![0; (num_vertices + 1) as usize]; Review Comment: ⚠️ Nothing bounds `num_vertices` on the way in, and `from_edges` allocates `8 * num_vertices` bytes three times over: `degree` (43), `row_offsets` (this line), and the `current_pos` clone (58). `computer_graph_create` (`ffi/c_api.rs:35`) validates nothing and `computer_graph_finalize` (`ffi/c_api.rs:74`) calls straight in, so `computer_graph_create(1 << 31)` followed by a finalize with zero edges asks for roughly 48 GiB from two C calls. The sharp edge is at `u32::MAX`. `num_vertices + 1` is computed in `u32` before the widening cast (same pattern at line 36), and `[profile.release]` (`Cargo.toml:39-43`) never sets `overflow-checks`, so release keeps the wrapping default and this becomes `vec![0; 0]`. Line 52 then panics with index out of bounds on the first iteration, and `panic = "abort"` at `Cargo.toml:43` turns that into a dead host process rather than a return code. This is a separate path from the `PageRankKernel::new` panic already raised at `ffi/c_api.rs:108`. Please use `num_vertices as usize + 1` at lines 36 and 50, and bound `num_vertices` in `computer_graph_create` by returning `NULL`. Neither `computer_rust_c_api.h:34` nor `docs/rust-modernization-roadmap.md:71` states a limit today. ########## computer-rust/src/ffi/c_api.rs: ########## @@ -0,0 +1,217 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#![allow(clippy::not_unsafe_ptr_arg_deref)] Review Comment: ⚠️ This module-wide `allow` silences the lint that is correctly reporting an unsound boundary. `computer_graph_add_edge` (45), `computer_graph_finalize` (69), `computer_graph_compute_pagerank` (80) and `computer_graph_free` (117) are declared `pub extern "C" fn`, not `pub unsafe extern "C" fn`, yet each dereferences a pointer the caller supplies: `&mut *handle` (54, 73), `&*handle` (91), `slice::from_raw_parts_mut` (111), `Box::from_raw` (120). The null checks do not recover safety. `computer_graph_free(0x1 as *mut GraphBuilder)` passes `!handle.is_null()` and reaches `Box::from_raw` on a bogus address. Because these functions are safe, no call site needs an `unsafe` block to do that, and the crate's own tests (143-158, 169-180, 186-203) already call them from safe Rust today. Please declare those four `pub unsafe extern "C" fn` and drop this `allow`. The exported symbols and the C ABI are unchanged, since `unsafe` only constrains Rust callers. The same change needs `unsafe` blocks around the three test call sites above. ########## computer-rust/Cargo.toml: ########## @@ -0,0 +1,43 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +[package] +name = "hugegraph-computer-rust" +version = "1.5.0" +edition = "2021" +authors = ["Apache HugeGraph Authors <[email protected]>"] +license = "Apache-2.0" +description = "High-performance Rust graph computing kernels for HugeGraph Computer and Vermeer" +repository = "https://github.com/apache/hugegraph-computer"; + +[lib] +name = "hugegraph_computer_rust" +crate-type = ["cdylib", "staticlib", "rlib"] + +[dependencies] +libc = "0.2" Review Comment: 🧹 `libc` is declared but never used. `git grep -n libc c1fc10a -- computer-rust` returns this line and nothing else; the FFI layer uses `std::ffi::CString`, `std::os::raw::c_char` and `std::slice` (`src/ffi/c_api.rs:23-25`). Neither `cargo build` nor the new `cargo clippy --all-targets -- -D warnings` gate reports an unused dependency, so this will not surface on its own. Please drop it, or switch `c_api.rs` to `libc::c_char` if a future `no_std` path is the intent. Declaring it while using `std::os::raw` gets the cost without the benefit. ########## .licenserc.yaml: ########## @@ -73,6 +73,7 @@ header: # `header` section is configurations for source codes license header. - '**/target/*' - '**/go.mod' - '**/go.sum' + - '**/Cargo.lock' Review Comment: 🧹 This exclusion is added for a file the PR never commits. `git ls-tree -r --name-only c1fc10a -- computer-rust` lists twelve files and no `Cargo.lock`, and `.gitignore` has no Cargo entry, so the entry is dead config today and the crate builds unpinned. That matters for the new gate. `.github/workflows/rust-ci.yml:69` runs `cargo clippy --all-targets -- -D warnings` on a floating `dtolnay/rust-toolchain@stable` against freshly resolved dependencies, so a new clippy lint or an upstream minor release turns the gate red on the next unrelated Rust PR, with no lockfile to bisect against. The cache key `hashFiles('computer-rust/Cargo.toml')` (`rust-ci.yml:62`) cannot see resolution changes that a lockfile would capture either. Please commit `computer-rust/Cargo.lock`, which is what this exclusion and the neighbouring committed `go.mod`/`go.sum` entries imply was intended. A `rust-toolchain.toml` would pin the other half. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
