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

[lolilolicon]

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...

>> 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?