pg_ctl - tighten command parameter checking

Oliver Elphick writes:

The attached patch improves the command parameter checking of pg_ctl.

At present, there is nothing to check that the parameter given with a
parameter-taking option is actually valid. For example, -l can be given
without a following logfile name; on a strict POSIX shell such as ash,
you will get a subsequent failure because of too many shifts, but bash
will let it pass without showing any error. The patch checks that each
parameter is not empty and is not another option.

Isn't this problem present in all of our scripts?

Btw., you shouldn't use "cut" in portable scripts. You could probably use
"case" to do the matching you want.

--
Peter Eisentraut peter_e@gmx.net

On Mon, 2002-02-18 at 16:35, Peter Eisentraut wrote:

Oliver Elphick writes:

The attached patch improves the command parameter checking of pg_ctl.

At present, there is nothing to check that the parameter given with a
parameter-taking option is actually valid. For example, -l can be given
without a following logfile name; on a strict POSIX shell such as ash,
you will get a subsequent failure because of too many shifts, but bash
will let it pass without showing any error. The patch checks that each
parameter is not empty and is not another option.

Isn't this problem present in all of our scripts?

Possibly, but this is the one where I had problems:-) I'll look at
others when I get some time.

Btw., you shouldn't use "cut" in portable scripts. You could probably use
"case" to do the matching you want.

What kind of an inadequate environment doesn't have cut?

OK. I'll redo it using case...esac.

NB. I saw a comment in this script about dirname's being non-portable.
But it uses basename. Is that portable?

--
Oliver Elphick Oliver.Elphick@lfix.co.uk
Isle of Wight http://www.lfix.co.uk/oliver
GPG: 1024D/3E1D0C1C: CA12 09E0 E8D5 8870 5839 932A 614D 4C34 3E1D 0C1C

"All we like sheep have gone astray; we have turned
every one to his own way; and the LORD hath laid on
him the iniquity of us all." Isaiah 53:6

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Oliver Elphick wrote:

The attached patch improves the command parameter checking of pg_ctl.

At present, there is nothing to check that the parameter given with a
parameter-taking option is actually valid. For example, -l can be given
without a following logfile name; on a strict POSIX shell such as ash,
you will get a subsequent failure because of too many shifts, but bash
will let it pass without showing any error. The patch checks that each
parameter is not empty and is not another option.

A consequence of this change is that no command-line parameter can begin
with "-" (except for the parameter to -o); this seems a reasonable
restriction.

For consistency and clarity, I have also changed every occurrence of
"shift ... var=$1" to "var=$2 ... shift".

--
Oliver Elphick Oliver.Elphick@lfix.co.uk
Isle of Wight http://www.lfix.co.uk/oliver
GPG: 1024D/3E1D0C1C: CA12 09E0 E8D5 8870 5839 932A 614D 4C34 3E1D 0C1C

"But as many as received him, to them gave he power to
become the sons of God, even to them that believe on
his name" John 1:12

[ text/x-patch is unsupported, treating like TEXT/PLAIN ]

