Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

>>>
>>> well... I think, this should work for both compile time known buffers and
>>> heap-alloced buffers.
>>>
>>
>> this is what __builtin_object_size() is for
>> (and sprintf already uses that due to -D_FORTIFY_SOURCE=2)
>>
>
> yes, but I didn't want to use GCC C extensions (e.g. __builtin_foo). though,
> clang/llvm probably support them.
>
>> btw all these "n" usages in these patches are buggy, they do not leave space for a trailing 0
>> making the problem worse, not better...
>> esp since with -D_FORTIFY_SOURCE=2 the compiler will abort the program if there's an overflow,
>> and now you make it silently keep running but corrupt.
>
> yes, this is the reason behind this proposal. these patches a) don't check for
> overrun; b) don't handle overrun. they just shut up the compiler.


that's never a good tradeoff.
-D_FORTIFY_SOURCE-2 is an essential compiler feature that has a defined semantic/etc
patches that make behavior worse than that baseline, but shut up some warning, are damage
not value.

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]

On 10/15/2014 08:42 AM, Sergey Senozhatsky wrote:
> Joe, care to take a second look at those buffer overruns?

All review feedback for this series will be addressed over the next few
days.

This is a "spare time" effort for me, so I can't promise
grease-lightning turn-around.

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On (10/15/14 08:24), Arjan van de Ven wrote:
> >
> >>the data is still truncated and partial.. and you keep running with it.
> >
> >which is not that big deal. I agree, that this is not 100% handy.
> >but suppose there is a process with very long name. long enough to cause
> >overrun and powertop abort(). and that process is part of a runtime and
> >it must be alive, etc... and that automatically leaves only one option --
> >either that process or powertop. and never together.
> >
> 
> for cases where you know you can truncate correctly, making a "truncate_string_to(str, n, trailing_pattern)" helper that you
> call explicitly is likely a better answer than hiding it inside a sprintf-like makro.
> (and you then truncate the source string/content)
> 
> but maybe I'm just getting old and grumpy ;-)
> 
> 
> also asprintf() for cases where you don't know how long it will be is not a bad idea

the idea was (and is) to introduce min (nearly 0) runtime overhead.



I'm not a fan of asprintf() personally. hidden malloc(), manual free()
everywhere, possible memleaks, etc...


Joe, care to take a second look at those buffer overruns?

	-ss

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

>
>> the data is still truncated and partial.. and you keep running with it.
>
> which is not that big deal. I agree, that this is not 100% handy.
> but suppose there is a process with very long name. long enough to cause
> overrun and powertop abort(). and that process is part of a runtime and
> it must be alive, etc... and that automatically leaves only one option --
> either that process or powertop. and never together.
>

for cases where you know you can truncate correctly, making a "truncate_string_to(str, n, trailing_pattern)" helper that you
call explicitly is likely a better answer than hiding it inside a sprintf-like makro.
(and you then truncate the source string/content)

but maybe I'm just getting old and grumpy ;-)


