Skip to content

Conversation

srajko
Copy link
Collaborator

@srajko srajko commented Oct 20, 2015

Applies the changes from nodegit/libgit2#2 to NodeGit.

@tbranyen
Copy link
Member

Shouldn't this fix be upstreamed to libgit2? Kind of sketched about us maintaining a separate libgit2 that differs from published versions when we update.

@srajko
Copy link
Collaborator Author

srajko commented Oct 20, 2015

@tbranyen There is an outstanding PR that includes this fix (libgit2/libgit2#3463) - this is the most critical part of that PR as it fixes a segfault. It is also probably the safest part - it only changes a malloc to a calloc.

The idea behind this PR was to get the segfault fixed in NodeGit until the upstream PR gets merged and released.

@johnhaley81
Copy link
Collaborator

Definitely not interested in maintaining a forked version of libgit2. I'd say make another PR to libgit2/libgit2#maint/v0.23 so we can expedite getting a released version of this in.

However, until that lands I'm ok with using a fork as a temporary work around.

@johnhaley81
Copy link
Collaborator

@tbranyen you cool with me merging this?

@saper
Copy link
Collaborator

saper commented Oct 22, 2015

What is really fixing the problem? Changing malloc to calloc? That's fishy a bit.

@srajko
Copy link
Collaborator Author

srajko commented Oct 22, 2015

@saper There is an explanation in libgit2/libgit2#3458 (comment)... it breaks down like this:

When we do entry->committer = git__malloc(sizeof(git_signature));, using malloc will not initialize the contents pointed to by entry->committer in any way, so the values of entry->committer->name, entry->committer->email etc. are indeterminate (that is, the values will be whatever happened to be in the memory block when it was allocated). If git_oid_fromstrn returns an error, we then immediately go to git_reflog_entry__free(entry), which in turn does git__free(entry->committer->name) and git__free(entry->committer->email). Because entry->committer->name and entry->committer->email are indeterminate, the free likely segfaults (in the included test, for me it would always segfault - in general, it will either do nothing if the value happens to be 0, free something it shouldn't free if the value happens to be a valid allocated pointer, or segfault / do some other kind of undefined behavior).

When using calloc instead of malloc, entry->committer = git__calloc(1, sizeof(git_signature)); will initialize the contents pointed to by entry->committer to all 0s. In the same scenario as above, when we get to git__free(entry->committer->name) and git__free(entry->committer->email), the free will do nothing because entry->committer->name and entry->committer->email are 0. That happens to be the correct behavior in this case.

@johnhaley81
Copy link
Collaborator

I'm going to go ahead and merge this. We'll change back to using libgi2 proper after libgit2/libgit2#3458 is merged and released.

johnhaley81 added a commit that referenced this pull request Oct 22, 2015
Update libgit2 to include segfault fix for git_reflog_read
@johnhaley81 johnhaley81 merged commit 3a19d0f into nodegit:master Oct 22, 2015
@srajko srajko deleted the reflog-read-segfault-fix branch May 9, 2016 22:20
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.

4 participants