nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.4k stars 28.99k forks source link

Node occasionally gives multiple files/folders the same inode #12115

Closed alutman closed 6 years ago

alutman commented 7 years ago

Moderator's note (@Fishrock123): Off-topic comments have and will be deleted.


Node sometimes reports different files/folders to have identical ino values. I can't reproduce this consistently and copying/reproducing a folder structure that contains dupes elsewhere doesn't replicate the issue.

I did encounter lots of duplicates under C:\Users\%USER%\AppData but it may be different for other people

Example

Specific example I encountered

# Structure
│   ServerStore.jsx
│
├───constants
│       Api.jsx
│
└───stores
        UserStore.jsx

> fs.lstatSync("stores").ino
5910974511014218
> fs.lstatSync("stores/UserStore.jsx").ino
24206847997202570
> fs.lstatSync("constants").ino //Duplicate
9851624184963316
> fs.lstatSync("constants/Api.jsx").ino //Duplicate
9851624184963316
> fs.lstatSync("ServerStore.jsx").ino
3659174697792238

Test Script

Here's a hacky node script to loop through a directory and look for duplicate inodes. Running it on most of my other folders didn't yield a result, until I ran it on C:\Users\%USER%\AppData where I encounted loads of duplicates

Usage: node dupe.js [dir]

var fs = require('fs');
var path = require('path');
var process = require('process');

// Recursively walks a directory and looks for duplicate inodes
// loop from http://stackoverflow.com/questions/5827612/node-js-fs-readdir-recursive-directory-search

var dir = process.argv[2];
if (dir == undefined) {
    dir = '.';
}

var walk = function(dir, done) {

  var results = [];
  fs.readdir(dir, function(err, list) {

    if (err) return done(err);

    var pending = list.length;
    if (!pending) return done(null, results);

    list.forEach(function(file) {

      file = path.resolve(dir, file);
      fs.stat(file, function(err, stat) {
        if(stat && stat.ino) {
            results.push({
                file: file,
                ino: stat.ino
            });
        }

        if (stat && stat.isDirectory()) {
          walk(file, function(err, res) {
            if(res) {
               results = results.concat(res);
            }
            if (!--pending) done(null, results);
          });
        } 
        else {
          if (!--pending) done(null, results);
        }
      });
    });
  });
};

walk(dir, function(err, results) {
    var merge = {};
    results.forEach(function(it) {
        if (!merge[it.ino]) {
            merge[it.ino] = [];
        }
        merge[it.ino].push(it.file);
    });
    var dupes = Object.keys(merge).filter(key => merge[key].length > 1);

    dupes.forEach(it => console.log(it, merge[it]));
})
wmertens commented 7 years ago

Would it be possible to return Symbols? And the when they are coerced to a string they are the hex value?

On Sun, May 14, 2017, 9:53 AM Vojtech Kral notifications@github.com wrote:

@dchest https://github.com/dchest Ah, makes sense, thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/12115#issuecomment-301296910, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlreQPTjR3Cmtr17WTfYJ76dtXx-Kks5r5rMVgaJpZM4MsuEt .

raffis commented 6 years ago

We have encountered the same issue, inode collisions. I shortly wrote this alternative: https://github.com/gyselroth/node-windows-fsstat maybe it helps...

XadillaX commented 6 years ago

I came from https://github.com/nodejs/node/issues/16679.

IMHO my suggestion is to add a raw data into fs.Stats, or add a new function fs.safeStat to make ino stringify.

verdy-p commented 5 years ago

I suppose it could be either a Windows bug or quirk1 but it could also be caused by node.js converting libuv's 64 bits st_ino field to a double, which won't be lossless for large numbers.

conversions of 64-bit integers to a double (Number in Javascript) is necessarily lossy. Of course a double is represented by a 64-bit field, but it is then normalized for some values of the exponent part:

So basic unsigned 64-bit integer values represented in hex by