also asprintf() for cases where you don't know how long it will be is not a bad idea

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On (10/15/14 08:01), Arjan van de Ven wrote:
> >>>yes, this is the reason behind this proposal. these patches a) don't check for
> >>>overrun; b) don't handle overrun. they just shut up the compiler.
> >>
> >>
> >>that's never a good tradeoff.
> >>-D_FORTIFY_SOURCE-2 is an essential compiler feature that has a defined semantic/etc
> >>patches that make behavior worse than that baseline, but shut up some warning, are damage
> >>not value.
> >>
> >
> >well, the macro handles overrun (should probably be snprintf(a, bsz - 1, ...) >= bsz)
> >and replaces small part of the buffer with '...\0'. which is, imho, a bit better
> >than `backtrace(); abort();' (I don't think we can benefit from compile-time checks
> >of -D_FORTIFY_SOURCE=2, only run time ones) for developers that behaviour makes
> >sense, for users it's just "seg fault/core dump/whatever"
> 
> well it's that or corruption....

well '...' is just for example. we usually don't see processes
with '...' or devices with '...' in their names. could be any
error indication. e.g. 'ERROR', 'E2BIG', etc.

> the data is still truncated and partial.. and you keep running with it.

which is not that big deal. I agree, that this is not 100% handy.
but suppose there is a process with very long name. long enough to cause
overrun and powertop abort(). and that process is part of a runtime and
it must be alive, etc... and that automatically leaves only one option --
either that process or powertop. and never together.

	-ss

> (this is why many people first think strlcpy is a good idea, and then they think more about it and realize it's not)
> 
> 

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]

On (10/15/14 23:46), Sergey Senozhatsky wrote:
> > >>
> > >>this is what __builtin_object_size() is for
> > >>(and sprintf already uses that due to -D_FORTIFY_SOURCE=2)
> > >>
> > >
> > >yes, but I didn't want to use GCC C extensions (e.g. __builtin_foo). though,
> > >clang/llvm probably support them.
> > >
> > >>btw all these "n" usages in these patches are buggy, they do not leave space for a trailing 0
> > >>making the problem worse, not better...
> > >>esp since with -D_FORTIFY_SOURCE=2 the compiler will abort the program if there's an overflow,
> > >>and now you make it silently keep running but corrupt.
> > >
> > >yes, this is the reason behind this proposal. these patches a) don't check for
> > >overrun; b) don't handle overrun. they just shut up the compiler.
> > 
> > 
> > that's never a good tradeoff.
> > -D_FORTIFY_SOURCE-2 is an essential compiler feature that has a defined semantic/etc
> > patches that make behavior worse than that baseline, but shut up some warning, are damage
> > not value.
> > 
> 
> well, the macro handles overrun (should probably be snprintf(a, bsz - 1, ...) >= bsz)
> and replaces small part of the buffer with '...\0'. which is, imho, a bit better
> than `backtrace(); abort();' (I don't think we can benefit from compile-time checks
> of -D_FORTIFY_SOURCE=2, only run time ones) for developers that behaviour makes
> sense, for users it's just "seg fault/core dump/whatever". so I don't think that
> that _sprintf() macro makes the situation even worse. though, I'm not married to that
> macro.
> 

I think I recall my missing "d) point":
-- bug fixing is harder with only -D_FORTIFY_SOURCE=2 behaviour for strip-ed
ELFs (not sure how often distros do this. in embedded may be. I guess).

	-ss

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

>>> yes, this is the reason behind this proposal. these patches a) don't check for
>>> overrun; b) don't handle overrun. they just shut up the compiler.
>>
>>
>> that's never a good tradeoff.
>> -D_FORTIFY_SOURCE-2 is an essential compiler feature that has a defined semantic/etc
>> patches that make behavior worse than that baseline, but shut up some warning, are damage
>> not value.
>>
>
> well, the macro handles overrun (should probably be snprintf(a, bsz - 1, ...) >= bsz)
> and replaces small part of the buffer with '...\0'. which is, imho, a bit better
> than `backtrace(); abort();' (I don't think we can benefit from compile-time checks
> of -D_FORTIFY_SOURCE=2, only run time ones) for developers that behaviour makes
> sense, for users it's just "seg fault/core dump/whatever"

well it's that or corruption....
the data is still truncated and partial.. and you keep running with it.

(this is why many people first think strlcpy is a good idea, and then they think more about it and realize it's not)

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On (10/15/14 07:34), Arjan van de Ven wrote:
> >>>
> >>>well... I think, this should work for both compile time known buffers and
> >>>heap-alloced buffers.
> >>>
> >>
> >>this is what __builtin_object_size() is for
> >>(and sprintf already uses that due to -D_FORTIFY_SOURCE=2)
> >>
> >
> >yes, but I didn't want to use GCC C extensions (e.g. __builtin_foo). though,
> >clang/llvm probably support them.
> >
> >>btw all these "n" usages in these patches are buggy, they do not leave space for a trailing 0
> >>making the problem worse, not better...
> >>esp since with -D_FORTIFY_SOURCE=2 the compiler will abort the program if there's an overflow,
> >>and now you make it silently keep running but corrupt.
> >
> >yes, this is the reason behind this proposal. these patches a) don't check for
> >overrun; b) don't handle overrun. they just shut up the compiler.
> 
> 
> that's never a good tradeoff.
> -D_FORTIFY_SOURCE-2 is an essential compiler feature that has a defined semantic/etc
> patches that make behavior worse than that baseline, but shut up some warning, are damage
> not value.
> 

well, the macro handles overrun (should probably be snprintf(a, bsz - 1, ...) >= bsz)
and replaces small part of the buffer with '...\0'. which is, imho, a bit better
than `backtrace(); abort();' (I don't think we can benefit from compile-time checks
of -D_FORTIFY_SOURCE=2, only run time ones) for developers that behaviour makes
sense, for users it's just "seg fault/core dump/whatever". so I don't think that
that _sprintf() macro makes the situation even worse. though, I'm not married to that
macro.

	-ss

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]

