[PATCH 1/7] Convert a bunch of sprintf to snprintf calls

[Alan Coopersmith]

On 11/02/11 04:44, walter harms wrote:
>> diff --git a/os/utils.c b/os/utils.c
>> index 1c75dfc..c828f01 100644
>> --- a/os/utils.c
>> +++ b/os/utils.c
>> @@ -258,7 +258,7 @@ LockServer(void)
>>      */
>>     tmppath = LOCK_DIR;
>>
>> -  sprintf(port, "%d", atoi(display));
>> +  snprintf(port, sizeof(port), "%d", atoi(display));
>>     len = strlen(LOCK_PREFIX)>  strlen(LOCK_TMP_PREFIX) ? strlen(LOCK_PREFIX) :
>>   						strlen(LOCK_TMP_PREFIX);
>>     len += strlen(tmppath) + strlen(port) + strlen(LOCK_SUFFIX) + 1;
>
> the snprintf() is ok but the code looks odd ....

The surrounding code is odd, but making it less odd is outside the scope
of this change.

>>       if (action->type==XkbSA_LockDeviceBtn) {
>>   	switch (act->flags&(XkbSA_LockNoUnlock|XkbSA_LockNoLock)) {
>>   	    case XkbSA_LockNoLock:
>> -		sprintf(tbuf,",affect=unlock"); break;
>> +		snprintf(tbuf,sizeof(tbuf),",affect=unlock"); break;
>>   	    case XkbSA_LockNoUnlock:
>> -		sprintf(tbuf,",affect=lock"); break;
>> +		snprintf(tbuf,sizeof(tbuf),",affect=lock"); break;
>>   	    case XkbSA_LockNoUnlock|XkbSA_LockNoLock:
>> -		sprintf(tbuf,",affect=neither"); break;
>> +		snprintf(tbuf,sizeof(tbuf),",affect=neither"); break;
>>   	    default:
>> -		sprintf(tbuf,",affect=both"); break;
>> +		snprintf(tbuf,sizeof(tbuf),",affect=both"); break;
>
>
> 	strncpy() ?
>
>
>>   	}
>>   	TryCopyStr(buf,tbuf,sz);
>>       }

Doesn't look like we need either snprintf or strncpy here actually, since
we're passing to a function to copy it already, so don't need to copy it
again to a temp buffer first.

>> @@ -1197,12 +1197,12 @@ char	buf[256],*tmp;
>>   		kn= XkbKeyNameText(xkb->names->keys[kc].name,XkbXKBFile);
>>   	    else {
>>   		static char tbuf[8];
>> -		sprintf(tbuf,"%d",kc);
>> +		snprintf(tbuf,sizeof(tbuf),"%d",kc);
>>   		kn= tbuf;
>
> why not snprintf(kn,sizeof(kn),"%d",kc); ?

Because kn is simply:
             char *kn;

with no memory allocated to it, since each case is responsible for
providing its own memory allocation.

That code could be written better - there's no need for a static buffer,
since the memory isn't accessed outside the scope of this function, but
that's a different cleanup than is being done in this patch.

-- 
	-Alan Coopersmith-        alan.coopersmith at oracle.com
	 Oracle Solaris Platform Engineering: X Window System