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

[Dave Reisner]

On Thu, Sep 29, 2011 at 02:46:56AM +0800, lolilolicon wrote:
> On Thu, Sep 29, 2011 at 1:52 AM, Dave Reisner <d at falconindy.com> 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 <lolilolicon at gmail.com>
> >> ---
> >>  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?
>