On (10/15/14 07:05), Arjan van de Ven wrote:
> On 10/15/2014 6:55 AM, Sergey Senozhatsky wrote:
> >On (10/15/14 21:54), Sergey Senozhatsky wrote:
> >>I believe Marc MERLIN has reported a crash due to sprint() buffer overrun
> >>some (long) time ago. back then my idea (which I never submitted... or
> >>did I?...) was to add our own lib.cpp::_sprint() [or whatever the name]
> >>and replace all snprintf/sprint at least for buffers with compile time
> >>known sizes (aka stack buffers) -- this let us:
> >>
> >>  a) calculate buffer sizeof() within this function and never hit any
> >>errors when someone change the buffer size from buf[32] to buf[23]
> >>and forget to update all snprint()s
> >>
> >>  b) handle buffer overruns and append too-small buffers with some
> >>meaningfull 'watermark' that will give us a hint that that buffer
> >>probably needs to be extended. (my choise was '...'). e.g.
> >>
> >>show
> >>	10	device_name_too_lo...
> >>
> >>instead of
> >>	10	device_name_too_lo
> >>
> >>
> >>and we remove a great pile of "numbers, numbers, numbers" from the code
> >>- snprintf(name, 20, "%s", all_power[i]->type());
> >>+ _sprintf(name, "%s", all_power[i]->type());
> >>
> >>
> >>  c) review can be easier, because we can make a rule to forbid
> >>direct sprint()/snprintf() usage (for compile time known buffers).
> >>
> >>  d) I forgot what was my d) point...
> >>
> >>
> >>I still think this is not that bad.
> >>any ideas/objections?
> >
> >
> >#define _sprintf(a,...)							\
> >	{								\
> >		if (((void *)&(a)) == ((void *)(a))) {			\
> >			int bsz = sizeof((a));				\
> >			if (snprintf((a), bsz, __VA_ARGS__) >= bsz)	\
> >				strcat((a) + bsz - 4, "...");		\
> >		} else {						\
> >			sprintf((a), __VA_ARGS__);			\
> >		}							\
> >	} while (0);
> >
> >
> >well... I think, this should work for both compile time known buffers and
> >heap-alloced buffers.
> >
> 
> this is what __builtin_object_size() is for
> (and sprintf already uses that due to -D_FORTIFY_SOURCE=2)
> 

yes, but I didn't want to use GCC C extensions (e.g. __builtin_foo). though,
clang/llvm probably support them.

> btw all these "n" usages in these patches are buggy, they do not leave space for a trailing 0
> making the problem worse, not better...
> esp since with -D_FORTIFY_SOURCE=2 the compiler will abort the program if there's an overflow,
> and now you make it silently keep running but corrupt.

yes, this is the reason behind this proposal. these patches a) don't check for
overrun; b) don't handle overrun. they just shut up the compiler.

	-ss

> 
> 
> 

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

