fixfiles: broken option processing?

fixfiles: broken option processing?

[Václav Ovsík]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

Hi,
I'm fighting with current fixfiles script. This script is called on
Debian from init scripts (/etc/init.d/selinux-basics) when
autorelabeling is requested in the following shape:

    /sbin/fixfiles -f -F relabel > /dev/null || true

With current version r2760, this never succeed at least on Debian. It
seems to me, that getopts is used with shifting arguments improperly.
Help for getopts says, that successive calls to getopts sets OPTIND on
next option. When arguments are shifted without reasigning OPTIND to 1,
bad argument is processed.

Please consider my patch. I distiled this code from some autoconf
output. This supports long options and leaves only non option arguments.
No bashism. Code needs review. I'm not certain how effective code should
be for very long argument list.

If this patch is not usable, original code can be fixed by removing
shift commands from processing or reasigning 1 to OPTIND, but man page
should be fixed, so that options must precede command.

Cheers.
-- 
Zito

[-- Attachment #2: fixfiles.optproc.patch --]
[-- Type: text/x-diff, Size: 2325 bytes --]

Index: fixfiles
===================================================================
--- fixfiles	(revision 2790)
+++ fixfiles	(working copy)
@@ -20,6 +20,8 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
+progname=$(basename $0)
+
 #
 # Set global Variables
 #
@@ -186,60 +188,61 @@
 esac
 }
 usage() {
-      	echo $"Usage: $0 [-l logfile ] [-o outputfile ] { check | restore|[-F] relabel } [[dir] ... ] "
+      	echo $"Usage: $progname [-l logfile ] [-o outputfile ] { check | restore|[-F] relabel } [[dir] ... ] "
 	echo or
-      	echo $"Usage: $0 -R rpmpackage[,rpmpackage...] -C PREVIOUS_FILECONTEXT [-l logfile ] [-o outputfile ] { check | restore }"
+      	echo $"Usage: $progname -R rpmpackage[,rpmpackage...] -C PREVIOUS_FILECONTEXT [-l logfile ] [-o outputfile ] { check | restore }"
 }
 
