Qt
  1. Qt
  2. QTBUG-56366

QSaveFile results in loss of extended file attributes

    Details

    • Type: Bug Bug
    • Status: Reported
    • Priority: P3: Somewhat important P3: Somewhat important
    • Resolution: Unresolved
    • Affects Version/s: 5.6.1
    • Fix Version/s: None
    • Component/s: Core: I/O
    • Labels:
      None
    • Environment:

      openSUSE Tumbleweed (Linux)

      Description

      QSaveFile, while advertizing itself for writing "without losing existing data if the writing operation fails", sadly loses existing data when the writing operation succeeds: the ownership of the file is changed to the system user and group of the current process (on linux) and any extended file attributes are dropped (unless direct write happens).

      How to reproduce:

      Prepare test file test.txt, making it owned by another system user in a special group (not users) shared with your own account

      touch test.txt
      # allow write access to the group
      chmod g+w test.txt
      # make file owned by another system user and by some special group (not "users") which the current user and other user are both in
      sudo chown otheruser:specialgroup test.txt
      # add some extended file attribut
      setfattr -n user.comment -v "foo" test.txt
      

      See metadata:

      ls -l test.txt
      getfattr -n user.comment test.txt
      

      Compile and run:

      qmake-qt5 && make
      ./qsavefile
      

      See how metadata is lost/changed:

      ls -l test.txt
      getfattr -n user.comment test.txt
      

      A fix to not lose the extended file attributes might be to copy them over to the temporary file, no idea if that works for all systems and all data, I only hope it does.
      The file ownership seems to be more tricky, if not unsolvable. Perhaps there could be another flag/mode like "useDirectWriteForNotOwnedFiles" (example name only), so the consumers of QSaveFile could tell if the value transaction support also over changed file ownership or not.

      At least both issues should be documented in the API dox of QSaveFile (including the lack of support for hard links). This behaviour comes as painful surprise to your naive developer :/ Too bad in 2016 filesystems still seem do not have any proper native support for transactional updates and such hard-to-get-right hacks are needed.

      1. main.cpp
        0.3 kB
        Friedrich W. H. Kossebau
      2. qsavefile.pro
        0.1 kB
        Friedrich W. H. Kossebau
      No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

        Hide
        Oswald Buddenhagen added a comment -

        this isn't "real" data loss, so this is in no way P1.

        arguably, foreign file ownership should activate the direct write mode fallback when it is enabled. it may have been this way in ksavefile. otoh, risking real data loss to preserve file ownership seems like a somewhat poor trade-off.

        this particular problem could be actually avoided with COW filesystems like btrfs - create a cow link of the original file, and then write directly into the copy, so it preserves all attributes. then rename it over the original file as usual.
        what this does not avoid is the need to have write permissions to the parent directory.

        i think reiserfs actually had support for real transactions, as it aimed to integrate many database features directly into the fs.

        Show
        Oswald Buddenhagen added a comment - this isn't "real" data loss, so this is in no way P1. arguably, foreign file ownership should activate the direct write mode fallback when it is enabled. it may have been this way in ksavefile. otoh, risking real data loss to preserve file ownership seems like a somewhat poor trade-off. this particular problem could be actually avoided with COW filesystems like btrfs - create a cow link of the original file, and then write directly into the copy, so it preserves all attributes. then rename it over the original file as usual. what this does not avoid is the need to have write permissions to the parent directory. i think reiserfs actually had support for real transactions, as it aimed to integrate many database features directly into the fs.
        Hide
        Thiago Macieira added a comment -

        Agreed, it's not data that is lost, but metadata. Also note it will break hardlinks, if any existed, and that's unavoidable. Onwership will always change too, period.

        We should copy any extended attributes and possibly the file permissions from the original file.

        Show
        Thiago Macieira added a comment - Agreed, it's not data that is lost, but metadata. Also note it will break hardlinks, if any existed, and that's unavoidable. Onwership will always change too, period. We should copy any extended attributes and possibly the file permissions from the original file.
        Hide
        Marc Wathelet added a comment -

        I'm using Kate (built against Qt 5.6.1) to edit files on a web server through a sshfs mount. Loosing file ownership is really painful.

        To preserve all metadata and data even in case of partial writing, you can do a backup copy of the original file (with the same limitations as renaming for directory permissions) and write to the original file directly. It is certainly slower than renaming and writing to a new file, but everything is preserved. In case of crash while writing, it is effectively less convenient than the method proposed by QSaveFile, a recovering method is necessary.

        Show
        Marc Wathelet added a comment - I'm using Kate (built against Qt 5.6.1) to edit files on a web server through a sshfs mount. Loosing file ownership is really painful. To preserve all metadata and data even in case of partial writing, you can do a backup copy of the original file (with the same limitations as renaming for directory permissions) and write to the original file directly. It is certainly slower than renaming and writing to a new file, but everything is preserved. In case of crash while writing, it is effectively less convenient than the method proposed by QSaveFile, a recovering method is necessary.
        Hide
        Thiago Macieira added a comment -

        In "you can do a backup", "you" refers to Kate developers.

        Show
        Thiago Macieira added a comment - In "you can do a backup", "you" refers to Kate developers.
        Hide
        Oswald Buddenhagen added a comment -

        i'm not quite sure why losing file ownership is "really painful". the permissions stay the same, so the ownership situation becomes inverse, but access stays the same. except for the xattr problem, that is.

        on another note, the windows api has support for filesystem transactions, so i presume NTFS actually does as well. somebody feels like giving this a shot?

        Show
        Oswald Buddenhagen added a comment - i'm not quite sure why losing file ownership is "really painful". the permissions stay the same, so the ownership situation becomes inverse, but access stays the same. except for the xattr problem, that is. on another note, the windows api has support for filesystem transactions, so i presume NTFS actually does as well. somebody feels like giving this a shot?
        Hide
        Friedrich W. H. Kossebau added a comment -

        Changed file ownership (user & group) can be painful because e.g.

        • only the owner has chmod rights, so this will break workflows (you share your file with others and cannot simply unshare it again)
        • QSaveFile seems to use default group of the user (not sure, perhaps of process gid?) for the new file version, which can result in taking complete access to the file for others (e.g. for service specific groups) as the (system) users might not be part of that new group, and instead might even make it accessable to others who should not be able to access it. And all that as side-effect without any warning/hint.
          Got some idea of possible pain levels now?
        Show
        Friedrich W. H. Kossebau added a comment - Changed file ownership (user & group) can be painful because e.g. only the owner has chmod rights, so this will break workflows (you share your file with others and cannot simply unshare it again) QSaveFile seems to use default group of the user (not sure, perhaps of process gid?) for the new file version, which can result in taking complete access to the file for others (e.g. for service specific groups) as the (system) users might not be part of that new group, and instead might even make it accessable to others who should not be able to access it. And all that as side-effect without any warning/hint. Got some idea of possible pain levels now?
        Hide
        Oswald Buddenhagen added a comment -

        as the file needs to be in a directory which both users have write access to, you can trivially restore your ownership by creating a copy yourself and moving it over the old file. and move it elsewhere, because you wouldn't keep a read-protected file in a shared write directory.

        the group thing is a genuine bug. feel free to submit a fix (it can be unix-specific; the windows solution is the xattr-based one).

        Show
        Oswald Buddenhagen added a comment - as the file needs to be in a directory which both users have write access to, you can trivially restore your ownership by creating a copy yourself and moving it over the old file. and move it elsewhere, because you wouldn't keep a read-protected file in a shared write directory. the group thing is a genuine bug. feel free to submit a fix (it can be unix-specific; the windows solution is the xattr-based one).
        Hide
        Thiago Macieira added a comment -

        The group and xattr should be retained. That's what this bug is about and we'd appreciate a patch.

        The file mode & ACLs should also be retained, so long as the owner didn't change. If the owner did change (and we can't do anything about that), then it's best to reset the file permissions and let the process umask keep it. Otherwise, we might end up with a file that cannot be read from by the user that has just created it.

        Show
        Thiago Macieira added a comment - The group and xattr should be retained. That's what this bug is about and we'd appreciate a patch. The file mode & ACLs should also be retained, so long as the owner didn't change. If the owner did change (and we can't do anything about that), then it's best to reset the file permissions and let the process umask keep it. Otherwise, we might end up with a file that cannot be read from by the user that has just created it.

          People

          • Assignee:
            David Faure
            Reporter:
            Friedrich W. H. Kossebau
          • Votes:
            3 Vote for this issue
            Watchers:
            14 Start watching this issue

            Dates

            • Created:
              Updated:

              Gerrit Reviews

              There are no open Gerrit changes