linux-kernel - Re: [PATCHv3] drivers: isdn: get rid of custom strtoul()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <4B8BD190.1060609@imap.cc>
Date:	Mon, 01 Mar 2010 15:39:12 +0100
From:	Tilman Schmidt <tilman@...p.cc>
To:	Andy Shevchenko <andy.shevchenko@...il.com>
CC:	linux-kernel@...r.kernel.org, Hansjoerg Lipp <hjlipp@....de>
Subject: Re: [PATCHv3] drivers: isdn: get rid of custom strtoul()

Am 2010-02-26 08:26 schrieb Andy Shevchenko:
> There were two methods isdn_gethex() and isdn_getnum() which are custom
> implementations of strtoul(). Get rid of them in regard to
> strict_strtoul() kernel's function.
> 
> The patch should be applied on top of "[PATCH 3/4] gigaset: reduce syslog
> clutter" [1]

That patch is now in net-next, so that remark isn't needed anymore.

> --- a/drivers/isdn/gigaset/ev-layer.c
> +++ b/drivers/isdn/gigaset/ev-layer.c
[...]
> @@ -647,24 +601,28 @@ void gigaset_handle_modem_response(struct cardstate *cs)
>  		case RT_ZCAU:
>  			event->parameter = -1;
>  			if (curarg + 1 < params) {
> -				i = isdn_gethex(argv[curarg]);
> -				j = isdn_gethex(argv[curarg + 1]);
> -				if (i >= 0 && i < 256 && j >= 0 && j < 256)
> -					event->parameter = (unsigned) i << 8
> -							   | j;
> -				curarg += 2;
> +				unsigned long hi, lo;
> +				int rh, rl;
> +
> +				rh = strict_strtoul(argv[curarg++], 16, &hi);
> +				rl = strict_strtoul(argv[curarg++], 16, &lo);
> +
> +				if (rh == 0 && hi < 256 && rl == 0 && lo < 256)
> +					event->parameter = (hi << 8) | lo;

If you want to give the two decoded value variables speaking names (as
opposed to the generic "i" and "j") I'd prefer that they correspond to
their actual meaning (which of course you couldn't know). What you
called "hi" is the "cause type", and what you called "lo", the "cause
value". More appropriate variable names would therefore be, for example,
"cautype" and "cauvalue".

Of course that's only cosmetic, but it would still be nice if you could
take it into account if you're doing another version of the patch.

>  			} else
>  				curarg = params - 1;
>  			break;
>  		case RT_NUMBER:
>  		case RT_HEX:
>  			if (curarg < params) {
> -				if (param_type == RT_HEX)
> -					event->parameter =
> -						isdn_gethex(argv[curarg]);
> -				else
> -					event->parameter =
> -						isdn_getnum(argv[curarg]);
> +				int base = (param_type == RT_HEX) ? 16 : 10;
> +				unsigned long res;
> +				int rc;
> +
> +				rc = strict_strtoul(argv[curarg], base, &res);
> +				if (rc == 0)
> +					event->parameter = res;
> +

That's not quite correct. (Sorry for not catching that in the last
review.) The original code would set event->parameter to -1 if the
argv[curarg] string isn't valid. Your code will leave it unchanged
in that case.

As a remedy, you can just set event->parameter = -1 at the beginning
of the case, just as in the case RT_ZCAU above. That will also make
the else clause below unnecessary.

>  				++curarg;
>  			} else
>  				event->parameter = -1;

Thanks,
Tilman

-- 
Tilman Schmidt                    E-Mail: tilman@...p.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)

Download attachment "signature.asc" of type "application/pgp-signature" (260 bytes)