linux-kernel - Re: [PATCH 26/45] kstrtox: convert drivers/isdn/

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 07 Dec 2010 11:06:27 +0100
From:	Tilman Schmidt <t.schmidt@...enixsoftware.de>
To:	Alexey Dobriyan <adobriyan@...il.com>
CC:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 26/45] kstrtox: convert drivers/isdn/

Am 2010-12-06 21:10 schrieb Alexey Dobriyan:
> On Mon, Dec 06, 2010 at 01:10:45AM +0100, Tilman Schmidt wrote:
>> I like the patch, but why not go all the way?
>>
>> Am 05.12.2010 18:49 schrieb Alexey Dobriyan:
>>> @@ -566,10 +566,10 @@ void gigaset_handle_modem_response(struct cardstate *cs)
>>>  		case RT_ZCAU:
>>>  			event->parameter = -1;
>>>  			if (curarg + 1 < params) {
>>> -				unsigned long type, value;
>>> +				u8 type, value;
>>>  
>>> -				i = strict_strtoul(argv[curarg++], 16, &type);
>>> -				j = strict_strtoul(argv[curarg++], 16, &value);
>>> +				i = kstrtou8(argv[curarg++], 16, &type);
>>> +				j = kstrtou8(argv[curarg++], 16, &value);
>>>  
>>>  				if (i == 0 && type < 256 &&
>>>  				    j == 0 && value < 256)
>>
>> Once type and value are u8, the checks for < 256 are unnecessary.
> 
> OK.
> 
>>> @@ -583,7 +583,7 @@ void gigaset_handle_modem_response(struct cardstate *cs)
>>>  				unsigned long res;
>>>  				int rc;
>>>  
>>> -				rc = strict_strtoul(argv[curarg++], 10, &res);
>>> +				rc = kstrtoul(argv[curarg++], 10, &res);
>>>  				if (rc == 0)
>>>  					event->parameter = res;
>>>  			}
>>
>> The new kstrtoul() promises not to touch the result field in the event
>> of a conversion error, so &event->parameter can be passed directly to
>> it, getting rid of the variables rc and res and the if statement.
> 
> What should be done in case of error?

The same as now: leave event->parameter alone. It's preset to -1
which will later be interpreted as "erroneous or missing value".

> Compiler will warn if kstrto*() result is unused:
> 
> 	kstrtoul(argv[curarg++], 10, &event->parameter);

That's annoying of course. Perhaps silence the warning with some
innocuous pro forma activity like emitting a message:

	if (kstrtoul(argv[curarg++], 10, &event->parameter))
		dev_warn(cs->dev, "bad number");

Or rearrange the code to assign -1 only after an error is
detected, like so:

@@ -578,15 +578,9 @@ void gigaset_handle_modem_response(struct cardstate *cs)
                                curarg = params - 1;
                        break;
                case RT_NUMBER:
-                       event->parameter = -1;
-                       if (curarg < params) {
-                               unsigned long res;
-                               int rc;
-
-                               rc = strict_strtoul(argv[curarg++], 10, &res);
-                               if (rc == 0)
-                                       event->parameter = res;
-                       }
+                       if (curarg >= params ||
+                           kstrtoul(argv[curarg], 10, &event->parameter))
+                               event->parameter = -1;
                        gig_dbg(DEBUG_EVENT, "parameter==%d", event->parameter);
                        break;
                }

Thanks,
Tilman

-- 
Tilman Schmidt
Phoenix Software GmbH
Bonn, Germany
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/