On Tue, Feb 17, 2015 at 04:17:24PM -0800, Andrew Morton wrote:
> ?
>
> Begin forwarded message:
>
> Date: Mon, 16 Feb 2015 10:48:50 -0800
> From: Anshul Garg <[email protected]>
> To: [email protected]
> Cc: [email protected], [email protected], [email protected]
> Subject: [PATCH] lib/kstrtox.c Stop parsing integer on overflow
>
>
> From: Anshul Garg <[email protected]>
>
> While converting string representation to integer
> break the loop if overflow is detected.
>
> Signed-off-by: Anshul Garg <[email protected]>
> ---
> lib/kstrtox.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index ec8da78..6f30209 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -70,8 +70,10 @@ unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long
> * it in the max base we support (16)
> */
> if (unlikely(res & (~0ull << 60))) {
> - if (res > div_u64(ULLONG_MAX - val, base))
> + if (res > div_u64(ULLONG_MAX - val, base)) {
> overflow = 1;
> + break;
> + }
> }
> res = res * base + val;
> rv++;
The _notion_ of a patch is OK if you want EVERY simple_strtoull() call
to stop parsing past overflow right now. It SHOULD have done so from day 1,
but it doesn't do that.
When I wrote kstrto*() code I deliberatedly didn't break this bug
because of the sheer number of call sites.
If you are OK with changing bug-for-bug compatibility,
then patch simply need to delete overflow detection code.
Alexey
On Wed, Feb 18, 2015 at 1:14 PM, Alexey Dobriyan <[email protected]> wrote:
> On Tue, Feb 17, 2015 at 04:17:24PM -0800, Andrew Morton wrote:
>> ?
>>
>> Begin forwarded message:
>>
>> Date: Mon, 16 Feb 2015 10:48:50 -0800
>> From: Anshul Garg <[email protected]>
>> To: [email protected]
>> Cc: [email protected], [email protected], [email protected]
>> Subject: [PATCH] lib/kstrtox.c Stop parsing integer on overflow
>>
>>
>> From: Anshul Garg <[email protected]>
>>
>> While converting string representation to integer
>> break the loop if overflow is detected.
>>
>> Signed-off-by: Anshul Garg <[email protected]>
>> ---
>> lib/kstrtox.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index ec8da78..6f30209 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -70,8 +70,10 @@ unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long
>> * it in the max base we support (16)
>> */
>> if (unlikely(res & (~0ull << 60))) {
>> - if (res > div_u64(ULLONG_MAX - val, base))
>> + if (res > div_u64(ULLONG_MAX - val, base)) {
>> overflow = 1;
>> + break;
>> + }
>> }
>> res = res * base + val;
>> rv++;
>
> The _notion_ of a patch is OK if you want EVERY simple_strtoull() call
> to stop parsing past overflow right now. It SHOULD have done so from day 1,
> but it doesn't do that.
>
> When I wrote kstrto*() code I deliberatedly didn't break this bug
> because of the sheer number of call sites.
>
> If you are OK with changing bug-for-bug compatibility,
> then patch simply need to delete overflow detection code.
>
> Alexey
I think this patch won't break any existing module using this function.
because this function sets KSTRTOX_OVERFLOW as error status.
which is checked by calling function to determine whether value is correct
or not.
If this flag is set we can simply discard the parsed value.
On Thu, Feb 19, 2015 at 8:53 PM, Anshul Garg <[email protected]> wrote:
> On Wed, Feb 18, 2015 at 1:14 PM, Alexey Dobriyan <[email protected]> wrote:
>> On Tue, Feb 17, 2015 at 04:17:24PM -0800, Andrew Morton wrote:
>>> ?
>>>
>>> Begin forwarded message:
>>>
>>> Date: Mon, 16 Feb 2015 10:48:50 -0800
>>> From: Anshul Garg <[email protected]>
>>> To: [email protected]
>>> Cc: [email protected], [email protected], [email protected]
>>> Subject: [PATCH] lib/kstrtox.c Stop parsing integer on overflow
>>>
>>>
>>> From: Anshul Garg <[email protected]>
>>>
>>> While converting string representation to integer
>>> break the loop if overflow is detected.
>>>
>>> Signed-off-by: Anshul Garg <[email protected]>
>>> ---
>>> lib/kstrtox.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>>> index ec8da78..6f30209 100644
>>> --- a/lib/kstrtox.c
>>> +++ b/lib/kstrtox.c
>>> @@ -70,8 +70,10 @@ unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long
>>> * it in the max base we support (16)
>>> */
>>> if (unlikely(res & (~0ull << 60))) {
>>> - if (res > div_u64(ULLONG_MAX - val, base))
>>> + if (res > div_u64(ULLONG_MAX - val, base)) {
>>> overflow = 1;
>>> + break;
>>> + }
>>> }
>>> res = res * base + val;
>>> rv++;
>>
>> The _notion_ of a patch is OK if you want EVERY simple_strtoull() call
>> to stop parsing past overflow right now. It SHOULD have done so from day 1,
>> but it doesn't do that.
>>
>> When I wrote kstrto*() code I deliberatedly didn't break this bug
>> because of the sheer number of call sites.
>>
>> If you are OK with changing bug-for-bug compatibility,
>> then patch simply need to delete overflow detection code.
>>
>> Alexey
>
> I think this patch won't break any existing module using this function.
> because this function sets KSTRTOX_OVERFLOW as error status.
>
> which is checked by calling function to determine whether value is correct
> or not.
>
> If this flag is set we can simply discard the parsed value.
Which is my previous email is all about.
If you discard, every single simple_strto*() call will change its behaviour.
If you're OK with that, patch is incomplete.
Alexey