-if [ $# = 0 ]; then
-	usage
-	exit 1
-fi
 
 # See how we were called.
-while getopts "C:Ffo:R:l:" i; do
-    case "$i" in
-	f)
-		fullFlag=1
-		shift 1
-		;;
-        R)
-		RPMFILES=$OPTARG
-		shift 2
-		;;
-        o)
-		OUTFILES=$OPTARG
-		shift 2
-		;;
-        l)
-		LOGFILE=$OPTARG
-		shift 2
-		;;
-        C)
-		PREFC=$OPTARG
-		shift 2
-		;;
-	F)
-		FORCEFLAG="-F"
-		shift 1
-		;;
-	*)
-	    usage
-	    exit 1
-esac
+opt_prev=
+i=0
+for arg
+do
+    let i=i+1
+    if test -n "$opt_prev"
+    then
+        eval "$opt_prev=\$arg"
+        opt_prev=
+        continue
+    fi
+    case $arg in
+	-f|--full) fullFlag=1 ;;
+	-R|--rpm-files) opt_prev=RPMFILES ;;
+	-o|--out-file) opt_prev=OUTFILES ;;
+	-l|--log-file) opt_prev=LOGFILE ;;
+	-C|--prev-fc) opt_prev=PREFC ;;
+	-F|--force) FORCEFLAG="-F" ;;
+        -h|--help) usage; exit 0 ;;
+        -*)
+            echo "$progname: error: unrecognized option: $arg
+Try \`$progname --help' for more information." >&2
+            exit 1
+            ;;
+        *) noa="$noa \${$i}" ;;
+    esac
 done
 
+eval set -- $noa
+
+if test $# -lt 1
+then
+    usage
+    exit 1
+fi
+
 # Check for the command
-command=$1
-if [ -z $command ]; then
+command="$1"
+if [ -z "$command" ]; then
     usage
+    exit 1
 fi
+shift 1
 
 #
 # check if they specified both DIRS and RPMFILES
 #
 
-shift 1
 if [ ! -z "$RPMFILES" ]; then
     process $command
     if [ $# -gt 0 ]; then

Re: fixfiles: broken option processing?

[Stephen Smalley]

On Wed, 2008-02-06 at 16:49 +0100, Václav Ovsík wrote:
> Hi,
> I'm fighting with current fixfiles script. This script is called on
> Debian from init scripts (/etc/init.d/selinux-basics) when
> autorelabeling is requested in the following shape:
> 
>     /sbin/fixfiles -f -F relabel > /dev/null || true
> 
> With current version r2760, this never succeed at least on Debian. It
> seems to me, that getopts is used with shifting arguments improperly.
> Help for getopts says, that successive calls to getopts sets OPTIND on
> next option. When arguments are shifted without reasigning OPTIND to 1,
> bad argument is processed.
> 
> Please consider my patch. I distiled this code from some autoconf
> output. This supports long options and leaves only non option arguments.
> No bashism. Code needs review. I'm not certain how effective code should
> be for very long argument list.
> 
> If this patch is not usable, original code can be fixed by removing
> shift commands from processing or reasigning 1 to OPTIND, but man page
> should be fixed, so that options must precede command.

Hmm..the shifting of arguments was introduced in r2699 from Dan Walsh
(cc'd), diff is below.  Also relevant are r2736, r2750 and r2760.

I'd agree that we don't want to break existing usage, but I'm not sure
what the least intrusive and cleanest fix is.

$ svn diff -c2699 fixfiles
Index: fixfiles
===================================================================
--- fixfiles	(revision 2698)
+++ fixfiles	(revision 2699)
@@ -92,7 +92,7 @@
 		      ! \( -fstype ext2 -o -fstype ext3 -o -fstype jfs -o -fstype xfs \) -prune  -o \
 		      \( -wholename /home -o -wholename /root -o -wholename /tmp -wholename /dev \) -prune -o -print; \
 		      done 2> /dev/null | \
-	 ${RESTORECON} $2 -v -f - 
+	 ${RESTORECON} $2 -f - 
 	rm -f ${TEMPFILE} ${PREFCTEMPFILE}
 fi
 }
@@ -189,21 +189,27 @@
     case "$i" in
 	f)
 		fullFlag=1
+		shift 1
 		;;
         R)
 		RPMFILES=$OPTARG
+		shift 2
 		;;
         o)
 		OUTFILES=$OPTARG
+		shift 2
 		;;
         l)
 		LOGFILE=$OPTARG
+		shift 2
 		;;
         C)
 		PREFC=$OPTARG
+		shift 2
 		;;
 	F)
 		FORCEFLAG="-F"
+		shift 1
 		;;
 	*)
 	    usage
@@ -211,10 +217,8 @@
 esac
 done
 
-
 # Check for the command
-eval command=\$${OPTIND}
-let OPTIND=$OPTIND+1
+command=$1
 if [ -z $command ]; then
     usage
 fi
@@ -223,17 +227,15 @@
 # check if they specified both DIRS and RPMFILES
 #
 
+shift 1
 if [ ! -z "$RPMFILES" ]; then
-    if [ $OPTIND -le $# ]; then
+    if [ $# -gt 0 ]; then
 	    usage
     fi
 else
-    while [ $OPTIND -le $# ]; do
-	eval DIR=\$${OPTIND}
-	DIRS="$DIRS $DIR"
-	let OPTIND=$OPTIND+1
-    done
+    DIRS=$*
 fi
+
 #
 # Make sure they specified one of the three valid commands
 #

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: fixfiles: broken option processing?

[Václav Ovsík]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

On Thu, Feb 07, 2008 at 04:28:03PM -0500, Stephen Smalley wrote:
> Hmm..the shifting of arguments was introduced in r2699 from Dan Walsh
> (cc'd), diff is below.  Also relevant are r2736, r2750 and r2760.
> 
> I'd agree that we don't want to break existing usage, but I'm not sure
> what the least intrusive and cleanest fix is.

Ok. What about this patch :)

The interpreter is changed to /bin/bash.  There are bashisms (getopts,
PIPESTATUS...), so this can't be interpreted by /bin/sh.

Regards
-- 
Zito

[-- Attachment #2: fixfiles.optproc2.patch --]
[-- Type: text/x-diff, Size: 945 bytes --]

Index: fixfiles
===================================================================
--- fixfiles	(revision 2793)
+++ fixfiles	(working copy)
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 # fixfiles
 #
 # Script to restore labels on a SELinux box
@@ -201,27 +201,21 @@
     case "$i" in
 	f)
 		fullFlag=1
-		shift 1
 		;;
         R)
 		RPMFILES=$OPTARG
-		shift 2
 		;;
         o)
 		OUTFILES=$OPTARG
-		shift 2
 		;;
         l)
 		LOGFILE=$OPTARG
-		shift 2
 		;;
         C)
 		PREFC=$OPTARG
-		shift 2
 		;;
 	F)
 		FORCEFLAG="-F"
-		shift 1
 		;;
 	*)
 	    usage
@@ -229,17 +223,22 @@
 esac
 done
 
+# Move out processed options from arguments
+shift $(( OPTIND - 1 ))
+
 # Check for the command
 command=$1
 if [ -z $command ]; then
     usage
 fi
 
+# Move out command from arguments
+shift
+
 #
 # check if they specified both DIRS and RPMFILES
 #
 
-shift 1
 if [ ! -z "$RPMFILES" ]; then
     process $command
     if [ $# -gt 0 ]; then

Re: fixfiles: broken option processing?

[Václav Ovsík]

On Fri, Feb 08, 2008 at 09:29:22AM +0100, Václav Ovsík wrote:
> The interpreter is changed to /bin/bash.  There are bashisms (getopts,
> PIPESTATUS...), so this can't be interpreted by /bin/sh.

No, getopts is ok (as dash(1) says). The problem is PIPESTATUS at least.

# etch:/home/zito# dash -x fixfiles
+ fullFlag=0
+ FORCEFLAG=
...
+ SELINUXTYPE=refpolicy
+ SETLOCALDEFS=0
+ FC=/etc/selinux/refpolicy/contexts/files/file_contexts
fixfiles: 109: Syntax error: Bad substitution

This code:

rpmlist() {
rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' '
[ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr
}

We should use /bin/bash.
Regards
-- 
Zito

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: fixfiles: broken option processing?

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Václav Ovsík wrote:
> On Thu, Feb 07, 2008 at 04:28:03PM -0500, Stephen Smalley wrote:
>> Hmm..the shifting of arguments was introduced in r2699 from Dan Walsh
>> (cc'd), diff is below.  Also relevant are r2736, r2750 and r2760.
>>
>> I'd agree that we don't want to break existing usage, but I'm not sure
>> what the least intrusive and cleanest fix is.
> 
> Ok. What about this patch :)
> 
> The interpreter is changed to /bin/bash.  There are bashisms (getopts,
> PIPESTATUS...), so this can't be interpreted by /bin/sh.
> 
> Regards
> 
This looks good to me.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkesV/IACgkQrlYvE4MpobMndQCfRC9x2SUT/wwZmIAX2JlubED/
3LAAniOtNSEEldpmXGX/2vubL27yvezh
=1tZw
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: fixfiles: broken option processing?

[Stephen Smalley]

On Fri, 2008-02-08 at 09:29 +0100, Václav Ovsík wrote:
> On Thu, Feb 07, 2008 at 04:28:03PM -0500, Stephen Smalley wrote:
> > Hmm..the shifting of arguments was introduced in r2699 from Dan Walsh
> > (cc'd), diff is below.  Also relevant are r2736, r2750 and r2760.
> > 
> > I'd agree that we don't want to break existing usage, but I'm not sure
> > what the least intrusive and cleanest fix is.
> 
> Ok. What about this patch :)
> 
> The interpreter is changed to /bin/bash.  There are bashisms (getopts,
> PIPESTATUS...), so this can't be interpreted by /bin/sh.

Thanks, merged.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.