Add heuristics to stats.ino comparison when bigint is not available #694
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbargiel
force-pushed
the
feature/stats-heuristics-without-bigint
branch
from
10b96d3
to
bea4a39
Compare
|
Sorry about the last commit (10b96d3), I pushed too fast - it's unnecessary. I removed it. |
|
Checked the coverage loss - it's due to the fact that the code branch where I added the heuristic checks was not covered by tests (of course, since BigInt is supported in the test runs used by the coverage check). |
manidlou
reviewed
Jul 26, 2019
lib/util/stat.js
Outdated
| throw new Error(errMsg(src, dest, funcName)) | ||
| } | ||
| return checkParentPathsSync(src, srcStat, destParent, funcName) | ||
| } | ||
|
|
||
| function areApparentlyIdentical (srcStat, destStat) { |
There was a problem hiding this comment.
This is good but the function name is a bit confusing. Will you please rename it to something simpler like just areIdentical?
manidlou
approved these changes
Jul 27, 2019
mbargiel
force-pushed
the
feature/stats-heuristics-without-bigint
branch
from
7e4c852
to
391fb73
Compare
|
I'm good with merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The resolution of #657 relies on Node >= 10.5, but actually leaves the issue not fixed on previous versions of Node (in particular, for the OP, who reported the issue on Node 8.11.3). The issue particularly problematic and frequent in automated setups, such as running tests that create files in sequence, where it is likely to happen.
This PR minimizes the frequency of this issue by comparing additional
fs.Statsvalues whenBigintsupport is not available. Note that the check is still heuristic: different stats prove they are different files*, but while same stats do not prove they are the same, we must err on the side of caution and still treat them as the same file.(*) Race condition: comparing file stats before performing a copy or other operation implicitly introduces a race condition. However the existing implementation already suffered from this, so we presume that this PR does not make the condition much worse (with the exception that unlike
inoandvol, a malicious user could in theory manipulate some of the otherstats). In practice, this race condition seems extremely hard to abuse, since it requires using a vulnerable system, and a means to change the file stats between the twostatscalls made bygetStatsandgetStatsSync.