On 10/15/2014 6:55 AM, Sergey Senozhatsky wrote:
> On (10/15/14 21:54), Sergey Senozhatsky wrote:
>> I believe Marc MERLIN has reported a crash due to sprint() buffer overrun
>> some (long) time ago. back then my idea (which I never submitted... or
>> did I?...) was to add our own lib.cpp::_sprint() [or whatever the name]
>> and replace all snprintf/sprint at least for buffers with compile time
>> known sizes (aka stack buffers) -- this let us:
>>
>>   a) calculate buffer sizeof() within this function and never hit any
>> errors when someone change the buffer size from buf[32] to buf[23]
>> and forget to update all snprint()s
>>
>>   b) handle buffer overruns and append too-small buffers with some
>> meaningfull 'watermark' that will give us a hint that that buffer
>> probably needs to be extended. (my choise was '...'). e.g.
>>
>> show
>> 	10	device_name_too_lo...
>>
>> instead of
>> 	10	device_name_too_lo
>>
>>
>> and we remove a great pile of "numbers, numbers, numbers" from the code
>> - snprintf(name, 20, "%s", all_power[i]->type());
>> + _sprintf(name, "%s", all_power[i]->type());
>>
>>
>>   c) review can be easier, because we can make a rule to forbid
>> direct sprint()/snprintf() usage (for compile time known buffers).
>>
>>   d) I forgot what was my d) point...
>>
>>
>> I still think this is not that bad.
>> any ideas/objections?
>
>
> #define _sprintf(a,...)							\
> 	{								\
> 		if (((void *)&(a)) == ((void *)(a))) {			\
> 			int bsz = sizeof((a));				\
> 			if (snprintf((a), bsz, __VA_ARGS__) >= bsz)	\
> 				strcat((a) + bsz - 4, "...");		\
> 		} else {						\
> 			sprintf((a), __VA_ARGS__);			\
> 		}							\
> 	} while (0);
>
>
> well... I think, this should work for both compile time known buffers and
> heap-alloced buffers.
>

this is what __builtin_object_size() is for
(and sprintf already uses that due to -D_FORTIFY_SOURCE=2)

btw all these "n" usages in these patches are buggy, they do not leave space for a trailing 0
making the problem worse, not better...
esp since with -D_FORTIFY_SOURCE=2 the compiler will abort the program if there's an overflow,
and now you make it silently keep running but corrupt.

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 4938 bytes --]

On (10/15/14 22:55), Sergey Senozhatsky wrote:
> On (10/15/14 21:54), Sergey Senozhatsky wrote:
> > I believe Marc MERLIN has reported a crash due to sprint() buffer overrun
> > some (long) time ago. back then my idea (which I never submitted... or
> > did I?...) was to add our own lib.cpp::_sprint() [or whatever the name]
> > and replace all snprintf/sprint at least for buffers with compile time
> > known sizes (aka stack buffers) -- this let us:
> > 
> >  a) calculate buffer sizeof() within this function and never hit any
> > errors when someone change the buffer size from buf[32] to buf[23]
> > and forget to update all snprint()s
> > 
> >  b) handle buffer overruns and append too-small buffers with some
> > meaningfull 'watermark' that will give us a hint that that buffer
> > probably needs to be extended. (my choise was '...'). e.g.
> > 
> > show
> > 	10	device_name_too_lo...
> > 
> > instead of
> > 	10	device_name_too_lo
> > 
> > 
> > and we remove a great pile of "numbers, numbers, numbers" from the code
> > - snprintf(name, 20, "%s", all_power[i]->type());
> > + _sprintf(name, "%s", all_power[i]->type());
> > 
> > 
> >  c) review can be easier, because we can make a rule to forbid
> > direct sprint()/snprintf() usage (for compile time known buffers).
> > 
> >  d) I forgot what was my d) point...
> > 
> > 
> > I still think this is not that bad.
> > any ideas/objections?
> 
> 
> #define _sprintf(a,...)							\
> 	{								\
> 		if (((void *)&(a)) == ((void *)(a))) {			\
> 			int bsz = sizeof((a));				\
> 			if (snprintf((a), bsz, __VA_ARGS__) >= bsz)	\
> 				strcat((a) + bsz - 4, "...");		\

.... 				^^^^^^ strcpy() of course.

> 		} else {						\
> 			sprintf((a), __VA_ARGS__);			\
> 		}							\
> 	} while (0);
> 


#define _sprintf(a,...)							\
	{								\
		if (((void *)&(a)) == ((void *)(a))) {			\
			int bsz = sizeof((a))/sizeof((a)[0]);		\
			if (snprintf((a), bsz, __VA_ARGS__) >= bsz)	\
				strcpy((a) + bsz - 4, "...");		\
		} else {						\
			sprintf((a), __VA_ARGS__);			\
		}							\
	} while (0);


