Re: CVS commit: src/sys/dev/pci

[christos%astron.com@localhost (Christos Zoulas)]

In article <Pine.NEB.4.64.1610260555490.23300%speedy.whooppee.com@localhost>,
Paul Goyette  <paul%whooppee.com@localhost> wrote:
>On Wed, 26 Oct 2016, matthew green wrote:
>
>> i think you're right that the 'cp' manipulation is the problem.
>> snprintf() will return the "desired" size, so upon the first
>> attempted overflow the 'cp' is moved beyond 'ep', and then the
>> next snprintf() gets a negative aka extremely massive value
>> for the buffer length and actual overflow happens here, and ssp
>> detects it.
>>
>> the fix would be to change this:
>>
>> 	cp += snprintf(cp, ep - cp, ...);
>>
>> into this:
>>
>> 	len = snprintf(cp, ep - cp, ...);
>> 	if (len > ep - cp)
>> 		return;
>> 	cp += len;
>>
>> which is annoying because there are a lot of the former.
>
>There's only 9 snprintf() calls.  I could simply provide a macro:
>
>#define ADD_TEXT(dest, end, format, ...)			\
> 	{							\
> 		int len, max = (end) - (dest);			\
>
> 		len = snprintf((dest), max, (format), __VA_ARGS__); \
> 		if (len > max)					\
> 			return;					\
> 		(dest) += len;					\
> 	}
>
>
>Then all of the snprintf() calls become simply
>
> 	ADD_TEXT(cp, ep, <format>, ...);
>
>(Of course, after last use I'd add a #undef ADD_TEXT to clean up...)

Do you really want to stop from attaching the driver because you could
not print it's name? I think that perhaps a variant that returns the
number of characters appended would be more useful. like
if (len >= max)
	dest = end;
else
	dest += len;
	
but, I would leave the whole thing alone instead :-)
or write a function that appends and moves the pointer, like

snappendprintf(char **dest, const char *end, fmt, ...) 

christos