Thread: pg_ctl - tighten command parameter checking

pg_ctl - tighten command parameter checking

From
Oliver Elphick
Date:
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
*** 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=:

Re: pg_ctl - tighten command parameter checking

From
Peter Eisentraut
Date:
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



Re: pg_ctl - tighten command parameter checking

From
Oliver Elphick
Date:
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
himthe iniquity of us all."         Isaiah 53:6 
 



Re: pg_ctl - tighten command parameter checking

From
Bruce Momjian
Date:
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,
Pennsylvania19026
 


Re: pg_ctl - tighten command parameter checking

From
Bruce Momjian
Date:
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,
Pennsylvania19026
 


Re: pg_ctl - tighten command parameter checking

From
Bruce Momjian
Date:
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,
Pennsylvania19026
 


Re: pg_ctl - tighten command parameter checking

From
Oliver Elphick
Date:
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 
 



Re: pg_ctl - tighten command parameter checking

From
Bruce Momjian
Date:
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,
Pennsylvania19026
 


Re: pg_ctl - tighten command parameter checking

From
Bruce Momjian
Date:
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,
Pennsylvania19026