this makes assuption about min buffer size, etc., etc.
anyway, just an idea.

	-ss

> well... I think, this should work for both compile time known buffers and
> heap-alloced buffers.
> 
> 
> test program:
> 
> 	char x[10];
> 	char d[240];
> 	char *p = malloc(10);
> 
> 	_sprintf(x, "%s", "ttttttttttttttttttttttttttttttttttttttt");
> 	_sprintf(d, "%s", "ttttttttttttttttttttttttttttttttttttttt");
> 	/* bug here */
> 	_sprintf(p, "%s", "ttttttttttttttttttttttttttttttttttttttt");
> 	printf("%s\n%s\n%s\n", x, d, p);
> 
> 
> $ ./a.out 
> ttttttttt...
> ttttttttttttttttttttttttttttttttttttttt
> ttttttttttttttttttttttttttttttttttttttt
> 
> 
> just what we want.
> 
> 	-ss
> 
> > 
> > > Signed-off-by: Joe Konno <joe.konno(a)intel.com>
> > > ---
> > >  src/process/do_process.cpp | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp
> > > index dd7d982..2b5a327 100644
> > > --- a/src/process/do_process.cpp
> > > +++ b/src/process/do_process.cpp
> > > @@ -856,8 +856,8 @@ void process_update_display(void)
> > >  
> > >  		format_watts(all_power[i]->Witts(), power, 10);
> > >  		if (!show_power)
> > > -			strcpy(power, "          ");
> > > -		sprintf(name, "%s", all_power[i]->type());
> > > +			strncpy(power, "          ", 16);
> > > +		snprintf(name, 20, "%s", all_power[i]->type());
> > >  
> > >  		align_string(name, 14, 20);
> > >  
> > > @@ -867,9 +867,9 @@ void process_update_display(void)
> > >  		usage[0] = 0;
> > >  		if (all_power[i]->usage_units()) {
> > >  			if (all_power[i]->usage() < 1000)
> > > -				sprintf(usage, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage_units());
> > > +				snprintf(usage, 20, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage_units());
> > >  			else
> > > -				sprintf(usage, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
> > > +				snprintf(usage, 20, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
> > >  		}
> > >  
> > >  		align_string(usage, 14, 20);
> > > @@ -878,7 +878,7 @@ void process_update_display(void)
> > >  		if (!all_power[i]->show_events())
> > >  			events[0] = 0;
> > >  		else if (all_power[i]->events() <= 0.3)
> > > -			sprintf(events, "%5.2f", all_power[i]->events());
> > > +			snprintf(events, 20, "%5.2f", all_power[i]->events());
> > >  
> > >  		align_string(events, 12, 20);
> > >  		wprintw(win, "%s  %s %s %s %s\n", power, usage, events, name, pretty_print(all_power[i]->description(), descr, 128));
> > > -- 
> > > 2.1.2
> > > 
> > > _______________________________________________
> > > PowerTop mailing list
> > > PowerTop(a)lists.01.org
> > > https://lists.01.org/mailman/listinfo/powertop
> > > 

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 4197 bytes --]

On (10/15/14 21:54), Sergey Senozhatsky wrote:
> I believe Marc MERLIN has reported a crash due to sprint() buffer overrun
> some (long) time ago. back then my idea (which I never submitted... or
> did I?...) was to add our own lib.cpp::_sprint() [or whatever the name]
> and replace all snprintf/sprint at least for buffers with compile time
> known sizes (aka stack buffers) -- this let us:
> 
>  a) calculate buffer sizeof() within this function and never hit any
> errors when someone change the buffer size from buf[32] to buf[23]
> and forget to update all snprint()s
> 
>  b) handle buffer overruns and append too-small buffers with some
> meaningfull 'watermark' that will give us a hint that that buffer
> probably needs to be extended. (my choise was '...'). e.g.
> 
> show
> 	10	device_name_too_lo...
> 
> instead of
> 	10	device_name_too_lo
> 
> 
> and we remove a great pile of "numbers, numbers, numbers" from the code
> - snprintf(name, 20, "%s", all_power[i]->type());
> + _sprintf(name, "%s", all_power[i]->type());
> 
> 
>  c) review can be easier, because we can make a rule to forbid
> direct sprint()/snprintf() usage (for compile time known buffers).
> 
>  d) I forgot what was my d) point...
> 
> 
> I still think this is not that bad.
> any ideas/objections?


