[ClusterLabs] [RFC at agents] unresolved issue with handling parameters containing whitespaces

Dejan Muhamedagic dejanmm at fastmail.fm
Fri May 15 13:54:40 EDT 2015


Hi,

On Wed, May 13, 2015 at 11:35:01AM +0300, Vladislav Bogdanov wrote:
> Hi
> 
> what you see is a bug in mysql-common.sh
> It uses shell tests on unquoted variables:
> 
>     if [ ! -f $OCF_RESKEY_config ]; then
>         ocf_exit_reason "Config $OCF_RESKEY_config doesn't exist";
>         return $OCF_ERR_INSTALLED;
>     fi
> 
>     if [ ! -d $OCF_RESKEY_datadir ]; then
>         ocf_exit_reason "Datadir $OCF_RESKEY_datadir doesn't exist";
>         return $OCF_ERR_INSTALLED;
>     fi
> 
> That should look like
> 
>     if [ ! -f "$OCF_RESKEY_config" ]; then
>         ocf_exit_reason "Config $OCF_RESKEY_config doesn't exist";
>         return $OCF_ERR_INSTALLED;
>     fi
> 
>     if [ ! -d "$OCF_RESKEY_datadir" ]; then
>         ocf_exit_reason "Datadir $OCF_RESKEY_datadir doesn't exist";
>         return $OCF_ERR_INSTALLED;
>     fi
> 
> May be the same should be done in other places too.
> It is the safest position to quote variables every time you use them
> unless you know for sure that in the specific case var should be
> used unquoted.

Quite right. And in particular when it's user's input. If
somebody opens an "issue" at github.com, there's some chance of
this being fixed.

I wonder if there's a lint for shells...

Cheers,

Dejan

