rust-lang / rust-lang/git2-rs

Problematic raw pointer conversion in buf.rs

Open
#760 5 comments 0 reactions 0 assignees View on GitHub

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.