René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Diff.blobToBuffer crashes the node process if given a file that contains null characters","articleBody":"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.\n\nThe 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.\n\nHere's some code that creates a new repo, writes out a file, tries to do a diff, and reproduces the issue:\n\n``` js\nvar fs = require('fs');\nvar path = require('path');\n\nvar rimraf = require('rimraf');\nvar git = require('nodegit');\n\nvar repoPath = 'C:\\\\blobToDiff-test-repo';\nvar testFilePath = \"test.txt\";\nvar testFileFullPath = path.join(repoPath, testFilePath);\n\nrimraf(repoPath, function(err) {\n    if (err) {\n        throw err;\n    }\n\n    console.log(\"rimraf \" + repoPath + \" successful\");\n\n    fs.mkdir(repoPath, function(err) {\n        if (err) {\n            throw err;\n        }\n\n        console.log(\"mkdir \" + repoPath + \" successful\");\n\n        var repository;\n        git.Repository.init(repoPath, 0)\n            .then(\n                function(repository_) {\n                    repository = repository_;\n                    console.log(\"repo successfully inited\");\n                    return repository.index();\n                }\n            )\n            .then(\n                function(index) {\n                    return new Promise(\n                        function(resolve, reject) {\n                            var testFileContent = \"Initial file content.\";\n\n                            fs.writeFile(testFileFullPath, testFileContent, function(err) {\n                                if (err) {\n                                    reject(err);\n                                    return;\n                                }\n\n                                index.addByPath(testFilePath);\n                                index.write();\n                                resolve(index.writeTree());\n                            });\n                        }\n                    );\n                }\n            )\n            .then(\n                function() {\n                    console.log(\"file staged successfully\");\n                    var signature = git.Signature.default(repository);\n                    return repository.createCommitOnHead([], signature, signature, \"Initial commit.\");\n                }\n            )\n            .then(\n                function(oid) {\n                    console.log(\"commit successfully made: \" + oid.toString());\n                    return repository.getCommit(oid);\n                }\n            )\n            .then(\n                function(commit) {\n                    console.log(\"commit object successfully gotten\");\n                    return commit.getTree();\n                }\n            )\n            .then(\n                function(tree) {\n                    console.log(\"commit tree successfully gotten\");\n                    return tree.entryByPath(testFilePath);\n                }\n            )\n            .then(\n                function(treeEntry) {\n                    console.log(\"tree entry successfully gotten\");\n                    return repository.getBlob(treeEntry.sha());\n                }\n            )\n            .then(\n                function(blob) {\n                    console.log(\"blob successfully gotten: \" + blob.id());\n\n                    //var testDiffContent = new Buffer(\"abc123\");\n                    var testDiffContent = new Buffer(1362240);\n                    var i;\n\n                    for (i = 0; i \u003c testDiffContent.length/2; i++) {\n                        testDiffContent.write('a', i);\n                    }\n                    for (i = Math.floor(testDiffContent.length/2); i \u003c testDiffContent.length; i++) {\n                        testDiffContent.write('\\0', i);\n                    }\n\n                    console.log(\"about to generate diff\");\n                    console.log();\n\n                    return git.Diff.blobToBuffer(\n                        blob, testFilePath,\n                        testDiffContent, testFilePath, new git.DiffOptions(),\n                        null,\n                        function() {\n                            console.log(\"binary file detected.\");\n                        },\n                        function(diffDelta, diffHunk) {\n                            console.log(\n                                \"diff\" + \" \" + diffDelta.oldFile().path() + \" \" +\n                                diffDelta.newFile().path(),\n                                diffHunk.header().trim()\n                            );\n                        },\n                        function(diffDelta, diffHunk, diffLine) {\n                            console.log(\n                                String.fromCharCode(diffLine.origin()) + diffLine.content()\n                            );\n                        }\n                    )\n                }\n            )\n            .then(\n                function() {\n                    console.log();\n                    console.log(\"program completed successfully\");\n                }\n            )\n            .catch(\n                function(e) {\n                    console.error(e);\n                }\n            )\n        ;\n    })\n});\n```\n\nThis is the output of the program when it fails:\n\n```\nrimraf C:\\blobToDiff-test-repo successful\nmkdir C:\\blobToDiff-test-repo successful\nrepo successfully inited\nfile staged successfully\ncommit successfully made: bb52bb49f1e0e3d5dfe5481675f0a859613f3aee\ncommit object successfully gotten\ncommit tree successfully gotten\ntree entry successfully gotten\nblob successfully gotten: b51b03824b122391a34d89c3c6afb61f2483333e\nabout to generate diff\n\n\nProcess finished with exit code -1073741819 (0xC0000005)\n```\n\nFor whatever its worth I'm running Windows 10 x64 and nodejs 4.2.3 x64 along with the nodegit 0.11.9.\n\nThese lines from nodegit's `diff.cc` file seem to be the cause of the issue:\n\n``` cpp\nString::Utf8Value buffer(info[2]-\u003eToString());\n  from_buffer = (const char *) strdup(*buffer);\n  }\n  else {\n    from_buffer = 0;\n  }\n// end convert_from_v8 block\n  baton-\u003ebuffer = from_buffer;\n// start convert_from_v8 block\n  size_t from_buffer_len;\n    if (info[3]-\u003eIsNumber()) {\n  from_buffer_len = (size_t)   info[3]-\u003eToNumber()-\u003eValue();\n  }\n  else {\n    from_buffer_len = 0;\n  }\n// end convert_from_v8 block\n  baton-\u003ebuffer_len = from_buffer_len;\n```\n\nThey 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.\n\nThe `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.\n\nI'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.\n","author":{"url":"https://github.com/tylerchurch","@type":"Person","name":"tylerchurch"},"datePublished":"2016-03-24T02:28:46.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":2},"url":"https://github.com/965/nodegit/issues/965"}