-
-
Notifications
You must be signed in to change notification settings - Fork 968
Description
Hello, it looks like the code in SftpFileAttributes.cs that handle the file type bits in the permission uint uses the wrong bitmask when comparing against the various file type flags.
For example, a symbolic link, while correctly returning true for IsSymbolicLink, also incorrectly returns true for IsCharacterDevice.
In particular, I think these lines in the private setter should be changed from:
IsSocket = ((value & S_IFSOCK) == S_IFSOCK);
IsSymbolicLink = ((value & S_IFLNK) == S_IFLNK);
IsRegularFile = ((value & S_IFREG) == S_IFREG);
IsBlockDevice = ((value & S_IFBLK) == S_IFBLK);
IsDirectory = ((value & S_IFDIR) == S_IFDIR);
IsCharacterDevice = ((value & S_IFCHR) == S_IFCHR);
IsNamedPipe = ((value & S_IFIFO) == S_IFIFO);
to
IsSocket = ((value & S_IFMT) == S_IFSOCK);
IsSymbolicLink = ((value & S_IFMT) == S_IFLNK);
IsRegularFile = ((value & S_IFMT) == S_IFREG);
IsBlockDevice = ((value & S_IFMT) == S_IFBLK);
IsDirectory = ((value & S_IFMT) == S_IFDIR);
IsCharacterDevice = ((value & S_IFMT) == S_IFCHR);
IsNamedPipe = ((value & S_IFMT) == S_IFIFO);
i.e. the value should always be bitmask-AND'ed with S_IFMT (aka 0xF000) before comparing to the file type.
Otherwise you get a false-positive true result for things like symbolic links (0xA000) being compared to character device (0x2000) because (0xA000 & 0x2000) == 0x2000 is true (but incorrect), but it probably should be (0xA000 & 0xF000) == 0x2000 which is false (and correct).
Also, the usage of private bool _isBitFiledsBitSet seems unnecessary. Perhaps this private bool field should be removed entirely (including the two statements that get and set the value). I don't think it makes sense to set permission = permission | S_IFMT since S_IFMT is just a bitmask intended to be used for bitwise-AND operations (&) to find the other file type values.
Also, somewhat unrelated, I wish the three private bools _isUIDBitSet, _isGroupIDBitSet and _isStickyBitSet were exposed in the public API with at least getters, so that we can show the full -rwsr-xr-x unix permission for /bin/su for example. (With the current API, we can't know that _isUIDBitSet is true so we'd have to show it as just -rwxr-xr-x instead). In fact, having a public getter for the uint permission directly would be very useful as it would allow us to pass a single uint to for example a web frontend, instead of dozens of IsXyz + XyzCanRead booleans.
This affects both the develop branch as well as the release/2020.0 branch (the file SftpFileAttributes.cs is identical in both branches)