View Issue Details

IDProjectCategoryView StatusLast Update
0000060Cinelerra-GGBugpublic2019-02-01 00:16
Reporterferdnyc Assigned Togoodguy  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product Version2018-11 
Summary0000060: [PATCH] cinelerra-gg sets unnecessary executable bits during install to system locations
DescriptionWhen the cinelerra install process triggers the target defined as `sys_install` in cinelerra-5.1/Makefile.am, which installs files in a location outside of the source tree (as opposed to `bin_install` which simply copies files to "./bin/" in the tree), the cinelerra-5.1/inst.sh script is used to take the contents of entire source directory trees and reproduce them at the destination path.

It does this by traversing the source directory using recursive-descent, repeatedly calling the $(install_sh) script — GNU Autotool's standard installer, transferred to the source directory as `install-sh` — to recreate every directory and file found.

Because inst.sh calls install-sh with its default options, each regular source file will be given the default file mode 0755, meaning the executable bits will be SET for every file installed. This results in entire directories full of files being set executable in /usr/lib64/cinelerra-gg and /usr/share/cinelerra-gg, even though none of their contents is an executable binary. This is a major violation of software policy in many Linux distros', and completely prohibited under their packaging guidelines.
Additional InformationThe attached patches, made against the curent GIT head, address this issue in a twofold manner:

(1): First, the `inst.sh` script is modified to install regular files ($f) in destination $dir via `$install_sh -m 0644 -c "$f" "$dir"`, so that every file installed recursively in this manner will explicitly _not_ have its executable bits set. The install command for directories is unchanged, and will still default to the proper 0755 mode.

(2): In concert with this, the rules in cinelerra-5.1/Makefile.am are modified, and every executable binary which **does** need to have its executable bits set is explicitly installed with `$(install_sh)` directly, rather than relying on the $(inst_sh) mass-install script.

As a result of this change, the only files installed executable in our current cinelerra-gg package are:

$ rpmls -p ~/rpmbuild/RPMS/x86_64/cinelerra-gg-5.1-55.20181208git230f4fd.fc29.x86_64.rpm|egrep '^\-rwx'
-rwxr-xr-x /usr/bin/bdwrite
-rwxr-xr-x /usr/bin/cin_db
-rwxr-xr-x /usr/bin/cinelerra-gg
-rwxr-xr-x /usr/bin/zmpeg3cat
-rwxr-xr-x /usr/bin/zmpeg3cc2txt
-rwxr-xr-x /usr/bin/zmpeg3ifochk
-rwxr-xr-x /usr/bin/zmpeg3show
-rwxr-xr-x /usr/bin/zmpeg3toc
-rwxr-xr-x /usr/lib64/cinelerra-gg/cutads
-rwxr-xr-x /usr/lib64/cinelerra-gg/hveg2enc
-rwxr-xr-x /usr/lib64/cinelerra-gg/mpeg2enc
-rwxr-xr-x /usr/lib64/cinelerra-gg/mplex
-rwxr-xr-x /usr/lib64/cinelerra-gg/mplexlo

Prior to this change, the package contained 941 (!) regular files with the executable bit set:

$ rpmls -p ~/rpmbuild/RPMS/x86_64/cinelerra-gg-5.1-55.20181208git230f4fd.ferd29.x86_64.rpm|egrep '^\-rwx'|wc -l
941
TagsNo tags attached.
Attached Files
0001-inst.sh-Don-t-set-files-executable.patch (1,072 bytes)   
From 1f5dfe033fdc1f677b6ffca7d2a0fd208730a62d Mon Sep 17 00:00:00 2001
From: "FeRD (Frank Dana)" <ferdnyc@gmail.com>
Date: Wed, 12 Dec 2018 19:26:17 -0500
Subject: [PATCH] inst.sh: Don't set files executable

inst.sh was running install-sh on directory contents recursively,
without setting a file mode. Since its default is 0755, that left
far too many data files executable. Changed to pass `-m 0644`
when installing regular files. (Directories will still get 0755.)
---
 cinelerra-5.1/inst.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cinelerra-5.1/inst.sh b/cinelerra-5.1/inst.sh
index bbfee54..ec20eee 100755
--- a/cinelerra-5.1/inst.sh
+++ b/cinelerra-5.1/inst.sh
@@ -7,7 +7,7 @@ $mkinstalldirs "$dir"
 if [ "$*" = "*" ]; then exit; fi
 
 for f in "$@"; do
-  if [ -f "$f" ]; then $install_sh -c "$f" "$dir"; continue; fi
+  if [ -f "$f" ]; then $install_sh -m 0644 -c "$f" "$dir"; continue; fi
   if [ -d "$f" ]; then ( cd $f; $inst_sh "$dir/$f" * )
   else echo "*** Error - install $f in $dir failed." 1>&2; exit 1; fi
 done
-- 
2.19.2

0002-Makefile.am-Explicitly-install-mod-x-binaries.patch (2,426 bytes)   
From 49f78ccdf337d7b03f4e69e33ac0aa6a4a9d53f5 Mon Sep 17 00:00:00 2001
From: "FeRD (Frank Dana)" <ferdnyc@gmail.com>
Date: Thu, 13 Dec 2018 00:27:46 -0500
Subject: [PATCH] Makefile.am: Explicitly install mod +x binaries

With the default file mode for $(inst_sh) changed to 0644,
it's now necessary to explicitly install the files we want
to be executable.

This commit replaces $(inst_sh) for those files (only) with
individual calls to the default autotools $(install_sh) script.
Destination directory paths are first created, if necessary.
---
 cinelerra-5.1/Makefile.am | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/cinelerra-5.1/Makefile.am b/cinelerra-5.1/Makefile.am
