At Tue, 05 Mar 2013 15:41:06 -0500, Christine Spang wrote:
On 03/05/2013 04:05 AM, Takashi Iwai wrote:
At Mon, 4 Mar 2013 17:02:59 -0500, Christine Spang wrote:
Having snd_BUG_ON() only evaluate its conditional when CONFIG_SND_DEBUG is set leads to frequent bugs, since other similar macros in the kernel have different behavior. Let's make snd_BUG_ON() act like those macros so it will stop being accidentally misused.
Signed-off-by: Christine Spang christine.spang@oracle.com
Sounds reasonable. The dependency on CONFIG_SND_DEBUG was for allowing more optimization, but since we use this for more places than expected, this change would be safer indeed.
If no one has objection, I'll apply it for 3.10 kernel.
thanks,
Takashi
This ought to be considered for 3.9 and stable@ as well. It fixes NULL derefs all over the place, e.g.
No, it doesn't "fix".
sound/core/device.c:126
if (snd_BUG_ON(!card || !device_data)) return -ENXIO; list_for_each_entry(dev, &card->devices, list) { [...]If card == NULL and CONFIG_SND_DEBUG is off, this code will NULL deref.
Yes, but such a condition must not happen. If card = NULL is passed, it's an error of the caller side, and the code there doesn't have to take care in general.
In other words, it's a good stuff to catch a bug. But it's never been there for "fixing" anything per se. That's the reason why it is turned off when no debug option is set.
But I agree that enabling such a check would be safer, indeed, since people tend to rely on the checks. So I'm for your patch, but this isn't the thing for the current tree or stable trees.
There are some 600 other instances of snd_BUG_ON() being used dubiously in the current tree. Some of these instances even perform extra cleanup before returning in error conditions.
Give more concrete examples. Such places must be fixed instead.
It's really broken with CONFIG_SND_DEBUG off, and no major distro ships production kernels with this setting enabled.
If it's broken, it's the caller side, not the fact that snd_BUG_ON() isn't compiled in.
Takashi