Skip to content

SftpFileAttributes incorrect bit masking for file type, should always use & S_IFMT #1000

@fluff

Description

@fluff

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)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions