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

Tie Patch lifetime to its input buffers #523

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

yuyoyuppe
Copy link
Contributor

It's currently possible to get a use-after-free errors when using Patch::from_buffers. Here's a small code to repro:

use git2::*;

fn main() {
    let repo = Repository::open(REPO_PATH).unwrap();
    let old_file = repo
        .find_blob(Oid::from_str(BLOB_SHA1).unwrap())
        .unwrap();
    let new_file = repo
        .find_blob(Oid::from_str(BLOB_SHA2).unwrap())
        .unwrap();

    let diff_options = &mut DiffOptions::new();
    let patch = Patch::from_buffers(
        old_file.content(),
        None,
        new_file.content(),
        None,
        Some(diff_options),
    )
    .unwrap();

     std::mem::drop(old_file);
     std::mem::drop(new_file);

    for hunk_idx in 0..patch.num_hunks() {
        for line_idx in 0..patch.num_lines_in_hunk(hunk_idx).unwrap() {
            let diff_line = patch.line_in_hunk(hunk_idx, line_idx).unwrap();
            let line = String::from_utf8(diff_line.content().into())
                .map(|s| String::from(s.trim_end()))
                .unwrap_or_else(|e| {
                    println!("utf8 decoding error: {}", e);
                    String::from("!USE AFTER FREE!")
                });
            println!("line: {}", line);
        }
    }
}

On my system it fails in ~5% cases, so it may be hard to repro. That happens because we're passing old_file.content()/old_file.content() pointers, free them with mem::drop and try to use them again.

We also don't attempt to double free the pointers in patch_generated_free during Patch drop, since we don't have GIT_DIFF_FLAG__FREE_DATA flags on theirs git_diff_file_content.

I understand that this change could break a lot of code and probably is overly defensive, since I'm not sure if other Patch::from_* functions behave the same, so I'm open to suggestions on how to improve this PR!

@alexcrichton
Copy link
Member

Oops thanks so much for catching this, and thanks for the PR!

@alexcrichton alexcrichton merged commit ecd56b0 into rust-lang:master Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants