0

I am new to bash scripting have patience.
I have this code where it scans ports of domains and shows them to the user and it works perfectly by it self but when i added functions to it gives me errors. What am I doing wrong here?

check_parms () {
case $# in
    1) ports='1-1023'
       host=$1 ;;
    2) ports=$1
       host=$2 ;;
    *) echo 'Usage: portscan [port|range] host'
       exit 1 ;;
esac
}

# check port range
check_ports () {
if [ "$(echo $ports | grep '^[1-9][0-9]*-[1-9][0-9]*$')" != "" ]; then
    firstport=$(echo $ports | cut -d- -f1)
    lastport=$(echo $ports | cut -d- -f2)
elif [ "$(echo $ports | grep '^[1-9][0-9]*$')" != "" ]; then
    firstport=$ports
    lastport=$ports
else
    echo "$ports is an invalid port(s) value"
    exit 2
fi
}

# check firstport > lastport
check_order () {
if [ $firstport -gt $lastport ]; then
    echo $firstport is larger than $lastport
    exit 3
fi
}

# check host value
check_host () {
regex='^([1-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])(\.([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])){3}$'
if [ "$(echo $host | grep '[A-Za-z]')" != "" ]; then
    response=$(host $host)
    if [[ "$response" =~ "timed out" || "$response" =~ "not found" ]]; then
        echo host $host not found
        exit 4
    fi
elif [[ ! $host =~ $regex ]]; then
    echo $host is an invalid host address
    exit 5
fi
}

# check if host is reachable using ping
check_ping () {
if [[ "$(ping -c 1 -W 2 -n $host)" =~ "0 received" ]]; then
    echo $host is unreachable
    exit 6
fi
}

# start the scan     
do_scan () {
echo -n "Scanning "
for p in $(seq $firstport $lastport)
do
    echo -n .
    x=$((echo >/dev/tcp/$host/$p) >/dev/null 2>&1 && echo "$p open")
    if [ "$x" != "" ]; then  
        y="${y}
$x"
    fi
done
}

# show results of scan
show_results () {
echo -e "\n$y\n"
exit 0
}

check_parms $#
check_ports $ports
check_order $firstport $lastport
check_host $host
check_ping $host
do_scan $host $firstport $lastport
show_results
4
  • 4
    The place to start when putting together a good question is isolating your problem, so you can show only the smallest piece of code necessary to show the issue you're asking about. Using set -x to trace each command run and see where things first go wrong is a good place to start with that. See also sscce.org and stackoverflow.com/help/mcve Nov 21, 2014 at 22:28
  • 4
    I'd also suggest putting your code through shellcheck.net and fixing the issues it finds. Nov 21, 2014 at 22:30
  • 3
    In addition to the above advice, it looks like you had a working script, did one "big" rewrite of it, and discovered it didn't work ... In general, you want to keep a tight feedback loop, that means, change one (or just a few things), then immediately test, fix if something works, and continue.... That way it's usually much clearer where the error is. Nov 21, 2014 at 22:34
  • For consistency with other networking tools, it would be better to switch the argument order: portscan host port. This would also coincidentally simplify the code, and also it is then trivial to adapt it to accept multiple discrete ports or ranges.
    – tripleee
    Nov 22, 2014 at 7:53

1 Answer 1

2

The way you're passing and using function arguments doesn't make sense. The one I see that's blatantly wrong is the first:

check_parms () {
case $# in
    1) ports='1-1023'
       host=$1 ;;
    2) ports=$1
       host=$2 ;;
    *) echo 'Usage: portscan [port|range] host'
       exit 1 ;;
esac
}

#... then in the main program:
check_parms $#

To understand the problem, suppose the main script is run with two arguments: "5-50" and "10.10.10.10". The line check_parms $# runs check_params and passes the number of arguments (2) as an argument to it. Fine so far. Then in check_params, it runs case $# in, but at this point we're inside check_params, so $# refers to the number of arguments passed to check_params, not the number passed to the main script. check_params was passed only one parameter (the number 2), so $# is 1, and it executes the 1) case, which sets ports to "1-1023" and host to "2". This is not what you wanted at all.

If you want check_params to be able to see the same parameters that were passed to the main script, you have to pass it those same parameters with check_params "$@".

The arguments passed to the other functions simply don't appear to be used; this won't necessarily cause trouble, but it makes for confusing code (and confusing code breeds bugs). Consider, for example the check_order function. You pass it $firstport and $lastport as arguments (meaning that within check_order, $1 is set to the value of $firstport and $2 is set to the value of $lastport), but then you don't use those arguments inside check_order; instead you use $firstport and $lastport directly, ignoring the parameters. Since they're the same thing, it doesn't really matter, but if someone later tried to use it for something else (check_order $currentport 1023) it would check $firstport and $lastport rather than what it looks like it should be checking.

Solution: change the functions to use their arguments rather than accessing global variables directly. (Or use the global variables explicitly and stop pretending to pass them as arguments; but globals are generally bad, so this is the worse option.) Something like this:

check_order () {
if [ $1 -gt $2 ]; then
    echo $1 is larger than $2
    exit 3
fi
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.