10:15 a.m.
Hi Powertop,
I've been working with the interactive mode and have run into cases where powertop
will crash due to a series of memory corruptions. The following series of patches have
helped to reduce the frequency of the issue, although not completely solved it. The
issues arise much faster on platforms where there are constrained amounts of RAM to work
within.
Patch 1 - When shutting down the interactive display, the display bits of memory is not
correctly released. This patch provides a method for correctly doing so.
Patch 2 - Solving a documented memory leak with a non-elegant solution. The path either
adds the bundle to the stack, or it forgets about it. If it is forgotten about, make sure
to clear that memory before moving on. This is done with a simple flag variable being
set.
Patch 3 - When the tuning window is updated, the current pointer is just set adrift and
not properly free'd. This patch catches that issue and removes the dangling pointer
by holding a reference to the pointer until it is reset or specifically free'd.
Patch 4 - Someone actually added in the code to create a onetime pretty-print array, this
patch just puts it to use by setting the variable.
Patch 5 - Limiting the buffer copy to the size of the allocated buffer with snprintf.
Patch 6 - There exist some processes and entries that can and do extend beyond the length
of these buffers. This limits those entries so as not to corrupt other memory on the
system when in interactive mode.
Patch 7 - This is an untested patch, but follows along the same lines of Patch 6. It
applies the same principals only for the report method.
Patch 8 - Creates a clean_shutdown function that can be used to cleanup the memory space
at shutdown time. Calls upon parts of Patch 1 to make this happen.
There will more than likely be some more patches in the future as time permits.
12:11 a.m.
New subject: [Powertop] A series of patches towards limiting memory corruption and foot print
Sorry about a second post, looks like email stripped the attachements. Second attempt
here.
From: PowerTop [mailto:powertop-bounces@lists.01.org] On Behalf Of Kalowsky, Daniel
Sent: Tuesday, April 29, 2014 5:15 PM
To: powertop(a)lists.01.org
Subject: [Powertop] A series of patches towards limiting memory corruption and foot print
Hi Powertop,
I've been working with the interactive mode and have run into cases where powertop
will crash due to a series of memory corruptions. The following series of patches have
helped to reduce the frequency of the issue, although not completely solved it. The
issues arise much faster on platforms where there are constrained amounts of RAM to work
within.
Patch 1 - When shutting down the interactive display, the display bits of memory is not
correctly released. This patch provides a method for correctly doing so.
Patch 2 - Solving a documented memory leak with a non-elegant solution. The path either
adds the bundle to the stack, or it forgets about it. If it is forgotten about, make sure
to clear that memory before moving on. This is done with a simple flag variable being
set.
Patch 3 - When the tuning window is updated, the current pointer is just set adrift and
not properly free'd. This patch catches that issue and removes the dangling pointer
by holding a reference to the pointer until it is reset or specifically free'd.
Patch 4 - Someone actually added in the code to create a onetime pretty-print array, this
patch just puts it to use by setting the variable.
Patch 5 - Limiting the buffer copy to the size of the allocated buffer with snprintf.
Patch 6 - There exist some processes and entries that can and do extend beyond the length
of these buffers. This limits those entries so as not to corrupt other memory on the
system when in interactive mode.
Patch 7 - This is an untested patch, but follows along the same lines of Patch 6. It
applies the same principals only for the report method.
Patch 8 - Creates a clean_shutdown function that can be used to cleanup the memory space
at shutdown time. Calls upon parts of Patch 1 to make this happen.
There will more than likely be some more patches in the future as time permits.
5:28 a.m.
New subject: [Powertop] A series of patches towards limiting memory corruption and foot print
On (05/01/14 14:11), Kalowsky, Daniel wrote:
Sorry about a second post, looks like email stripped the
attachements.
Second attempt here.
Hello,
it's much easier to review/comment/etc if a patch is inlined in your
email, in plain text. (http://www.tux.org/lkml/#s1-10)
care to resend using git send-email?
thanks.
-ss
From: PowerTop [mailto:powertop-bounces@lists.01.org] On Behalf Of
Kalowsky, Daniel
Sent: Tuesday, April 29, 2014 5:15 PM
To: powertop(a)lists.01.org
Subject: [Powertop] A series of patches towards limiting memory corruption
and foot print
Hi Powertop,
I’ve been working with the interactive mode and have run into cases where
powertop will crash due to a series of memory corruptions. The following
series of patches have helped to reduce the frequency of the issue,
although not completely solved it. The issues arise much faster on
platforms where there are constrained amounts of RAM to work within.
Patch 1 – When shutting down the interactive display, the display bits of
memory is not correctly released. This patch provides a method for
correctly doing so.
Patch 2 – Solving a documented memory leak with a non-elegant solution.
The path either adds the bundle to the stack, or it forgets about it. If
it is forgotten about, make sure to clear that memory before moving on.
This is done with a simple flag variable being set.
Patch 3 – When the tuning window is updated, the current pointer is just
set adrift and not properly free’d. This patch catches that issue and
removes the dangling pointer by holding a reference to the pointer until
it is reset or specifically free’d.
Patch 4 – Someone actually added in the code to create a onetime
pretty-print array, this patch just puts it to use by setting the
variable.
Patch 5 – Limiting the buffer copy to the size of the allocated buffer
with snprintf.
Patch 6 – There exist some processes and entries that can and do extend
beyond the length of these buffers. This limits those entries so as not
to corrupt other memory on the system when in interactive mode.
Patch 7 – This is an untested patch, but follows along the same lines of
Patch 6. It applies the same principals only for the report method.
Patch 8 – Creates a clean_shutdown function that can be used to cleanup
the memory space at shutdown time. Calls upon parts of Patch 1 to make
this happen.
There will more than likely be some more patches in the future as time
permits.
_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
5:09 a.m.
New subject: [Powertop] [PATCH v2 0/8] A series of patches towards limiting memory corruption and foot print
Changes since v1:
* Rebased atop "41c54e8 allow none-ncurses messaging..."
* Removed double-free in original patch 3/8
This is a follow-on to Daniel Kalowsky's series submitted a few months
ago, before the v2.6 release of PowerTOP. It was initially merged but
then reverted just before the v2.6 release due to issues identified by
the PowerTOP community, such as segfaults on exit.
While I was not able to reproduce the segfaults, with Valgrind's
memcheck I did identify a double-free event within the original patch
series. The double-free was introduced in patch 3/8, but the double-free
was not seen until patch 8/8, when the errant function was actually
called. Fixed by removing the "delete my window" code block within
clear_tunables(), as close_display() would do the work (again). After
fix, Valgrind memcheck no longer reports a double-free event.
Dan Kalowsky (8):
Properly cleaning up the display tabs
Removing a documented memory leak
Remove another memory leak
Mark pretty_print_init as initialized
Limit sprintf to buffer size length.
Stop buffer overflow
Clean up potential buffer issues
Adding a clean_shutdown function
src/display.cpp | 11 +++++++++++
src/display.h | 1 +
src/lib.cpp | 10 ++++++----
src/main.cpp | 15 ++++++++++++---
src/parameters/parameters.h | 1 +
src/parameters/persistent.cpp | 15 +++++++++++++++
src/process/do_process.cpp | 28 ++++++++++++++--------------
src/tuning/tuning.cpp | 7 +++++++
8 files changed, 67 insertions(+), 21 deletions(-)
--
2.1.2
5:09 a.m.
New subject: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
This patch properly cleans up the display tabs so that none of the tab
data is being leaked between refresh changes
Signed-off-by: Dan Kalowsky <daniel.kalowsky(a)intel.com>
v2: rebased atop 41c54e8; there's a bug here
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/display.cpp | 11 +++++++++++
src/display.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/src/display.cpp b/src/display.cpp
index 123620a..e227e74 100644
--- a/src/display.cpp
+++ b/src/display.cpp
@@ -87,6 +87,17 @@ void reset_display(void)
resetterm();
}
+void close_display(void)
+{
+ for (unsigned int i = 0; i < tab_windows.size(); i++) {
+ if (tab_windows[tab_names[i]])
+ delete tab_windows[tab_names[i]];
+ tab_windows[tab_names[i]] = NULL;
+ }
+
+ tab_names.clear();
+}
+
WINDOW *tab_bar = NULL;
WINDOW *bottom_line = NULL;
diff --git a/src/display.h b/src/display.h
index 9af3ec0..c3e8d61 100644
--- a/src/display.h
+++ b/src/display.h
@@ -34,6 +34,7 @@ using namespace std;
extern void init_display(void);
extern void reset_display(void);
+extern void close_display(void);
extern int ncurses_initialized(void);
extern void show_tab(unsigned int tab);
extern void show_next_tab(void);
--
2.1.2
11:58 p.m.
New subject: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs
On (10/14/14 11:09), Joe Konno wrote:
src/display.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/src/display.cpp b/src/display.cpp
index 123620a..e227e74 100644
--- a/src/display.cpp
+++ b/src/display.cpp
@@ -87,6 +87,17 @@ void reset_display(void)
resetterm();
}
+void close_display(void)
+{
+ for (unsigned int i = 0; i < tab_windows.size(); i++) {
+ if (tab_windows[tab_names[i]])
+ delete tab_windows[tab_names[i]];
+ tab_windows[tab_names[i]] = NULL;
+ }
+
+ tab_names.clear();
+}
here and in other patch -- why does it matter? we are exiting,
memory will be freed anyway, etc. why take that extra effort?
-ss
WINDOW *tab_bar = NULL;
WINDOW *bottom_line = NULL;
diff --git a/src/display.h b/src/display.h
index 9af3ec0..c3e8d61 100644
--- a/src/display.h
+++ b/src/display.h
@@ -34,6 +34,7 @@ using namespace std;
extern void init_display(void);
extern void reset_display(void);
+extern void close_display(void);
extern int ncurses_initialized(void);
extern void show_tab(unsigned int tab);
extern void show_next_tab(void);
--
2.1.2
_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
5:12 a.m.
New subject: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs
On 10/15/2014 05:58 AM, Sergey Senozhatsky wrote:
here and in other patch -- why does it matter? we are exiting,
memory will be freed anyway, etc. why take that extra effort?
-ss
Adhering to the "for every new/malloc there is a delete/free" principle
as we are not using any garbage collection facilities (memory pools,
smart pointers, etc) in this project.
9:53 a.m.
New subject: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs
On Fri, Oct 17, 2014 at 11:12:12AM -0700, Joe Konno wrote:
On 10/15/2014 05:58 AM, Sergey Senozhatsky wrote:
>
> here and in other patch -- why does it matter? we are exiting,
> memory will be freed anyway, etc. why take that extra effort?
>
> -ss
Adhering to the "for every new/malloc there is a delete/free" principle
as we are not using any garbage collection facilities (memory pools,
smart pointers, etc) in this project.
I have to admit that I have asked myself why there is so little use of
RAII in powertop but I havent found a good reason,
11:17 p.m.
New subject: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs
On (10/17/14 11:12), Joe Konno wrote:
On 10/15/2014 05:58 AM, Sergey Senozhatsky wrote:
>
> here and in other patch -- why does it matter? we are exiting,
> memory will be freed anyway, etc. why take that extra effort?
>
Adhering to the "for every new/malloc there is a delete/free" principle
as we are not using any garbage collection facilities (memory pools,
smart pointers, etc) in this project.
by example.
void foo()
{
again:
for (int i = 0; i < 100; i++) {
void *p = malloc(100);
do_things(p);
do_other_things(p);
}
goto again;
}
v.s.
void foo()
{
void *p = malloc(100);
exit(0);
}
free(p) before exit(0) is really useless, the kernel will clean
up everything.
-ss
1:49 a.m.
New subject: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs
While I do not agree, I understand.
Irrespective of the OS's handling of allocated memory on exit, I
consider it best practice to free all allocated memory at each and every
program exit point. Call me pedantic, but there it is.
Cheers-- I appreciate your reviews and feedback
On 10/22/2014 05:17 AM, Sergey Senozhatsky wrote:
free(p) before exit(0) is really useless, the kernel will clean
up everything.
5:09 a.m.
New subject: [Powertop] [PATCH v2 2/8] Removing a documented memory leak
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
A non-elegant solution to it, but it works well enough to remove the
documented memory leak. Proves to provide a much larger impact on
memory constrained systems.
v2: rebased atop 41c54e8
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/parameters/parameters.h | 1 +
src/parameters/persistent.cpp | 15 +++++++++++++++
2 files changed, 16 insertions(+)
diff --git a/src/parameters/parameters.h b/src/parameters/parameters.h
index bd9fee8..72f8d32 100644
--- a/src/parameters/parameters.h
+++ b/src/parameters/parameters.h
@@ -100,6 +100,7 @@ extern void store_results(double duration);
extern void learn_parameters(int iterations, int do_base_power);
extern char *get_param_directory(const char *filename);
extern void save_all_results(const char *filename = "saved_results.powertop");
+extern void close_results(void);
extern void load_results(const char *filename);
extern void save_parameters(const char *filename);
extern void load_parameters(const char *filename);
diff --git a/src/parameters/persistent.cpp b/src/parameters/persistent.cpp
index 483227b..9a5688a 100644
--- a/src/parameters/persistent.cpp
+++ b/src/parameters/persistent.cpp
@@ -61,6 +61,16 @@ void save_all_results(const char *filename)
}
+void close_results()
+{
+ for (unsigned int i = 0; i < past_results.size(); i++) {
+ delete past_results[i];
+ }
+
+ past_results.clear();
+ return;
+}
+
void load_results(const char *filename)
{
ifstream file;
@@ -70,6 +80,7 @@ void load_results(const char *filename)
int first = 1;
unsigned int count = 0;
char* pathname;
+ int bundle_saved = 0;
pathname = get_param_directory(filename);
@@ -97,6 +108,7 @@ void load_results(const char *filename)
if (strlen(line) < 3) {
int overflow_index;
+ bundle_saved = 1;
overflow_index = 50 + (rand() % MAX_KEEP);
if (past_results.size() >= MAX_PARAM) {
/* memory leak, must free old one first */
@@ -118,6 +130,9 @@ void load_results(const char *filename)
set_result_value(line, d, bundle);
}
+ if (bundle_saved == 0)
+ delete bundle;
+
file.close();
// '%i" is for count, do not translate
fprintf(stderr, _("Loaded %i prior measurements\n"), count);
--
2.1.2
5:09 a.m.
New subject: [Powertop] [PATCH v2 3/8] Remove another memory leak
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
Pointer gets over written without ever being properly cleaned up. This
should solve that problem.
v2: rebased atop 41c54e8, do not delete window in clear_tunables() to
prevent double-free in close_display().
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/tuning/tuning.cpp | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tuning/tuning.cpp b/src/tuning/tuning.cpp
index 7d64cad..5414022 100644
--- a/src/tuning/tuning.cpp
+++ b/src/tuning/tuning.cpp
@@ -46,6 +46,8 @@
static void sort_tunables(void);
static bool should_clear = false;
+class tuning_window *tune_window;
+
class tuning_window: public tab_window {
public:
virtual void repaint(void);
@@ -80,6 +82,11 @@ void initialize_tuning(void)
init_tuning();
w->cursor_max = all_tunables.size() - 1;
+
+ if (tune_window)
+ delete tune_window;
+
+ tune_window = w;
}
--
2.1.2
5:09 a.m.
New subject: [Powertop] [PATCH v2 4/8] Mark pretty_print_init as initialized
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
There is a check for it, but it's never actually set. Now it's set.
v2: rebased atop 41c54e8
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/lib.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/lib.cpp b/src/lib.cpp
index cce7973..437803b 100644
--- a/src/lib.cpp
+++ b/src/lib.cpp
@@ -408,6 +408,8 @@ static void init_pretty_print(void)
pretty_prints["[12] i8042"] = _("PS/2 Touchpad / Keyboard /
Mouse");
pretty_prints["ahci"] = _("SATA controller");
pretty_prints["usb-device-8087-0020"] = _("Intel built in USB
hub");
+
+ pretty_print_init = 1;
}
--
2.1.2
5:09 a.m.
New subject: [Powertop] [PATCH v2 5/8] Limit sprintf to buffer size length.
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
Not completely needed, but good practice. Limiting sprintf to the
length of the buffer size. This patch not completely necessary as it's
unlikely to be an issue here.
v2: rebase atop 41c54e8; adaptated to newer context
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/lib.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/lib.cpp b/src/lib.cpp
index 437803b..9cbf78d 100644
--- a/src/lib.cpp
+++ b/src/lib.cpp
@@ -474,10 +474,10 @@ int read_msr(int cpu, uint64_t offset, uint64_t *value)
int fd;
char msr_path[256];
- sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
+ fd = snprintf(msr_path, 256, "/dev/cpu/%d/msr", cpu);
if (access(msr_path, R_OK) != 0){
- sprintf(msr_path, "/dev/msr%d", cpu);
+ fd = snprintf(msr_path, 256, "/dev/msr%d", cpu);
if (access(msr_path, R_OK) != 0){
fprintf(stderr,
@@ -506,10 +506,10 @@ int write_msr(int cpu, uint64_t offset, uint64_t value)
int fd;
char msr_path[256];
- sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
+ fd = snprintf(msr_path, 256, "/dev/cpu/%d/msr", cpu);
if (access(msr_path, R_OK) != 0){
- sprintf(msr_path, "/dev/msr%d", cpu);
+ fd = snprintf(msr_path, 256, "/dev/msr%d", cpu);
if (access(msr_path, R_OK) != 0){
fprintf(stderr,
--
2.1.2
11:15 p.m.
New subject: [Powertop] [PATCH v2 5/8] Limit sprintf to buffer size length.
On (10/14/14 11:09), Joe Konno wrote:
Not completely needed, but good practice. Limiting sprintf to the
length of the buffer size. This patch not completely necessary as it's
unlikely to be an issue here.
v2: rebase atop 41c54e8; adaptated to newer context
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/lib.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/lib.cpp b/src/lib.cpp
index 437803b..9cbf78d 100644
--- a/src/lib.cpp
+++ b/src/lib.cpp
@@ -474,10 +474,10 @@ int read_msr(int cpu, uint64_t offset, uint64_t *value)
int fd;
char msr_path[256];
- sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
+ fd = snprintf(msr_path, 256, "/dev/cpu/%d/msr", cpu);
iirc, this `fd =' has been removed. it's useless per se.
-ss
if (access(msr_path, R_OK) != 0){
- sprintf(msr_path, "/dev/msr%d", cpu);
+ fd = snprintf(msr_path, 256, "/dev/msr%d", cpu);
if (access(msr_path, R_OK) != 0){
fprintf(stderr,
@@ -506,10 +506,10 @@ int write_msr(int cpu, uint64_t offset, uint64_t value)
int fd;
char msr_path[256];
- sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
+ fd = snprintf(msr_path, 256, "/dev/cpu/%d/msr", cpu);
if (access(msr_path, R_OK) != 0){
- sprintf(msr_path, "/dev/msr%d", cpu);
+ fd = snprintf(msr_path, 256, "/dev/msr%d", cpu);
if (access(msr_path, R_OK) != 0){
fprintf(stderr,
--
2.1.2
_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
4:36 a.m.
New subject: [Powertop] [PATCH v2 5/8] Limit sprintf to buffer size length.
On 10/15/2014 05:15 AM, Sergey Senozhatsky wrote:
On (10/14/14 11:09), Joe Konno wrote:
> diff --git a/src/lib.cpp b/src/lib.cpp
> index 437803b..9cbf78d 100644
> --- a/src/lib.cpp
> +++ b/src/lib.cpp
> @@ -474,10 +474,10 @@ int read_msr(int cpu, uint64_t offset, uint64_t *value)
> int fd;
> char msr_path[256];
>
> - sprintf(msr_path, "/dev/cpu/%d/msr", cpu);
> + fd = snprintf(msr_path, 256, "/dev/cpu/%d/msr", cpu);
iirc, this `fd =' has been removed. it's useless per se.
-ss
Will fix.
5:09 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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
11:54 p.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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
12:55 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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
>
1:03 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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
> >
1:05 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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.
1:21 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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
1:34 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
>>
>> 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.
1:46 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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
2:01 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
>> 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)
2:19 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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)
2:24 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
> 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
2:42 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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
2:55 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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.
2:01 a.m.
New subject: [Powertop] [PATCH v2 6/8] Stop buffer overflow
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
5:09 a.m.
New subject: [Powertop] [PATCH v2 7/8] Clean up potential buffer issues
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
While no issues have been seen in this section of code yet, it's likely
the same issues that show on the active display will also show on the
report process.
v2: rebased atop 41c54e8
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/process/do_process.cpp | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp
index 2b5a327..ce34071 100644
--- a/src/process/do_process.cpp
+++ b/src/process/do_process.cpp
@@ -943,8 +943,8 @@ void report_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());
if (strcmp(name, "Device") == 0)
continue;
@@ -956,17 +956,17 @@ void report_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());
}
- sprintf(wakes, "%5.1f", all_power[i]->wake_ups / measurement_time);
+ snprintf(wakes, 20, "%5.1f", all_power[i]->wake_ups / measurement_time);
if (all_power[i]->wake_ups / measurement_time <= 0.3)
- sprintf(wakes, "%5.2f", all_power[i]->wake_ups / measurement_time);
- sprintf(gpus, "%5.1f", all_power[i]->gpu_ops / measurement_time);
- sprintf(disks, "%5.1f (%5.1f)", all_power[i]->hard_disk_hits /
measurement_time,
+ snprintf(wakes, 20, "%5.2f", all_power[i]->wake_ups / measurement_time);
+ snprintf(gpus, 20, "%5.1f", all_power[i]->gpu_ops / measurement_time);
+ snprintf(disks, 20, "%5.1f (%5.1f)", all_power[i]->hard_disk_hits /
measurement_time,
all_power[i]->disk_hits / measurement_time);
- sprintf(xwakes, "%5.1f", all_power[i]->xwakes / measurement_time);
+ snprintf(xwakes, 20, "%5.1f", all_power[i]->xwakes / measurement_time);
if (!all_power[i]->show_events()) {
wakes[0] = 0;
gpus[0] = 0;
--
2.1.2
5:09 a.m.
New subject: [Powertop] [PATCH v2 8/8] Adding a clean_shutdown function
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
Function is designed to be a on stop place to clean up the entire
application at shutdown time.
v2: rebased atop 41c54e8
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/main.cpp | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/main.cpp b/src/main.cpp
index d3963ed..b6ce090 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -368,6 +368,17 @@ static void powertop_init(void)
initialized = 1;
}
+void clean_shutdown()
+{
+ close_results();
+ close_display();
+ clean_open_devices();
+ clear_all_devices();
+ clear_all_cpus();
+
+ return;
+}
+
int main(int argc, char **argv)
{
@@ -498,9 +509,7 @@ int main(int argc, char **argv)
clear_tuning();
reset_display();
- clean_open_devices();
- clear_all_devices();
- clear_all_cpus();
+ clean_shutdown();
return 0;
}
--
2.1.2
11:59 p.m.
New subject: [Powertop] [PATCH v2 8/8] Adding a clean_shutdown function
On (10/14/14 11:09), Joe Konno wrote:
src/main.cpp | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/main.cpp b/src/main.cpp
index d3963ed..b6ce090 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -368,6 +368,17 @@ static void powertop_init(void)
initialized = 1;
}
+void clean_shutdown()
+{
+ close_results();
+ close_display();
+ clean_open_devices();
+ clear_all_devices();
+ clear_all_cpus();
+
+ return;
+}
+
I would agree on that refactoring if we would call this function
from several places. otherwise, I see no actual reason.
-ss
int main(int argc, char **argv)
{
@@ -498,9 +509,7 @@ int main(int argc, char **argv)
clear_tuning();
reset_display();
- clean_open_devices();
- clear_all_devices();
- clear_all_cpus();
+ clean_shutdown();
return 0;
}
--
2.1.2
_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
4:34 a.m.
New subject: [Powertop] [PATCH v2 8/8] Adding a clean_shutdown function
On 10/15/2014 05:59 AM, Sergey Senozhatsky wrote:
I would agree on that refactoring if we would call this function
from several places. otherwise, I see no actual reason.
-ss
I'm sympathetic to that argument. I haven't a strong feeling one way or
the other in this instance, though.
Alex and maintainers?
9:58 a.m.
New subject: [Powertop] [PATCH v3 0/5] A series of patches towards limiting memory leaks
From: Joe Konno <joe.konno(a)intel.com>
Changes since v1:
* Rebased atop "41c54e8 allow none-ncurses messaging..."
* Removed double-free in original patch 3/8
Changes since v2:
* Modified cover letter subject to bring focus to memory leaks
* Removed buffer overflow protection patches, to be re-submitted
separately
The three buffer overflow protection patches in the original series from
Dan were removed in this submission, leaving only the five (mostly)
memory leak patches.
The buffer overflow protection patches, which had been committed (and
reverted) before the v2.6 release was tagged, replaced some calls to
sprintf(), strcpy(), and friends with their 'n' functions. That alone
is insufficient and potentially harmful. Buffer overflow protection
deserves consideration appropriate to the privilege level necessary to
run PowerTOP.
The remaining five patches from the original series from Dan deal
(mostly) with memory leaks.
Sergey and Arjan, thanks for your v2 reviews and discussion!
Dan Kalowsky (5):
Properly cleaning up the display tabs
Removing a documented memory leak
Remove another memory leak
Mark pretty_print_init as initialized
Adding a clean_shutdown function
src/display.cpp | 11 +++++++++++
src/display.h | 1 +
src/lib.cpp | 2 ++
src/main.cpp | 15 ++++++++++++---
src/parameters/parameters.h | 1 +
src/parameters/persistent.cpp | 15 +++++++++++++++
src/tuning/tuning.cpp | 7 +++++++
7 files changed, 49 insertions(+), 3 deletions(-)
--
2.1.2
9:58 a.m.
New subject: [Powertop] [PATCH v3 1/5] Properly cleaning up the display tabs
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
This patch properly cleans up the display tabs so that none of the tab
data is being leaked between refresh changes
Signed-off-by: Dan Kalowsky <daniel.kalowsky(a)intel.com>
v2: rebased atop 41c54e8; there's a bug here
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/display.cpp | 11 +++++++++++
src/display.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/src/display.cpp b/src/display.cpp
index 123620a..e227e74 100644
--- a/src/display.cpp
+++ b/src/display.cpp
@@ -87,6 +87,17 @@ void reset_display(void)
resetterm();
}
+void close_display(void)
+{
+ for (unsigned int i = 0; i < tab_windows.size(); i++) {
+ if (tab_windows[tab_names[i]])
+ delete tab_windows[tab_names[i]];
+ tab_windows[tab_names[i]] = NULL;
+ }
+
+ tab_names.clear();
+}
+
WINDOW *tab_bar = NULL;
WINDOW *bottom_line = NULL;
diff --git a/src/display.h b/src/display.h
index 9af3ec0..c3e8d61 100644
--- a/src/display.h
+++ b/src/display.h
@@ -34,6 +34,7 @@ using namespace std;
extern void init_display(void);
extern void reset_display(void);
+extern void close_display(void);
extern int ncurses_initialized(void);
extern void show_tab(unsigned int tab);
extern void show_next_tab(void);
--
2.1.2
9:58 a.m.
New subject: [Powertop] [PATCH v3 2/5] Removing a documented memory leak
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
A non-elegant solution to it, but it works well enough to remove the
documented memory leak. Proves to provide a much larger impact on
memory constrained systems.
v2: rebased atop 41c54e8
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/parameters/parameters.h | 1 +
src/parameters/persistent.cpp | 15 +++++++++++++++
2 files changed, 16 insertions(+)
diff --git a/src/parameters/parameters.h b/src/parameters/parameters.h
index bd9fee8..72f8d32 100644
--- a/src/parameters/parameters.h
+++ b/src/parameters/parameters.h
@@ -100,6 +100,7 @@ extern void store_results(double duration);
extern void learn_parameters(int iterations, int do_base_power);
extern char *get_param_directory(const char *filename);
extern void save_all_results(const char *filename = "saved_results.powertop");
+extern void close_results(void);
extern void load_results(const char *filename);
extern void save_parameters(const char *filename);
extern void load_parameters(const char *filename);
diff --git a/src/parameters/persistent.cpp b/src/parameters/persistent.cpp
index 483227b..9a5688a 100644
--- a/src/parameters/persistent.cpp
+++ b/src/parameters/persistent.cpp
@@ -61,6 +61,16 @@ void save_all_results(const char *filename)
}
+void close_results()
+{
+ for (unsigned int i = 0; i < past_results.size(); i++) {
+ delete past_results[i];
+ }
+
+ past_results.clear();
+ return;
+}
+
void load_results(const char *filename)
{
ifstream file;
@@ -70,6 +80,7 @@ void load_results(const char *filename)
int first = 1;
unsigned int count = 0;
char* pathname;
+ int bundle_saved = 0;
pathname = get_param_directory(filename);
@@ -97,6 +108,7 @@ void load_results(const char *filename)
if (strlen(line) < 3) {
int overflow_index;
+ bundle_saved = 1;
overflow_index = 50 + (rand() % MAX_KEEP);
if (past_results.size() >= MAX_PARAM) {
/* memory leak, must free old one first */
@@ -118,6 +130,9 @@ void load_results(const char *filename)
set_result_value(line, d, bundle);
}
+ if (bundle_saved == 0)
+ delete bundle;
+
file.close();
// '%i" is for count, do not translate
fprintf(stderr, _("Loaded %i prior measurements\n"), count);
--
2.1.2
9:58 a.m.
New subject: [Powertop] [PATCH v3 3/5] Remove another memory leak
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
Pointer gets over written without ever being properly cleaned up. This
should solve that problem.
v2: rebased atop 41c54e8, do not delete window in clear_tunables() to
prevent double-free in close_display().
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/tuning/tuning.cpp | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/tuning/tuning.cpp b/src/tuning/tuning.cpp
index 7d64cad..5414022 100644
--- a/src/tuning/tuning.cpp
+++ b/src/tuning/tuning.cpp
@@ -46,6 +46,8 @@
static void sort_tunables(void);
static bool should_clear = false;
+class tuning_window *tune_window;
+
class tuning_window: public tab_window {
public:
virtual void repaint(void);
@@ -80,6 +82,11 @@ void initialize_tuning(void)
init_tuning();
w->cursor_max = all_tunables.size() - 1;
+
+ if (tune_window)
+ delete tune_window;
+
+ tune_window = w;
}
--
2.1.2
9:58 a.m.
New subject: [Powertop] [PATCH v3 4/5] Mark pretty_print_init as initialized
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
There is a check for it, but it's never actually set. Now it's set.
v2: rebased atop 41c54e8
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/lib.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/lib.cpp b/src/lib.cpp
index cce7973..437803b 100644
--- a/src/lib.cpp
+++ b/src/lib.cpp
@@ -408,6 +408,8 @@ static void init_pretty_print(void)
pretty_prints["[12] i8042"] = _("PS/2 Touchpad / Keyboard /
Mouse");
pretty_prints["ahci"] = _("SATA controller");
pretty_prints["usb-device-8087-0020"] = _("Intel built in USB
hub");
+
+ pretty_print_init = 1;
}
--
2.1.2
9:58 a.m.
New subject: [Powertop] [PATCH v3 5/5] Adding a clean_shutdown function
From: Dan Kalowsky <daniel.kalowsky(a)intel.com>
Function is designed to be a on stop place to clean up the entire
application at shutdown time.
v2: rebased atop 41c54e8
Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
src/main.cpp | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/main.cpp b/src/main.cpp
index d3963ed..b6ce090 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -368,6 +368,17 @@ static void powertop_init(void)
initialized = 1;
}
+void clean_shutdown()
+{
+ close_results();
+ close_display();
+ clean_open_devices();
+ clear_all_devices();
+ clear_all_cpus();
+
+ return;
+}
+
int main(int argc, char **argv)
{
@@ -498,9 +509,7 @@ int main(int argc, char **argv)
clear_tuning();
reset_display();
- clean_open_devices();
- clear_all_devices();
- clear_all_cpus();
+ clean_shutdown();
return 0;
}
--
2.1.2
6:07 a.m.
New subject: [Powertop] [PATCH v3 0/5] A series of patches towards limiting memory leaks
From: Joe Konno <joe.konno(a)intel.com>
Changes since v1:
* Rebased atop "41c54e8 allow none-ncurses messaging..."
* Removed double-free in original patch 3/8
Changes since v2:
* Modified cover letter subject to bring focus to memory leaks
* Removed buffer overflow protection patches, to be re-submitted
separately
The three buffer overflow protection patches in the original series from
Dan were removed in this submission, leaving only the five (mostly)
memory leak patches.
The buffer overflow protection patches, which had been committed (and
reverted) before the v2.6 release was tagged, replaced some calls to
sprintf(), strcpy(), and friends with their 'n' functions. That alone
is insufficient and potentially harmful. Buffer overflow protection
deserves consideration appropriate to the privilege level necessary to
run PowerTOP.
The remaining five patches from the original series from Dan deal
(mostly) with memory leaks.
Sergey and Arjan, thanks for your v2 reviews and discussion!
Dan Kalowsky (5):
Properly cleaning up the display tabs
Removing a documented memory leak
Remove another memory leak
Mark pretty_print_init as initialized
Adding a clean_shutdown function
src/display.cpp | 11 +++++++++++
src/display.h | 1 +
src/lib.cpp | 2 ++
src/main.cpp | 15 ++++++++++++---
src/parameters/parameters.h | 1 +
src/parameters/persistent.cpp | 15 +++++++++++++++
src/tuning/tuning.cpp | 7 +++++++
7 files changed, 49 insertions(+), 3 deletions(-)
--
2.1.2
_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
Joe,
Your patches were added today.
Thank you,
Alexandra.
2888
days inactive
3063
days old
40 comments
6 participants
participants (6)
-
Alexandra Yates -
Arjan van de Ven -
Joe Konno -
Kalowsky, Daniel -
Magnus Fromreide -
Sergey Senozhatsky