2011-02-13 20:53:44

by Bodo Eggert

[permalink] [raw]
Subject: Re: [PATCH] md: Remove risk of overflow via sprintf) by using snprintf() in md_check_recovery()

Michael Tokarev <[email protected]> wrote:
> 12.02.2011 12:34, Daniel K. wrote:
>> Jesper Juhl wrote:

>>> sprintf() is dangerous - given the wrong source string it will
>>> overflow the destination. snprintf() is safer in that at least we'll
>>> never overflow the destination. Even if overflow will never happen
>>> today, code changes over time and snprintf() is just safer in the long
>>> run.
>>
>>> - sprintf(nm,"rd%d", rdev->raid_disk);
>>> + snprintf(nm, sizeof(nm), "rd%d",
>>> rdev->raid_disk);
>>> sysfs_remove_link(&mddev->kobj, nm);

> C'mon guys, this is pointless. 20 bytes allocated for the device
> name, and this is for raid disk number. It is impossible to have
> more than 10^17 (20 bytes total, 2 for "rd" and on for the zero
> terminator) drives in a single array.

If you argue that you might get a buffer overflow, you'll have to check
for snprintf errors, too.
--
Logic: The art of being wrong with confidence...

Friß, Spammer: [email protected] [email protected]
[email protected] [email protected]