index a61379c..e38ba2d 100644
--- a/cinelerra-5.1/Makefile.am
+++ b/cinelerra-5.1/Makefile.am
@@ -50,7 +50,10 @@ bin_uninstall:
 
 # with system_build
 sys_install: $(CIN_INSTALLS)
-	cd bin; $(inst_sh) "$(DESTDIR)$(bindir)" "$(WANT_CIN)" bdwrite
+	$(install_sh) -d "$(DESTDIR)$(bindir)"
+	cd bin; $(install_sh) -t "$(DESTDIR)$(bindir)" "$(WANT_CIN)";\
+			$(install_sh) -t "$(DESTDIR)$(bindir)" bdwrite
+
 	cd bin; $(inst_sh) "$(DESTDIR)$(datadir)/$(WANT_CIN)" \
 		COPYING README models Cinelerra_factory expanders.txt \
 		ffmpeg msg info tips doc
@@ -77,18 +80,26 @@ dvl_uninstall:
 
 # with-libzmpeg3
 zmp_install:
-	cd bin; $(inst_sh) "$(DESTDIR)$(bindir)" \
-		zmpeg3{cat,cc2txt,ifochk,show,toc}
-	cd bin; $(inst_sh) "$(DESTDIR)$(WANT_CINLIB_DIR)" \
-		hveg2enc mpeg2enc mplex mplexlo
+	$(install_sh) -d "$(DESTDIR)$(bindir)"
+	cd bin; for b in zmpeg3{cat,cc2txt,ifochk,show,toc}; do\
+		$(install_sh) -t "$(DESTDIR)$(bindir)" $$b;\
+	done
+
+	$(install_sh) -d "$(DESTDIR)$(WANT_CINLIB_DIR)"
+	cd bin; for b in hveg2enc mpeg2enc mplex mplexlo; do\
+		$(install_sh) -t "$(DESTDIR)$(WANT_CINLIB_DIR)" $$b;\
+	done
 
 zmp_uninstall:
 	rm -f "$(DESTDIR)$(bindir)"/zmpeg3{cat,cc2txt,ifochk,show,toc}
 
 # with-commercial
 com_install:
-	cd bin; $(inst_sh) "$(DESTDIR)$(bindir)" cin_db
-	cd bin; $(inst_sh) "$(DESTDIR)$(WANT_CINLIB_DIR)" cutads
+	$(install_sh) -d "$(DESTDIR)$(bindir)"
+	cd bin; $(install_sh) -t "$(DESTDIR)$(bindir)" cin_db
+
+	$(install_sh) -d "$(DESTDIR)$(WANT_CINLIB_DIR)"
+	cd bin; $(install_sh) -t "$(DESTDIR)$(WANT_CINLIB_DIR)" cutads
 
 com_uninstall:
 	rm -f "$(DESTDIR)$(bindir)/cin_db"
@@ -110,4 +121,3 @@ lv2_uninstall:
 
 val-%:
 	@echo $($(subst val-,,$@))
-
-- 
2.19.2

Activities

PhyllisSmith

2018-12-15 14:14

manager   ~0000202

For now, gg ended up implementing the simpler method of "simply copies files to "./bin/" in the tree".

ferdnyc

2018-12-15 17:54

reporter   ~0000212

Any method that solves the core issue works for me, I'm far less hung up on method than outcome. I'll prepare a new build off the latest commits and submit it for review, thanks!

ferdnyc

2018-12-17 06:38

reporter   ~0000243

Hi, I'm afraid the umask was inverted on this fix — in the current source, all regular files install with file mode 0000 (no permissions of any kind).

The issue is in inst.sh, which now contains:

for f in "$@"; do
  if [ -f "$f" ]; then ( umask 755; cp "$f" "$dir" ); continue; fi
  if [ -d "$f" ]; then ( cd $f; $inst_sh "$dir/$f" * )
  else echo "*** Error - inst.sh $f in $dir failed." 1>&2; exit 1; fi
done

Since umask is the mask of permission bits that will NOT be set, it should be the inverse of the
maximum desired permissions — the default is umask 022 (which would correspond to a mode of 755).

However, even using umask 022 to manage install permissions would result in all of the plugin files being installed with their executable bits set, as that's how they're output by the compiler. The mismatch between build process output and desired final state is why permissions are typically set explicitly by the install process, rather than relying on the source or build trees to have the correct permissions at the time of install.

PhyllisSmith

2018-12-18 02:51

manager   ~0000255

A fix for inst.sh has just now been checked into the GIT repository. Sorry about putting in the inverse.
Plugin binaries are shared object libraries and have to be executable.

PhyllisSmith

2019-02-01 00:16

manager   ~0000733

Appears to be OK now from our point of view. If further information becomes available, can re-open.

Issue History

Date Modified Username Field Change
2018-12-13 23:33 ferdnyc New Issue
2018-12-13 23:33 ferdnyc File Added: 0001-inst.sh-Don-t-set-files-executable.patch
2018-12-13 23:33 ferdnyc File Added: 0002-Makefile.am-Explicitly-install-mod-x-binaries.patch
2018-12-14 03:54 PhyllisSmith Assigned To => goodguy
2018-12-14 03:54 PhyllisSmith Status new => assigned
2018-12-15 14:14 PhyllisSmith Note Added: 0000202
2018-12-15 17:54 ferdnyc Note Added: 0000212
2018-12-17 06:38 ferdnyc Note Added: 0000243
2018-12-18 02:51 PhyllisSmith Note Added: 0000255
2019-02-01 00:16 PhyllisSmith Status assigned => closed
2019-02-01 00:16 PhyllisSmith Resolution open => fixed
2019-02-01 00:16 PhyllisSmith Note Added: 0000733