#define _sprintf(a,...)							\
	{								\
		if (((void *)&(a)) == ((void *)(a))) {			\
			int bsz = sizeof((a));				\
			if (snprintf((a), bsz, __VA_ARGS__) >= bsz)	\
				strcat((a) + bsz - 4, "...");		\
		} else {						\
			sprintf((a), __VA_ARGS__);			\
		}							\
	} while (0);


well... I think, this should work for both compile time known buffers and
heap-alloced buffers.


test program:

	char x[10];
	char d[240];
	char *p = malloc(10);

	_sprintf(x, "%s", "ttttttttttttttttttttttttttttttttttttttt");
	_sprintf(d, "%s", "ttttttttttttttttttttttttttttttttttttttt");
	/* bug here */
	_sprintf(p, "%s", "ttttttttttttttttttttttttttttttttttttttt");
	printf("%s\n%s\n%s\n", x, d, p);


$ ./a.out 
ttttttttt...
ttttttttttttttttttttttttttttttttttttttt
ttttttttttttttttttttttttttttttttttttttt


just what we want.

	-ss

> 
> > Signed-off-by: Joe Konno <joe.konno(a)intel.com>
> > ---
> >  src/process/do_process.cpp | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp
> > index dd7d982..2b5a327 100644
> > --- a/src/process/do_process.cpp
> > +++ b/src/process/do_process.cpp
> > @@ -856,8 +856,8 @@ void process_update_display(void)
> >  
> >  		format_watts(all_power[i]->Witts(), power, 10);
> >  		if (!show_power)
> > -			strcpy(power, "          ");
> > -		sprintf(name, "%s", all_power[i]->type());
> > +			strncpy(power, "          ", 16);
> > +		snprintf(name, 20, "%s", all_power[i]->type());
> >  
> >  		align_string(name, 14, 20);
> >  
> > @@ -867,9 +867,9 @@ void process_update_display(void)
> >  		usage[0] = 0;
> >  		if (all_power[i]->usage_units()) {
> >  			if (all_power[i]->usage() < 1000)
> > -				sprintf(usage, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage_units());
> > +				snprintf(usage, 20, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage_units());
> >  			else
> > -				sprintf(usage, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
> > +				snprintf(usage, 20, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
> >  		}
> >  
> >  		align_string(usage, 14, 20);
> > @@ -878,7 +878,7 @@ void process_update_display(void)
> >  		if (!all_power[i]->show_events())
> >  			events[0] = 0;
> >  		else if (all_power[i]->events() <= 0.3)
> > -			sprintf(events, "%5.2f", all_power[i]->events());
> > +			snprintf(events, 20, "%5.2f", all_power[i]->events());
> >  
> >  		align_string(events, 12, 20);
> >  		wprintw(win, "%s  %s %s %s %s\n", power, usage, events, name, pretty_print(all_power[i]->description(), descr, 128));
> > -- 
> > 2.1.2
> > 
> > _______________________________________________
> > PowerTop mailing list
> > PowerTop(a)lists.01.org
> > https://lists.01.org/mailman/listinfo/powertop
> > 

Re: [Powertop] [PATCH v2 6/8] Stop buffer overflow

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 3164 bytes --]

On (10/14/14 11:09), Joe Konno wrote:

I believe Marc MERLIN has reported a crash due to sprint() buffer overrun
some (long) time ago. back then my idea (which I never submitted... or
did I?...) was to add our own lib.cpp::_sprint() [or whatever the name]
and replace all snprintf/sprint at least for buffers with compile time
known sizes (aka stack buffers) -- this let us:

 a) calculate buffer sizeof() within this function and never hit any
errors when someone change the buffer size from buf[32] to buf[23]
and forget to update all snprint()s

 b) handle buffer overruns and append too-small buffers with some
meaningfull 'watermark' that will give us a hint that that buffer
probably needs to be extended. (my choise was '...'). e.g.

