[pacman-dev] [PATCH 3/3] paccache: verify nonnegative integer for -k option

newer
[pacman-dev] [PATCH 1/3] paccache:...

older
[pacman-dev] [PATCH 2/3] pkgdelta:...

lolilolicon

28 Sep 2011 28 Sep '11

4:58 p.m.

Allow leading zeros, but always treat it as decimal. Also quote right side of [[ expression ]] when the != or == operator is used, unless a pattern is intended. Signed-off-by: lolilolicon --- contrib/paccache.in | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/contrib/paccache.in b/contrib/paccache.in index 11b7bbb..c86ea31 100755 --- a/contrib/paccache.in +++ b/contrib/paccache.in @@ -153,7 +153,7 @@ summarize() { while read -r pkg; do if (( verbose >= 3 )); then [[ $pkg =~ $pkg_re ]] && name=${BASH_REMATCH[1]} arch=${BASH_REMATCH[2]} - if [[ -z $seen || $seenarch != $arch || $seen != $name ]]; then + if [[ -z $seen || $seenarch != "$arch" || $seen != "$name" ]]; then printf '%s (%s):\n' "$name" "$arch" fi printf ' %s\n' "$pkg" @@ -220,8 +220,10 @@ while getopts ':a:c:dfhi:k:m:rsuvz' opt; do blacklist+=("${ign[@]}") unset i ign ;; k) keep=$OPTARG - if [[ $keep != $OPTARG ]] || (( keep < 0 )); then + if [[ -z $keep || -n ${keep//[0-9]/} ]]; then die 'argument to option -k must be a non-negative integer' + else + keep=$(( 10#$keep )) fi ;; m) move=1 movedir=$OPTARG ;; r) delete=1 ;; -- 1.7.6.4

Show replies by date

Dave Reisner

28 Sep 28 Sep

5:52 p.m.

New subject: [pacman-dev] [PATCH 3/3] paccache: verify nonnegative integer for -k option

On Thu, Sep 29, 2011 at 12:58:59AM +0800, lolilolicon wrote:

...

Allow leading zeros, but always treat it as decimal.

If this was your goal, you missed. 'keep' is declared with an integer attribute meaning that all conversions to decimal are done at assignment, i.e. $ declare -i keep=015 # declare -p keep declare -i keep="13"

...

Also quote right side of [[ expression ]] when the != or == operator is used, unless a pattern is intended.

This really should be separate commits, or the first line should be a more accurate summation of the changes involved.

...

Signed-off-by: lolilolicon --- contrib/paccache.in | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/paccache.in b/contrib/paccache.in index 11b7bbb..c86ea31 100755 --- a/contrib/paccache.in +++ b/contrib/paccache.in @@ -153,7 +153,7 @@ summarize() { while read -r pkg; do if (( verbose >= 3 )); then [[ $pkg =~ $pkg_re ]] && name=${BASH_REMATCH[1]} arch=${BASH_REMATCH[2]} - if [[ -z $seen || $seenarch != $arch || $seen != $name ]]; then + if [[ -z $seen || $seenarch != "$arch" || $seen != "$name" ]]; then

I'm not saying this is wrong, but there'd have to be some seriously screwed up packages in your cache for this quoting to matter.

...

printf '%s (%s):\n' "$name" "$arch" fi printf ' %s\n' "$pkg" @@ -220,8 +220,10 @@ while getopts ':a:c:dfhi:k:m:rsuvz' opt; do blacklist+=("${ign[@]}") unset i ign ;; k) keep=$OPTARG - if [[ $keep != $OPTARG ]] || (( keep < 0 )); then + if [[ -z $keep || -n ${keep//[0-9]/} ]]; then die 'argument to option -k must be a non-negative integer' + else + keep=$(( 10#$keep )) fi ;; m) move=1 movedir=$OPTARG ;; r) delete=1 ;; -- 1.7.6.4

lolilolicon

6:46 p.m.

New subject: [pacman-dev] [PATCH 3/3] paccache: verify nonnegative integer for -k option

On Thu, Sep 29, 2011 at 1:52 AM, Dave Reisner wrote:

...

On Thu, Sep 29, 2011 at 12:58:59AM +0800, lolilolicon wrote:

...
Allow leading zeros, but always treat it as decimal.

If this was your goal, you missed. 'keep' is declared with an integer attribute meaning that all conversions to decimal are done at assignment, i.e.

$ declare -i keep=015 # declare -p keep declare -i keep="13"

Indeed, I missed 'keep' being declared '-i'. But then, the result is different from my intention, i.e. I meant to just strip the leading zeros (015 -> 15): $ typeset +i i $ i=015 $ declare -p i declare -- i="015" $ echo $(( 10#$i )) 15 'declare -i keep' might be problematic, if we just do 'keep=$OPTARG', since arithmetic evaluation is performed on the argument passed to -k, which means: - If the expression is not valid, the simple assignment 'keep=$OPTARG' will spill errors. - If the expression can be evaluated, it can potentially take a huge amount of resources, e.g. 2**2**32. Both situations should be avoided IMO. I say scratch '-i'; but YMMV...

...

...
Also quote right side of [[ expression ]] when the != or == operator is used, unless a pattern is intended.

This really should be separate commits, or the first line should be a more accurate summation of the changes involved.

You're right. This kind of got suppressed by the other issue.

...

...
Signed-off-by: lolilolicon --- contrib/paccache.in | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/paccache.in b/contrib/paccache.in index 11b7bbb..c86ea31 100755 --- a/contrib/paccache.in +++ b/contrib/paccache.in @@ -153,7 +153,7 @@ summarize() { while read -r pkg; do if (( verbose >= 3 )); then [[ $pkg =~ $pkg_re ]] && name=${BASH_REMATCH[1]} arch=${BASH_REMATCH[2]} - if [[ -z $seen || $seenarch != $arch || $seen != $name ]]; then + if [[ -z $seen || $seenarch != "$arch" || $seen != "$name" ]]; then

I'm not saying this is wrong, but there'd have to be some seriously screwed up packages in your cache for this quoting to matter.

In reality, sure, but nothing's stopping an $arch that is '[idk]686' in the future ;) After all, we should rely our code on data as little as possible, don't you think?

Dave Reisner

8:09 p.m.

New subject: [pacman-dev] [PATCH 3/3] paccache: verify nonnegative integer for -k option

On Thu, Sep 29, 2011 at 02:46:56AM +0800, lolilolicon wrote:

...

On Thu, Sep 29, 2011 at 1:52 AM, Dave Reisner wrote:

...
On Thu, Sep 29, 2011 at 12:58:59AM +0800, lolilolicon wrote:

...
Allow leading zeros, but always treat it as decimal.

If this was your goal, you missed. 'keep' is declared with an integer attribute meaning that all conversions to decimal are done at assignment, i.e.

$ declare -i keep=015 # declare -p keep declare -i keep="13"

Indeed, I missed 'keep' being declared '-i'. But then, the result is different from my intention, i.e. I meant to just strip the leading zeros (015 -> 15):

$ typeset +i i $ i=015 $ declare -p i declare -- i="015" $ echo $(( 10#$i )) 15

'declare -i keep' might be problematic, if we just do 'keep=$OPTARG', since arithmetic evaluation is performed on the argument passed to -k, which means:

- If the expression is not valid, the simple assignment 'keep=$OPTARG' will spill errors. - If the expression can be evaluated, it can potentially take a huge amount of resources, e.g. 2**2**32.

Both situations should be avoided IMO.

I say scratch '-i'; but YMMV...

You've convinced me. Write it up.

...

...
...
Also quote right side of [[ expression ]] when the != or == operator is used, unless a pattern is intended.

This really should be separate commits, or the first line should be a more accurate summation of the changes involved.

You're right. This kind of got suppressed by the other issue.

...
...
Signed-off-by: lolilolicon --- contrib/paccache.in | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/paccache.in b/contrib/paccache.in index 11b7bbb..c86ea31 100755 --- a/contrib/paccache.in +++ b/contrib/paccache.in @@ -153,7 +153,7 @@ summarize() { while read -r pkg; do if (( verbose >= 3 )); then [[ $pkg =~ $pkg_re ]] && name=${BASH_REMATCH[1]} arch=${BASH_REMATCH[2]} - if [[ -z $seen || $seenarch != $arch || $seen != $name ]]; then + if [[ -z $seen || $seenarch != "$arch" || $seen != "$name" ]]; then

I'm not saying this is wrong, but there'd have to be some seriously screwed up packages in your cache for this quoting to matter.

In reality, sure, but nothing's stopping an $arch that is '[idk]686' in the future ;) After all, we should rely our code on data as little as possible, don't you think?

4604

Age (days ago)

4604

Last active (days ago)

List overview

Download

3 comments

2 participants

participants (2)

Dave Reisner
lolilolicon