The best you can get is not preserving the full 64 bits, but only the sign bit, then force the next bit to 0 (avoids zeros/denormals/Nans,infinites where this bit is necessarily 1) and the 62 lowest bits (note that this transform will not preserve the apparent numeric value of this coerced "double" which will be very different from the value of the 63-bit integer (note also that the 63-bit is also not fully preserved the most negative value is also forbidden): you can only represent a 62-bit signed or unsigned integer this way (to convert that "strange" double to a computable and comparable value, you still need to use a BigInt): the unsigned 64-bit 0xFFFF_FFFF_FFFF_FFFF (-1 if signed) is not stored exactly, you loose 2 bits, you can only store 0x9FFF_FFFF_FFFF_FFFF (which would also be a negative double). With such 62-bit field, you can't perform any arithmetic (all you can do is to compare them, but even a double addition or substraction or multiplication or division, or just a negation, will not give you the correct result in this representation).

So your inodes are necessarily limited to 62 bit: if the two highest bits of the 64-bit integer inode are significant, they will be lost. So you get the bug where two distinct files on the same volume (not hardlinked to the same store, so with possibly different contents and different security attributes) will apparently have the same inode in Javascript (the bug can be reproduced only on very large volumes which can hold many inodes but does not affect mosts users using harddisks or small RAID arrays: it occurs on very large cloud storages where there's a single virtual volume; it does not affect clients of these clouds that have partitioned their space in multiple virtual devices, but affects the very large volumes needed for very database storage of several petabytes). The only safe work around for now is to use a second Number to represent a 64-bit inode... or a BigInt.

If inodes are represented as Javascript numbers with identical values, then max inodes cannot be more than MAX_SAFE_INTEGER=(2**53-1) per volume, i.e. you cannot have more than ~9 billions millions inodes per volume (i.e. per mountable filesystem device), and Javascript should be safe: this is already a very considerable space that only a few very large cloud providers like Google can offer in their big farms to their business customers (but something that will be useful for example to create a filesystem-based hashstore for RDF, with lot of extremely small files, typically about 1KB each, i.e. a filesystem total size slightly above 10^16 KB when counting the extra filesytem overhead for managing its own filename indexes, transaction logs and security attributes; with typical harddisks of 1TB each, such giant volume would require more than 100,000 harddisks, not counting redundant disks for RAID 5/6, but for Google that manages millions of customers and allows allocating several Gigabytes for each, such quantity is not demesurated, but Google would normally partition this space with one virtual volume per customer)! Even for Youtube, all videos don't need to be in the same volume and have to be in the storage space of one client

So I have serious doubt that the 53 bit limit of inodes seen in Javascript is a real problem, except if you use javascript as a virtual connector to millions remote volumes, seen in Javascript as if it was only one: in that case, it's jsut best to split this huge space by creating multiple virtual volumes.

The bug may be visible however if inodes returned to applications are not linear but are actually weak hashes (such as MD5) of effective remote inodes.

Or if inodes are used not just to locate a file in the volume, but also to track other information such as a transaction id or version number, like in ReFS (not all nodes in a volume may be accessible simultaneously or with the same security context), in which case you need more bits to store that info (which may be encrypted along with the location in the inode exposed to clients, that must then use inode values exactly, without any loss).

An inode number may even be an opaque 128-bit UUID (created randomly and associated to real inodes on servers, or with a strong hash like SHA), where most inode numbers on the same volume are invalid (inaccessible to the client) and the rest is distributed completely randomly over the 128-bit space: truncating this to 53 bit would necessarily reduce the usable space on the volume to just a tiny fraction of the volume before collisions in 53-bit values start occuring for very distinct 128-bit values.

My opinion is that even Unix/Linux/POSIX will reform the datatype used for "inode numbers" to allow much longer pseudo-integer types (inode_t), and that it may as well be a long structure containing multiple words whose actual storage length/precision will depend on each device/volume, even if 64-bit will remain largely enough for any physical storage volumes like harddisks or RAID arrays or most clouds. So the BigInt representation in Javascript will still be a solution, even if Javascript is extended with native 64-bit integer support.

refack commented 5 years ago

@verdy-p fs.stats can now return a struct of BigInt - https://nodejs.org/api/fs.html#fs_fs_stat_path_options_callback

Ref: https://github.com/nodejs/node/pull/23821

verdy-p commented 5 years ago

Just another idea, the 64-bit inode integer returned by Windows could be remapped through a hash into the 53-bit address space supported by JS integers. This should give less collisions than the current folding.

This is identical to the solution of converting inode numbers to strings: strings are already hashed; but strings at least can detect collisions and ensure uniqueness.

verdy-p commented 5 years ago

@verdy-p fs.stats can now return a struct of BigInt - https://nodejs.org/api/fs.html#fs_fs_stat_path_options_callback

Ref: #23821

Unfortunately, BigInts are easily mutable, and not suitable as atomic identifiers. And they suffer from serilization locking problem (performance cost) when all we need is an atomic store, which is what strings offer natively (Javascript strings do not have to be displayable or even valid UTF-16, they are just immutable vectors of unsigned 16-bit code units; if we want to display these identifiers, we can still reencode them easily into other valid UTF-16 strings). Strings are also faster to compare (at least for equality: this is almost immediate by comparing only the references, if they are stored in a hashtable). But you may argue that we'll suffer from the storage cost of the hastable to make them atomic if we want to manage large collections of inodes: they would slow down all the rest of the API using strings. In terms of memory management and garbage collection, their cost will be equivalent to BigInts which also need to be allocated (the hashing of inode strings is not absolutely necessary as these strings are short enough to be compared directly, without comparing first their hash then looking up their actual location in the hashtable). I'm not sure that BigInts are solutions, it is largely possible and very likely that file identifiers will be strings with variable length, just like URIs: consider WebDav filesystems...

ljharb commented 5 years ago

BigInts are primitives; how are they mutable?

refack commented 5 years ago

Adding a string inode field will require some non trivial changes to our marshaling logic. ATM we pass the data as using a globaly mapped array of either double or BigInt: https://github.com/nodejs/node/blob/98278584ee2c322655849e6d673ac1739e720b53/src/node_file.h#L193-L219 Adding another string value will also have a performance penalty.

We could add make this an opt-in new option, something like fs.stat({withStringINode: true})

verdy-p commented 5 years ago

fs.stat({withStringINode: true}) I see that more like:

  • fs.stat({as: {ino: 'string'}}), or
  • fs.stat({as: {ino: 'BigInt'}}), the default option being
  • fs.stat({as: {ino: 'Number'}}) notably if other fields will need to change size in the future (e.g. device numbers, mode, nlink, uid/gid, size/blksize/blocks, dates, ...)

As well some of the returned properties returned by fs.stat() may not be necessary, but costly to get from the underlying OS, and a script may just require a specific set of values, or just one, leaving others possibly unset in the returned Javascript object (also if these properties make no sense for the remote filesystem):

refack commented 5 years ago

So I'm thinking iteratively. ATM we have:

What I'm suggesting is adding an independent { inodeString } option that will be orthogonal to bigint true or false, i.e.

This is a nice server-minor change. It's opt-in so those who don't want it don't pay for it.

As for unneeded fields, on POSIX we use stat syscall, so we get everything at the same price. If we're considering backward compatibility, it's safer to keep that as it is.

verdy-p commented 5 years ago

Note that you speak about POSIX, not actual systems. Even if most OSes implement more or less the POSIX API, filling all the fields may be costly, and not enough to get file info for all filesystems. In the web context we can speak about virtual filesystems with web APIs that need security enforcement, and where getting all the infos at once may be costly or simply denied: under POSIX rules they would be set to pseudovalues like you do in your code by setting arbitrarily some fields arbitrarily to 0. A file may itself have also several contents: a main content with size (all other fields are not relevant), and possibly an additional directory content which is enumeratable (in NTFS this could be a set of streams, on the web it would be roughly the same as adding a "/" to an URL) and you could query that content with fs.stat({props: "dir"}): the property "dir" returned would be a directory object, not a simple string or number. There can be potentially a lot of properties for files, like versions, views, streams; users/groups are not necessarily reduced to just a single integer; modes are not necesssarily limited to just a few bits and we may need to query modes for specific groups/users, or according to the securityTokens provided. Filesize and dates may eventually be not defined (for continuous live streams). Another info is if the file can be seeked to random positions or not, or if it can be seeked before its initial start position (a live stream may have a record of the last hour before the first position you get when opening it; file positions you can seek to are not necesssarily byte offsets but may be timestamps and two successive timestamps may be separated by unbreakable records with variable byte sizes. Conceptually the POSIX rules just tries to mimic what was in the minimum common set of features implemented in the first filesystems for Unix; not taking into account new filesystems/data services that have appeared over time or those that existed since long in other systems like former mainframes, then VMS, and then NT, and the web (many of these are now integrable in Unix/Linux as well).

dchest commented 5 years ago

Why is there's a theoretical discussion about future and past file systems, and request for string option? The original issue was solved by introducing BigInt for inode numbers, which is good enough for existing file systems on all platforms supported by Node, in line with the purpose of fs module: "The fs module provides an API for interacting with the file system in a manner closely modeled around standard POSIX functions", can represent arbitrary data in an immutable value, and can be compared with ==. (The only downside compared to strings is that it can't currently be directly encoded with JSON.stringify, but that's a problem for those who encode them to solve.) I only proposed to use string or any other immutable object because we didn't have BigInt, but now that we have them, it's as good as anything else: a value referencing an inode.