Thread: pg_ctl - tighten command parameter checking
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=:
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 himthe 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, Pennsylvania19026
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
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
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
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
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