*** postgresql-7.2.orig/src/bin/pg_ctl/pg_ctl.sh	Sat Sep 29 04:09:32 2001
--- postgresql-7.2/src/bin/pg_ctl/pg_ctl.sh	Sat Feb 16 10:50:36 2002
***************
*** 127,156 ****
exit 0
;;
-D)
- 	    shift
# pass environment into new postmaster
! 	    PGDATA="$1"
export PGDATA
;;
-l)
logfile="$2"
shift;;
-l*)
logfile=`echo "$1" | sed 's/^-l//'`
;;
-m)
shutdown_mode="$2"
shift;;
-m*)
shutdown_mode=`echo "$1" | sed 's/^-m//'`
;;
-o)
shift
- 	    POSTOPTS="$1"
;;
-p)
shift
- 	    po_path="$1"
;;
-s)
silence_echo=:
--- 127,197 ----
exit 0
;;
-D)
# pass environment into new postmaster
! 	    PGDATA="$2"
! 	    if [ -z "$PGDATA" -o `echo x$PGDATA | cut -c1-2` = "x-" ]
! 	    then
! 	    	echo "$CMDNAME: option '-D' specified without a data directory"
! 		exit 1
! 	    fi
export PGDATA
+ 	    shift
;;
-l)
logfile="$2"
+ 	    if [ -z "$logfile" -o `echo x$logfile | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-l' specified without a logfile"
+ 		exit 1
+ 	    fi
shift;;
-l*)
logfile=`echo "$1" | sed 's/^-l//'`
+ 	    if [ -z "$logfile" -o `echo x$logfile | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-l' specified without a logfile"
+ 		exit 1
+ 	    fi
;;
-m)
shutdown_mode="$2"
+ 	    if [ -z "$shutdown_mode" -o `echo x$shutdown_mode | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-m' specified without a shutdown mode"
+ 		exit 1
+ 	    fi
shift;;
-m*)
shutdown_mode=`echo "$1" | sed 's/^-m//'`
+ 	    if [ -z "$shutdown_mode" -o `echo x$shutdown_mode | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-m' specified without a shutdown mode"
+ 		exit 1
+ 	    fi
;;
-o)
+ 	    POSTOPTS="$2"
+ 	    if [ -z "$POSTOPTS" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-o' specified without any passed options"
+ 		exit 1
+ 	    fi
+ 	    if [ `echo x$POSTOPTS | cut -c1-2` != x- ]
+ 	    then
+ 	    	echo "$CMDNAME: option -o must be followed by one or more further options
+ to pass to the postmaster"
+ 		exit 1
+ 	fi
shift
;;
-p)
+ 	    po_path="$2"
+ 	    if [ -z "$po_path" -o `echo x$po_path | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-p' specified without a path"
+ 		exit 1
+ 	    fi
shift
;;
-s)
silence_echo=:

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Need to use case before patch application.

---------------------------------------------------------------------------

Peter Eisentraut wrote:

Oliver Elphick writes:

The attached patch improves the command parameter checking of pg_ctl.

At present, there is nothing to check that the parameter given with a
parameter-taking option is actually valid. For example, -l can be given
without a following logfile name; on a strict POSIX shell such as ash,
you will get a subsequent failure because of too many shifts, but bash
will let it pass without showing any error. The patch checks that each
parameter is not empty and is not another option.

Isn't this problem present in all of our scripts?

Btw., you shouldn't use "cut" in portable scripts. You could probably use
"case" to do the matching you want.

--
Peter Eisentraut peter_e@gmx.net

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Oliver, I am going to reject this. We give them the syntax for the
params. I don't see a need to check for leading dash to see if they
forgot a param. I would like to see a more general solution that uses
getopt or something more robust, but moving all that checking to each
param just seems like a waste.

---------------------------------------------------------------------------

Oliver Elphick wrote:

The attached patch improves the command parameter checking of pg_ctl.

At present, there is nothing to check that the parameter given with a
parameter-taking option is actually valid. For example, -l can be given
without a following logfile name; on a strict POSIX shell such as ash,
you will get a subsequent failure because of too many shifts, but bash
will let it pass without showing any error. The patch checks that each
parameter is not empty and is not another option.

A consequence of this change is that no command-line parameter can begin
with "-" (except for the parameter to -o); this seems a reasonable
restriction.

For consistency and clarity, I have also changed every occurrence of
"shift ... var=$1" to "var=$2 ... shift".

--
Oliver Elphick Oliver.Elphick@lfix.co.uk
Isle of Wight http://www.lfix.co.uk/oliver
GPG: 1024D/3E1D0C1C: CA12 09E0 E8D5 8870 5839 932A 614D 4C34 3E1D 0C1C

"But as many as received him, to them gave he power to
become the sons of God, even to them that believe on
his name" John 1:12

[ text/x-patch is unsupported, treating like TEXT/PLAIN ]

*** postgresql-7.2.orig/src/bin/pg_ctl/pg_ctl.sh	Sat Sep 29 04:09:32 2001
--- postgresql-7.2/src/bin/pg_ctl/pg_ctl.sh	Sat Feb 16 10:50:36 2002
***************
*** 127,156 ****
exit 0
;;
-D)
- 	    shift
# pass environment into new postmaster
! 	    PGDATA="$1"
export PGDATA
;;
-l)
logfile="$2"
shift;;
-l*)
logfile=`echo "$1" | sed 's/^-l//'`
;;
-m)
shutdown_mode="$2"
shift;;
-m*)
shutdown_mode=`echo "$1" | sed 's/^-m//'`
;;
-o)
shift
- 	    POSTOPTS="$1"
;;
-p)
shift
- 	    po_path="$1"
;;
-s)
silence_echo=:
--- 127,197 ----
exit 0
;;
-D)
# pass environment into new postmaster
! 	    PGDATA="$2"
! 	    if [ -z "$PGDATA" -o `echo x$PGDATA | cut -c1-2` = "x-" ]
! 	    then
! 	    	echo "$CMDNAME: option '-D' specified without a data directory"
! 		exit 1
! 	    fi
export PGDATA
+ 	    shift
;;
-l)
logfile="$2"
+ 	    if [ -z "$logfile" -o `echo x$logfile | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-l' specified without a logfile"
+ 		exit 1
+ 	    fi
shift;;
-l*)
logfile=`echo "$1" | sed 's/^-l//'`
+ 	    if [ -z "$logfile" -o `echo x$logfile | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-l' specified without a logfile"
+ 		exit 1
+ 	    fi
;;
-m)
shutdown_mode="$2"
+ 	    if [ -z "$shutdown_mode" -o `echo x$shutdown_mode | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-m' specified without a shutdown mode"
+ 		exit 1
+ 	    fi
shift;;
-m*)
shutdown_mode=`echo "$1" | sed 's/^-m//'`
+ 	    if [ -z "$shutdown_mode" -o `echo x$shutdown_mode | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-m' specified without a shutdown mode"
+ 		exit 1
+ 	    fi
;;
-o)
+ 	    POSTOPTS="$2"
+ 	    if [ -z "$POSTOPTS" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-o' specified without any passed options"
+ 		exit 1
+ 	    fi
+ 	    if [ `echo x$POSTOPTS | cut -c1-2` != x- ]
+ 	    then
+ 	    	echo "$CMDNAME: option -o must be followed by one or more further options
+ to pass to the postmaster"
+ 		exit 1
+ 	fi
shift
;;
-p)
+ 	    po_path="$2"
+ 	    if [ -z "$po_path" -o `echo x$po_path | cut -c1-2` = "x-" ]
+ 	    then
+ 	    	echo "$CMDNAME: option '-p' specified without a path"
+ 		exit 1
+ 	    fi
shift
;;
-s)
silence_echo=:

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Oliver Elphick wrote:

On Sat, 2002-02-23 at 21:31, Bruce Momjian wrote:

Oliver, I am going to reject this. We give them the syntax for the
params. I don't see a need to check for leading dash to see if they
forgot a param. I would like to see a more general solution that uses
getopt or something more robust, but moving all that checking to each
param just seems like a waste.

I would certainly prefer to use getopt, but is that portable? Peter
wants me to use case..esac instead of cut; I would have thought getopt
was a lot less portable.

No, it isn't. The problem is we don't have a portable solution _and_ we
don't want to throw checks all over the place. I realize this is a
non-solution, but I guess I don't consider is a big problem.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Oliver Elphick wrote:

On Sat, 2002-02-23 at 21:31, Bruce Momjian wrote:

Oliver, I am going to reject this. We give them the syntax for the
params. I don't see a need to check for leading dash to see if they
forgot a param. I would like to see a more general solution that uses
getopt or something more robust, but moving all that checking to each
param just seems like a waste.

I would certainly prefer to use getopt, but is that portable? Peter
wants me to use case..esac instead of cut; I would have thought getopt
was a lot less portable.

Added to TODO, at least:

* Add checks for missing parameters to shell script, to prevent
over-shifting

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

On Sat, 2002-02-23 at 21:31, Bruce Momjian wrote:

Oliver, I am going to reject this. We give them the syntax for the
params. I don't see a need to check for leading dash to see if they
forgot a param. I would like to see a more general solution that uses
getopt or something more robust, but moving all that checking to each
param just seems like a waste.

I would certainly prefer to use getopt, but is that portable? Peter
wants me to use case..esac instead of cut; I would have thought getopt
was a lot less portable.

--
Oliver Elphick Oliver.Elphick@lfix.co.uk
Isle of Wight http://www.lfix.co.uk/oliver
GPG: 1024D/3E1D0C1C: CA12 09E0 E8D5 8870 5839 932A 614D 4C34 3E1D 0C1C

"All scripture is given by inspiration of God, and is
profitable for doctrine, for reproof, for correction,
for instruction in righteousness; That the man of God
may be perfect, thoroughly furnished unto all good
works." II Timothy 3:16,17