Diff.blobToBuffer crashes the node process if given a file that contains null characters #965
Description
I was using nodegit to grab some diffs out of a repo, and when it hit a Visio file that was in there it would crash and die.
The Visio file was big enough and contained null characters in the right places such that it exposed a bug in nodegit that was causing libgit2 to throw a memory access violation error.
Here's some code that creates a new repo, writes out a file, tries to do a diff, and reproduces the issue:
var fs = require('fs');
var path = require('path');
var rimraf = require('rimraf');
var git = require('nodegit');
var repoPath = 'C:\\blobToDiff-test-repo';
var testFilePath = "test.txt";
var testFileFullPath = path.join(repoPath, testFilePath);
rimraf(repoPath, function(err) {
if (err) {
throw err;
}
console.log("rimraf " + repoPath + " successful");
fs.mkdir(repoPath, function(err) {
if (err) {
throw err;
}
console.log("mkdir " + repoPath + " successful");
var repository;
git.Repository.init(repoPath, 0)
.then(
function(repository_) {
repository = repository_;
console.log("repo successfully inited");
return repository.index();
}
)
.then(
function(index) {
return new Promise(
function(resolve, reject) {
var testFileContent = "Initial file content.";
fs.writeFile(testFileFullPath, testFileContent, function(err) {
if (err) {
reject(err);
return;
}
index.addByPath(testFilePath);
index.write();
resolve(index.writeTree());
});
}
);
}
)
.then(
function() {
console.log("file staged successfully");
var signature = git.Signature.default(repository);
return repository.createCommitOnHead([], signature, signature, "Initial commit.");
}
)
.then(
function(oid) {
console.log("commit successfully made: " + oid.toString());
return repository.getCommit(oid);
}
)
.then(
function(commit) {
console.log("commit object successfully gotten");
return commit.getTree();
}
)
.then(
function(tree) {
console.log("commit tree successfully gotten");
return tree.entryByPath(testFilePath);
}
)
.then(
function(treeEntry) {
console.log("tree entry successfully gotten");
return repository.getBlob(treeEntry.sha());
}
)
.then(
function(blob) {
console.log("blob successfully gotten: " + blob.id());
//var testDiffContent = new Buffer("abc123");
var testDiffContent = new Buffer(1362240);
var i;
for (i = 0; i < testDiffContent.length/2; i++) {
testDiffContent.write('a', i);
}
for (i = Math.floor(testDiffContent.length/2); i < testDiffContent.length; i++) {
testDiffContent.write('\0', i);
}
console.log("about to generate diff");
console.log();
return git.Diff.blobToBuffer(
blob, testFilePath,
testDiffContent, testFilePath, new git.DiffOptions(),
null,
function() {
console.log("binary file detected.");
},
function(diffDelta, diffHunk) {
console.log(
"diff" + " " + diffDelta.oldFile().path() + " " +
diffDelta.newFile().path(),
diffHunk.header().trim()
);
},
function(diffDelta, diffHunk, diffLine) {
console.log(
String.fromCharCode(diffLine.origin()) + diffLine.content()
);
}
)
}
)
.then(
function() {
console.log();
console.log("program completed successfully");
}
)
.catch(
function(e) {
console.error(e);
}
)
;
})
});This is the output of the program when it fails:
rimraf C:\blobToDiff-test-repo successful
mkdir C:\blobToDiff-test-repo successful
repo successfully inited
file staged successfully
commit successfully made: bb52bb49f1e0e3d5dfe5481675f0a859613f3aee
commit object successfully gotten
commit tree successfully gotten
tree entry successfully gotten
blob successfully gotten: b51b03824b122391a34d89c3c6afb61f2483333e
about to generate diff
Process finished with exit code -1073741819 (0xC0000005)
For whatever its worth I'm running Windows 10 x64 and nodejs 4.2.3 x64 along with the nodegit 0.11.9.
These lines from nodegit's diff.cc file seem to be the cause of the issue:
String::Utf8Value buffer(info[2]->ToString());
from_buffer = (const char *) strdup(*buffer);
}
else {
from_buffer = 0;
}
// end convert_from_v8 block
baton->buffer = from_buffer;
// start convert_from_v8 block
size_t from_buffer_len;
if (info[3]->IsNumber()) {
from_buffer_len = (size_t) info[3]->ToNumber()->Value();
}
else {
from_buffer_len = 0;
}
// end convert_from_v8 block
baton->buffer_len = from_buffer_len;They are using the strdup function, which is expecting a C-style string, which is terminated by a null. The strings we get back from nodejs are not null-terminated, nor do we want them to be, since in this case we're using the nodejs string to pass the contents of an entire file to libgit2, and that file may very well contain null characters in it depending on the file type.
The strdup, combined with trusting the buffer_len that's passed in as being accurate to the size of the buffer we have, causes libgit2 to try to read into invalid memory space, and thus crashes the program.
I'm not 100% sure of what the best way to solve this is, but I have a guess which I will send in a pull request in a few minutes.