[PATCH] [xkb] Fix possible NULL dereference in XkbFlushLedEvents()

[Tomas Carnecky]

On Dec 7, 2009, at 2:14 AM, Peter Hutterer wrote:

> On Sun, Dec 06, 2009 at 11:52:55PM +0100, Tomas Carnecky wrote:
>> Through some code paths it is possible that NULL is being passed in the
>> 'ed' parameter to XkbFlushLedEvents(). Make sure we don't pass it along
>> to bzero().
>> 
>> Signed-off-by: Tomas Carnecky <tom at dbservice.com>
>> ---
>> xkb/xkbLEDs.c |    3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>> 
>> diff --git a/xkb/xkbLEDs.c b/xkb/xkbLEDs.c
>> index 59cdba4..dfdd5a2 100644
>> --- a/xkb/xkbLEDs.c
>> +++ b/xkb/xkbLEDs.c
>> @@ -750,7 +750,8 @@ XkbFlushLedEvents(	DeviceIntPtr			dev,
>> 	    XkbDDXUpdateDeviceIndicators(dev,sli,sli->effectiveState);
>> 	XkbSendExtensionDeviceNotify(dev,cause->client,ed);
>>     }
>> -    bzero((char *)ed,sizeof(XkbExtensionDeviceNotify));
>> +    if (ed)
>> +	bzero((char *)ed,sizeof(XkbExtensionDeviceNotify));
>>     return;
>> }
> 
> given that the bzero man page claims it's deprecated this would be a good
> time to replace it with memset.

True, but bzero is #defined to memset anyway (X11/Xfuncs.h).

> given the previous condition, the final code after applying this patch would
> look like this:
> 
> if (ed && ed->reason) {
>        foo
> }
> if (ed)
>        bzero(...);
> 
> 
> A better flow would be:
> if (ed) {
>        if (ed->reason)
>                foo
>        bzero/memset(...):
> }

I'll send an updated patch in a minute. As for bzero vs. memset, I prefer to keep logic changes separate from other changes (such as whitespace changes, function renaming etc). I have a separate commit in my tree that replaces all bzero with memset.

tom