> Best,
> Vladislav
> 
> 
> 12.05.2015 16:03, Jan Pokorný wrote:
> 
> >Aloha,
> >
> >recently I realized one of those rough edges that probably were never
> >run into (per quick ML seach) but are lurking to bite anyone that will
> >leave the safe, well-tested paths.
> >
> >What I have in mind in particular is unability to propagate parameters
> >participating directly in underlying resource execution as command's
> >arguments[*] (referred to as "argv items" for clarity) when they contain
> >(white)spaces (per IFS variable?) due to (intentionally?) preserving them
> >as-are in the majority of shell scripts constituting resource agents
> >(will leave fence agents for a follow-up now):
> >
> >1. no wrapping them with quotes just for the sake of protecting against
> >    word-splitting/preserving a _single_ argv item (eventually handled
> >    in unquoted form, see the next point) to cover cases with space
> >    contained within
> >
> >2. no unwrapping in case the user-provided parameter already is wrapped
> >    with {single,double} quotes so as to denote "this is to be treated
> >    as a single argv item" explicitly from user's input (somewhat
> >    intuitive workaround attempt if one is familiar with shell and/or
> >    quoting rules in some configuration files)
> >
> >[*] not exclusively, but these parameters are most exposed to the problem
> >     at hand
> >
> >
> >To demonstrate the problem in a real-world scenario:
> >
> >- I want to run ocf:heartbeat:mysql resource in my cluster
> >
> >- I am a rebel going against best practices, hence my configuration
> >   file for this mysql instance is "/mnt/foo/mysql/config final.cnf"
> >   (notice the space)
> >
> >- so I go ahead and type:
> >
> >   # pcs resource create mysql-test1 ocf:heartbeat:mysql \
> >     'config=/mnt/foo/mysql/config final.cnf'
> >   (quotes present so as to pass the whole config=... argument at once,
> >   otherwise the same word-splitting problem as mentioned would be
> >   present)
> >
> >   # pcs resource debug-start --full mysql-test1 \
> >     | grep /mnt/foo/mysql/config
> >   >  stderr: ++ 06:47:57: 52: : /mnt/foo/mysql/config final.cnf
> >   >  stderr: + 06:47:57: mysql_common_validate:108: \
> >   >    '[' '!' -f /mnt/foo/mysql/config final.cnf ']'
> >                                       ^^^
> >                                      OOPS#1a
> >   >  stderr: /usr/lib/ocf/lib/heartbeat/mysql-common.sh: line 108: \
> >   >    [: /mnt/foo/mysql/config: binary operator expected
> >                                   ^^^
> >                                  OOPS#1b
> >   >  stderr: + 06:47:58: mysql_common_start:209: \
> >   >    /usr/bin/mysqld_safe \
> >   >    --defaults-file=/mnt/foo/mysql/config final.cnf \
> >                                            ^^^
> >                                            OOPS#2a
> >   >    --pid-file=/var/run/mysql/mysqld.pid \
> >   >    --socket=/var/lib/mysql/mysql.sock --datadir=/var/lib/mysql
> >   >    --log-error=/var/log/mysqld.log --user=mysql
> >
> >   # grep '/mnt/foo/mysql/config' /var/log/mysqld.log
> >   Could not open required defaults file: /mnt/foo/mysql/config
> >                                                          ^^^
> >                                                         OOPS#2b
> >
> >  - well, no good, let's try a workaround sketched in point 2. above:
> >
> >   # pcs resource create mysql-test2 ocf:heartbeat:mysql \
> >     'config="/mnt/foo/mysql/config final.cnf"'
> >     (outer singlequotes so as to preserve inner doublequotes)
> >
> >   # pcs resource debug-start --full mysql-test2 \
> >     | grep '/mnt/foo/mysql/config'
> >  >  stderr: ++ 07:06:44: 52: : '"/mnt/foo/mysql/config' 'final.cnf"'
> >  >  stderr: + 07:06:44: mysql_common_validate:108: \
> >       '[' '!' -f '"/mnt/foo/mysql/config' 'final.cnf"' ']'
> >                                       ^^^
> >                                      OOPS#1a
> >  >  stderr: /usr/lib/ocf/lib/heartbeat/mysql-common.sh: line 108: \
> >       [: "/mnt/foo/mysql/config: binary operator expected
> >                                   ^^^
> >                                  OOPS#1b ???
> >  >  stderr: + 07:06:45: mysql_common_start:209: /usr/bin/mysqld_safe \
> >       '--defaults-file="/mnt/foo/mysql/config' 'final.cnf"' \
> >                                            ^^^
> >                                            OOPS#2a
> >       --pid-file=/var/run/mysql/mysqld.pid \
> >       --socket=/var/lib/mysql/mysql.sock --datadir=/var/lib/mysql \
> >       --log-error=/var/log/mysqld.log --user=mysql
> >
> >     # grep '/mnt/foo/mysql/config' /var/log/mysqld.log
> >     Could not open required defaults file: /root/"/mnt/foo/mysql/config
> >                                                          ^^^
> >                                                         OOPS#2b
> >
> >
> >Possible solutions:
> >
> >A. claim whitespace as invalid character in agents' parameters
> >    o seems pretty dull solution
> >      - we shouldn't limit the domain that hasn't been limited like that
> >        from the beginning
> >
> >B. smart quoting logic per 1. on the Pacemaker side to accommodate for
> >    shell  scripts deficiencies causing the stated issues
> >    (like whitespace as a proper radix of parameter -> add quotes)
> >    o non-systemic
> >      - no guarantee all shell-based agents are broken in the same way
> >      - would almost certainly break non-shell-based agents
> >
> >C. fix the shell-based resource agents
> >
> >    o C0. use doublequotes whenever suitable (like the tests denoted with
> >          OOPS#1 above) in the shell code
> >
> >    o C1. (optionally) smart quoting logic per 1. directly in the agents
> >      - otherwise, a convention would have to be introduced that user
> >        has to quote the parameters on her own when configuring the
> >        cluster -- suboptimal as there would be artificial distinction
> >        betweeb shell-based RAs and the rest (if any)
> >
> >    o C2. instead of invoking the underlying commands:
> >            $exec $param1 $param2
> >          use:
> >            eval $exec $param1 $param2
> >          to ensure that the quoting from C1. will be taken into account
> >          properly and not passed as part of argv items
> >      - together with C1., this would fix OOPS#2
> >      - possibly, there could also be OCF helper's level function to unify
> >        the approach
> >
> >
> >What the community thinks?  I hope this is not too crazy :-)
> >Let's get rid of these remaining rough edges.
> >
> >
> >Some related bugs, these are against pcs but other management tools may
> >be similarly affected (or be using [influential] approaches already):
> >- https://bugzilla.redhat.com/show_bug.cgi?id=1219574
> >   [gui] resource optional arguments: quoted strings missing
> >- https://bugzilla.redhat.com/show_bug.cgi?id=1220778
> >   [RFC] should pcs escape double-quotes to prevent clashes/irregularity with
> >   string-denoting ones in parameter printouts?
> >
> >
> >P.S. OCF list CC'd only for posterity and to remind its audience that the
> >      other lists (developers and users at ClusterLabs) have become
> >      preferred venues to discuss these things (this in particular
> >      is a better fit for the former, admittedly).
> >
> >
> >
> >_______________________________________________
> >Users mailing list: Users at clusterlabs.org
> >http://clusterlabs.org/mailman/listinfo/users
> >
> >Project Home: http://www.clusterlabs.org
> >Getting started: http://www.clusterlabs.org/doc/Cluster_from_Scratch.pdf
> >Bugs: http://bugs.clusterlabs.org
> >
> 
> 
> _______________________________________________
> Users mailing list: Users at clusterlabs.org
> http://clusterlabs.org/mailman/listinfo/users
> 
> Project Home: http://www.clusterlabs.org
> Getting started: http://www.clusterlabs.org/doc/Cluster_from_Scratch.pdf
> Bugs: http://bugs.clusterlabs.org




More information about the Users mailing list