Problematic raw pointer conversion in buf.rs
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 2.1k
- Forks
- 450
- Avg merge
- 11m
- Merged PRs (30d)
- 1
Description
Hello all,
I'm not sure if I made a conceptual mistake here and of not if this is actually a problem, but I would like to ask you to have a look at this.
Here are the tests that I ran on buf.rs
#[cfg(test)]
mod buf_test {
use crate::{util::Binding, Buf};
use libgit2_sys::git_buf;
#[test]
#[allow(unused)]
fn pointer_tests() {
let buf: Buf = Buf::new();
let buf_addr: u64 = (&buf) as *const Buf as u64;
let buf_raw_addr: u64 = (&buf.raw) as *const git_buf as u64;
/* Since Buf is just a wrapper around raw::git_buf which actually holds the raw::git_buf struct,
they should refer to the same memory address */
assert_eq!(buf_addr, buf_raw_addr);
let git_buf: *mut git_buf = Binding::raw(&buf);
let git_buf_addr: u64 = git_buf as u64;
/* It should not make a difference if we retrieve the raw::git_buf from Binding::raw or from struct Buf.raw */
assert_eq!(buf_raw_addr, git_buf_addr);
let from_raw_buf: Buf = unsafe { Binding::from_raw(git_buf) };
let from_raw_buf_addr: u64 = (&from_raw_buf) as *const Buf as u64;
/* If we recreate Buf from raw pointer to git_buf and Buf actually holds the struct raw::git_buf we should end up with
the same memory address as to begin with (which is not possible) or we have moved raw::git_buf which could cause trouble
on the C side since C does not know about our move. */
assert_eq!(buf_addr, from_raw_buf_addr);
let from_raw_buf_raw_addr: u64 = (&from_raw_buf.raw) as *const git_buf as u64;
/* This actually shows that we moved raw::git_buf here
impl Binding for Buf {
type Raw = *mut raw::git_buf;
unsafe fn from_raw(raw: *mut raw::git_buf) -> Buf {
Buf { raw: *raw } // move of raw pointer
}
This behavior may be problematic when Rust receives a buffer from C to write into e.g. here
@param out git_buf to write data into
@param repo Repository to read prepared message from
@return 0, GIT_ENOTFOUND if no message exists or an error code
GIT_EXTERN(int) git_repository_message(git_buf *out, git_repository *repo);
*/
assert_eq!(buf_raw_addr, from_raw_buf_raw_addr);
}
}
So my assumption is, that the struct
pub struct Buf {
raw: raw::git_buf,
}
should have been
pub struct Buf {
raw: *mut raw::git_buf,
}
similar to repo.rs
pub struct Repository {
raw: *mut raw::git_repository,
}
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in buf.rs with the Buf implementation and Binding::raw/from_raw, then compare its representation with Repository in repo.rs. Run the pointer_tests shown in the issue and inspect the git_repository_message usage. Done means establishing whether the raw-pointer conversion is safe for C output buffers and adding or updating tests to verify the correct behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100