bug#55287: make-file-writable adds the executable bit on some files added to the store

[Raphaël Mélotte]

Hello,

Recently I tried to use Lynis provided by Guix again.

I was pretty sure it worked some time ago, but the current version refuses to 
start:
======
Fatal error: permissions of file 
/gnu/store/52yj60gjhzkrg10dq2xybfwx7g5x9z9w-lynis-3.0.6/share/lynis/db/languages/en
 are not strict enough. Access to 'owner' should be read-write, or read. Change 
with: chmod u=rw 
/gnu/store/52yj60gjhzkrg10dq2xybfwx7g5x9z9w-lynis-3.0.6/share/lynis/db/languages/en
======

The permissions on files in the "languages" folder in the store are surprising 
(I omitted some for clarity) :
======
...
-r--r--r-- 2 root root 4033 Jan  1  1970 da
-r-xr-xr-x 2 root root 4187 Jan  1  1970 de
lrwxrwxrwx 1 root root    2 Jan  1  1970 de-AT -> de
-r-xr-xr-x 2 root root 3865 Jan  1  1970 en
lrwxrwxrwx 1 root root    2 Jan  1  1970 en-GB -> en
lrwxrwxrwx 1 root root    2 Jan  1  1970 en-US -> en
-r--r--r-- 2 root root 4258 Jan  1  1970 es
-r--r--r-- 2 root root 4076 Jan  1  1970 fi
-r--r--r-- 2 root root 4210 Jan  1  1970 fr
...
======

Note for example that "da" is "0444", but "de" and "en" are "0555" (which is 
why Lynis refuses to start).

I wanted to know why this is the case, so I built Lynis from source using Guix.

Strangely, in the checkout the permissions look fine (no executable bit):
======
-r--r--r-- 1 root root 4033 jan  1  1970 da
-r--r--r-- 1 root root 4187 jan  1  1970 de
lrwxrwxrwx 1 root root    2 jan  1  1970 de-AT -> de
-r--r--r-- 1 root root 3865 jan  1  1970 en
lrwxrwxrwx 1 root root    2 jan  1  1970 en-GB -> en
lrwxrwxrwx 1 root root    2 jan  1  1970 en-US -> en
-r--r--r-- 1 root root 4258 jan  1  1970 es
-r--r--r-- 1 root root 4076 jan  1  1970 fi
-r--r--r-- 1 root root 4210 jan  1  1970 fr
======

Still, after they are added to the store, they end up with the executable bit 
set.

I then deleted most phases of the build one by one until I could find the 
culprit (spoiler: it was one of the firsts: unpack).

Indeed, after "unpack", the files end up with surprising permissions:
======
-rw-r--r-- 1 cabal cabal 4033 jan  1  1970 da
-rwxrwxrwx 1 cabal cabal 4187 jan  1  1970 de
lrwxrwxrwx 1 cabal cabal    2 jan  1  1970 de-AT -> de
-rwxrwxrwx 1 cabal cabal 3865 jan  1  1970 en
lrwxrwxrwx 1 cabal cabal    2 jan  1  1970 en-GB -> en
lrwxrwxrwx 1 cabal cabal    2 jan  1  1970 en-US -> en
-rw-r--r-- 1 cabal cabal 4258 jan  1  1970 es
-rw-r--r-- 1 cabal cabal 4076 jan  1  1970 fi
-rw-r--r-- 1 cabal cabal 4210 jan  1  1970 fr
======

Note how every file that is the target of a symlink is "0777", and the other regular 
files are "0644".

It turns out that we're doing this in the "unpack" phase of the 
gnu-build-system:
======
                ;; Make the source checkout files writable, for convenience.
                (for-each (lambda (f)
                                        (false-if-exception (make-file-writable 
f)))
                                  (find-files ".")))
======

So this explains the additional writable bit set on some of the file, but not 
where the executable bit comes from.

The answer is in make-file-writable from (guix build utils):
======
(define (make-file-writable file)
  "Make FILE writable for its owner."
  (let ((stat (lstat file)))                      ;XXX: symlinks
        (chmod file (logior #o600 (stat:perms stat)))))
======

Since it uses lstat to get the permissions of files, whenever a symlink is 
encountered the target of the symlink (because chmod dereferences the link) 
will have its permissions changed to the ones of the symlink (777).
Later when the file is copied to the store the writable bit is removed, so our target 
files end up with "0555" permissions.

This is problematic, as files that were originally not meant to be executable 
will be added to the store as executable.

I wonder if it ever makes sense to call "lstat" instead of "stat" in 
make-file-writable.
Since (AFAIK) symlinks always have "777" permissions and chmod anyway cannot 
change the permissions on the symlink itself, I can't think of a case where using lstat 
would be useful.
Am I missing one?

An alternative to changing "lstat" to "stat" would be to skip symlinks (either 
within make-file-writable or in the callers).

What do you think?

Related commits:
6129ebddbdd306ab60bb657d627db87686d76aa0
5a64a791317d98171435eff541a835ab0d3f498c

Related thread: https://issues.guix.gnu.org/43015

Kind regards,

Raphaël