show
	10	device_name_too_lo...

instead of
	10	device_name_too_lo


and we remove a great pile of "numbers, numbers, numbers" from the code
- snprintf(name, 20, "%s", all_power[i]->type());
+ _sprintf(name, "%s", all_power[i]->type());


 c) review can be easier, because we can make a rule to forbid
direct sprint()/snprintf() usage (for compile time known buffers).

 d) I forgot what was my d) point...


I still think this is not that bad.
any ideas/objections?

	-ss

> Signed-off-by: Joe Konno <joe.konno(a)intel.com>
> ---
>  src/process/do_process.cpp | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp
> index dd7d982..2b5a327 100644
> --- a/src/process/do_process.cpp
> +++ b/src/process/do_process.cpp
> @@ -856,8 +856,8 @@ void process_update_display(void)
>  
>  		format_watts(all_power[i]->Witts(), power, 10);
>  		if (!show_power)
> -			strcpy(power, "          ");
> -		sprintf(name, "%s", all_power[i]->type());
> +			strncpy(power, "          ", 16);
> +		snprintf(name, 20, "%s", all_power[i]->type());
>  
>  		align_string(name, 14, 20);
>  
> @@ -867,9 +867,9 @@ void process_update_display(void)
>  		usage[0] = 0;
>  		if (all_power[i]->usage_units()) {
>  			if (all_power[i]->usage() < 1000)
> -				sprintf(usage, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage_units());
> +				snprintf(usage, 20, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage_units());
>  			else
> -				sprintf(usage, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
> +				snprintf(usage, 20, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
>  		}
>  
>  		align_string(usage, 14, 20);
> @@ -878,7 +878,7 @@ void process_update_display(void)
>  		if (!all_power[i]->show_events())
>  			events[0] = 0;
>  		else if (all_power[i]->events() <= 0.3)
> -			sprintf(events, "%5.2f", all_power[i]->events());
> +			snprintf(events, 20, "%5.2f", all_power[i]->events());
>  
>  		align_string(events, 12, 20);
>  		wprintw(win, "%s  %s %s %s %s\n", power, usage, events, name, pretty_print(all_power[i]->description(), descr, 128));
> -- 
> 2.1.2
> 
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
> 

[Powertop] [PATCH v2 6/8] Stop buffer overflow

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

From: Dan Kalowsky <daniel.kalowsky(a)intel.com>

On some builds of Android, we are seeing entries that overrun this
buffer causing some ill-advised effects.  Limiting the buffer copies to
the size of the allocated buffer solves this issue.

v2: rebased atop 41c54e8

Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
 src/process/do_process.cpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp
index dd7d982..2b5a327 100644
--- a/src/process/do_process.cpp
+++ b/src/process/do_process.cpp
@@ -856,8 +856,8 @@ void process_update_display(void)
 
 		format_watts(all_power[i]->Witts(), power, 10);
 		if (!show_power)
-			strcpy(power, "          ");
-		sprintf(name, "%s", all_power[i]->type());
+			strncpy(power, "          ", 16);
+		snprintf(name, 20, "%s", all_power[i]->type());
 
 		align_string(name, 14, 20);
 
@@ -867,9 +867,9 @@ void process_update_display(void)
 		usage[0] = 0;
 		if (all_power[i]->usage_units()) {
 			if (all_power[i]->usage() < 1000)
-				sprintf(usage, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage_units());
+				snprintf(usage, 20, "%5.1f%s", all_power[i]->usage(), all_power[i]->usage_units());
 			else
-				sprintf(usage, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
+				snprintf(usage, 20, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
 		}
 
 		align_string(usage, 14, 20);
@@ -878,7 +878,7 @@ void process_update_display(void)
 		if (!all_power[i]->show_events())
 			events[0] = 0;
 		else if (all_power[i]->events() <= 0.3)
-			sprintf(events, "%5.2f", all_power[i]->events());
+			snprintf(events, 20, "%5.2f", all_power[i]->events());
 
 		align_string(events, 12, 20);
 		wprintw(win, "%s  %s %s %s %s\n", power, usage, events, name, pretty_print(all_power[i]->description(), descr, 128));
-- 
2.1.2