-
Notifications
You must be signed in to change notification settings - Fork 557
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
feat: use jsdiff instead of 'patch' to reduce dependency on OS binaries #355
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
module.exports = applyPatch; | ||
|
||
var debug = require('debug')('snyk'); | ||
var diff = require('diff'); | ||
var exec = require('child_process').exec; | ||
var path = require('path'); | ||
var fs = require('fs'); | ||
|
@@ -10,16 +11,12 @@ function applyPatch(patch, vuln, live) { | |
var cwd = vuln.source; | ||
|
||
return new Promise(function (resolve, reject) { | ||
|
||
var cmd = 'patch -p1 --backup --verbose < ' + patch; | ||
var test = ' --dry-run'; | ||
|
||
if (!cwd) { | ||
cwd = process.cwd(); | ||
} | ||
|
||
var relative = path.relative(process.cwd(), cwd); | ||
debug('DRY RUN: relative: %s cmd + test: %s', relative, cmd + test); | ||
debug('DRY RUN: relative: %s', relative); | ||
|
||
try { | ||
var packageJson = fs.readFileSync(path.resolve(relative, 'package.json')); | ||
|
@@ -29,48 +26,88 @@ function applyPatch(patch, vuln, live) { | |
debug('Failed loading package.json of package about to be patched', err); | ||
} | ||
|
||
// default is 200K, we increase it as we've seen | ||
// "stdout maxBuffer exceeded" erros in some user debug logs | ||
var STDOUT_BUFFER_SIZE = 5 * 1024 * 1024; | ||
var patchContent = fs.readFileSync(path.resolve(relative, patch), 'utf8'); | ||
|
||
// do a dry run first, otherwise the patch can "succeed" (exit 0) if it | ||
// only manages to patch *some* of the chunks (and leave the file partly | ||
// patched). | ||
exec(cmd + test, { | ||
cwd: cwd, | ||
env: process.env, | ||
maxBuffer: STDOUT_BUFFER_SIZE, | ||
}, function (error, stdout) { // stderr is ignored | ||
var out = stdout.trim(); | ||
if (error || out.indexOf('FAILED') !== -1) { | ||
debug('patch command failed', relative, error, out); | ||
return patchError(error, out, relative, vuln).catch(reject); | ||
} | ||
if (!live) { | ||
return resolve(); | ||
} | ||
jsDiff(patchContent, relative, live).then(function () { | ||
debug('patch succeed'); | ||
resolve(); | ||
}).catch(function (error) { | ||
debug('patch command failed', relative, error); | ||
patchError(error, relative, vuln).catch(reject); | ||
}); | ||
}); | ||
} | ||
|
||
// if it was okay, and it wasn't a dry-run, then let's do it for real | ||
exec(cmd, { | ||
cwd: cwd, | ||
env: process.env, | ||
maxBuffer: STDOUT_BUFFER_SIZE, | ||
}, function (error, stdout) { | ||
var out = stdout.trim(); | ||
if (error || out.indexOf('FAILED') !== -1) { | ||
debug('patch command failed', relative, error, out); | ||
return patchError(error, out, relative, vuln).catch(reject); | ||
function jsDiff(patchContent, relative, live) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, thanks. Would suggest renaming it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This goes way up into the flow (https://github.com/snyk/snyk/blob/master/src/cli/commands/protect/index.js#L78), used in |
||
const patchedFiles = {}; | ||
return new Promise(function (resolve, reject) { | ||
diff.applyPatches(patchContent, { | ||
loadFile: function (index, callback) { | ||
try { | ||
var fileName = trimUpToFirstSlash(index.oldFileName); | ||
if (patchedFiles[fileName]) { | ||
return callback(null, patchedFiles[fileName]); | ||
} | ||
var content = fs.readFileSync(path.resolve(relative, fileName), 'utf8'); | ||
callback(null, content); | ||
} catch (err) { | ||
callback(err); | ||
} | ||
|
||
debug('patch succeed', out); | ||
|
||
resolve(); | ||
}); | ||
}, | ||
patched: function (index, content, callback) { | ||
try { | ||
if (content === false) { | ||
adrukh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// `false` means the patch does not match the original content. | ||
throw new Error('Found a mismatching patch\n' + JSON.stringify(index, null, 2)); | ||
} | ||
var newFileName = trimUpToFirstSlash(index.newFileName); | ||
var oldFileName = trimUpToFirstSlash(index.oldFileName); | ||
if (newFileName !== oldFileName) { | ||
patchedFiles[oldFileName] = null; | ||
} | ||
patchedFiles[newFileName] = content; | ||
callback(); | ||
} catch (err) { | ||
callback(err); | ||
} | ||
}, | ||
compareLine: function (_, line, operation, patchContent) { | ||
if (operation === ' ') { | ||
adrukh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Ignore when no patch operators as GNU patch does | ||
return true; | ||
} | ||
return line === patchContent; | ||
}, | ||
complete: function (error) { | ||
if (error) { | ||
return reject(error); | ||
} | ||
if (!live) { | ||
return resolve(); | ||
} | ||
try { | ||
for (var fileName of patchedFiles) { | ||
if (patchedFiles[fileName] === null) { | ||
fs.unlinkSync(path.resolve(relative, fileName)); | ||
} | ||
fs.writeFileSync(path.resolve(relative, fileName), patchedFiles[fileName]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line looks like a potential bug generator where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This area writes the patched dependency source back to the disk, seems ok :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aha! I see your point. Will make this safer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See 908bc3a |
||
} | ||
resolve(); | ||
} catch (err) { | ||
reject(err); | ||
} | ||
}, | ||
}); | ||
}); | ||
} | ||
|
||
function patchError(error, stdout, dir, vuln) { | ||
// diff data compares the same file with a dummy path (a/path/to/real.file vs b/path/to/real.file) | ||
// skipping the dummy folder name by trimming up to the first slash | ||
function trimUpToFirstSlash(fileName) { | ||
return fileName.replace(/^[^\/]+\//, ''); | ||
} | ||
|
||
function patchError(error, dir, vuln) { | ||
if (error && error.code === 'ENOENT') { | ||
error.message = 'Failed to patch: the target could not be found.'; | ||
return Promise.reject(error); | ||
|
@@ -79,13 +116,11 @@ function patchError(error, stdout, dir, vuln) { | |
return new Promise(function (resolve, reject) { | ||
var id = vuln.id; | ||
|
||
// sneaky trick to do two sys calls in one. | ||
exec('npm -v && patch -v', { | ||
exec('npm -v', { | ||
env: process.env, | ||
}, function (patchVError, versions) { // stderr is ignored | ||
var parts = versions.split('\n'); | ||
var npmVersion = parts.shift(); | ||
var patchVersion = parts.shift(); | ||
|
||
// post the raw error to help diagnose | ||
errorAnalytics({ | ||
|
@@ -97,15 +132,13 @@ function patchError(error, stdout, dir, vuln) { | |
packageVersion: vuln.version, | ||
package: vuln.name + '@' + vuln.version, | ||
error: error, | ||
stdout: stdout, | ||
'patch-version': patchVersion, | ||
'npm-version': npmVersion, | ||
}, | ||
}); | ||
|
||
// this is a general "patch failed", since we already check if the | ||
// patch was applied via a flag, this means something else went | ||
// wrong, so we'll ask the user for help to diganose. | ||
// wrong, so we'll ask the user for help to diagnose. | ||
var filename = path.relative(process.cwd(), dir); | ||
error = new Error('"' + filename + '" (' + id + ')'); | ||
error.code = 'FAIL_PATCH'; | ||
|
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we added the @types pkg for it, so why not
.ts
it? 😉