[stgt] [PATCH] improved conversion and checking of numerical cmd line args

[Mark Harvey]

I've got a fix that I'm testing. I'll forward once I'm happy

Sent from my iPhone

On 20/06/2010, at 12:01, FUJITA Tomonori  
<fujita.tomonori at lab.ntt.co.jp> wrote:

> On Thu, 17 Jun 2010 16:19:43 +0300 (IDT)
> Alexander Nezhinsky <alexandern at voltaire.com> wrote:
>
>> When a numerical command line param value is out of range or  
>> contains garbage
>> (both tgtd and tgtadm), this remains unchecked in many cases.
>> In some of them 0 value will be used, which may lead to unpredicted  
>> or
>> "silently" faulty behavior.
>>
>> This fix defines a new function str_to_uit64() which uses strtoull 
>> () and
>> detects its errors properly, by checking 1) errno (range errors) and
>> 2) comparing endptr to the original argument (nun-numerical garbage);
>> must check both.
>>
>> str_to_uit64() is declared static twice, in tgtd.c and tgtadm.c, as
>> tgtadm does not uses util.c, where the natural place of this function
>> would be otherwise.
>>
>> It returns the converted integer value directly, while error is  
>> returned
>> through a pointer argument. Although the opposite arrangement may  
>> seem
>> intuitive, this one allows free casting of the returned value to  
>> various
>> types. Return type uint64 is choosen to cover all possible cases.
>> min and max arguments support range checking. To use the natural  
>> integer
>> ranges, use ULLONG_MAX, ULONG_MAX, USHRT_MAX etc.
>>
>> Signed-off-by: Alexander Nezhinsky <alexandern at voltaire.com>
>
> I'm for the strict checking however two comments.
>
> - casting a returned value looks ugly.
>
> - duplicating the same function isn't nice.
>
>
> How about something like this?
>
> diff --git a/usr/tgtadm.c b/usr/tgtadm.c
> index 28ed754..2282c86 100644
> --- a/usr/tgtadm.c
> +++ b/usr/tgtadm.c
> @@ -471,8 +471,8 @@ int main(int argc, char **argv)
>            mode = str_to_mode(optarg);
>            break;
>        case 't':
> -            tid = strtol(optarg, NULL, 10);
> -            if (errno == EINVAL) {
> +            tid = str_to_val(optarg, int, 0, INT_MAX, &rc);
> +            if (rc == EINVAL) {
>                eprintf("invalid tid '%s'\n", optarg);
>                exit(EINVAL);
>            }
> diff --git a/usr/util.h b/usr/util.h
> index 6e1fd6f..fc406b1 100644
> --- a/usr/util.h
> +++ b/usr/util.h
> @@ -156,4 +156,17 @@ struct signalfd_siginfo {
> };
> #endif
>
> +#define str_to_val(str, type, min, max, err_ptr)    \
> +({                            \
> +    char *ptr;                    \
> +    int ret = 0;                    \
> +    typeof(type) v = strtoull(str, &ptr, 0);    \
> +    if (errno || ptr == str)            \
> +        ret = EINVAL;                \
> +    if (v < min|| v > max)                \
> +        ret = ERANGE;                \
> +    *err_ptr = ret;                    \
> +    v;                        \
> +})                            \
> +
> #endif
> --
> To unsubscribe from this list: send the line "unsubscribe stgt" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html