Re: [Powertop] Seg Fault due to function format_watts in file src/lib.cpp

[Sergey Senozhatsky <sergey.senozhatsky at gmail.com>]

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

On (02/28/13 14:45), Ganapati Bhat wrote:
> Date: Thu, 28 Feb 2013 14:45:04 +0530
> From: Ganapati Bhat <life.mytake(a)gmail.com>
> To: powertop(a)lists.01.org
> Subject: [Powertop] Seg Fault due to function format_watts in file
>  src/lib.cpp
> 
>    Dear All,
>    I was compiling powertop as an Android external module and using the
>    option powertop --html. However, there used to be a segmentation fault.
>    When i looked into it, I observed that in the function format_watts in
>    src/lib.cpp, the last�argument�to mbstows was zero. By changing the
>    argument to len, which is itself an argument to the function format_watts,
>    powertop worked properly? Does this also happen on an x86 machine? ( I am
>    unable to verify this due to some problems in my laptop which I have not
>    yet resolved.) If not then can someone explain to me why is it happening
>    in the case of android?
>    Thanks and Regards,
>    Ganapati

could you try the following patch?

------8<-------8<---------


Use

size_t mbsrtowcs (wchar t *restrict dst, const char **restrict src,
        size t len, mbstate t *restrict ps)

which explicitly handles dst == NULL case:

	If dst is not a null pointer, the result is stored in the array pointed to by dst; otherwise,
	the conversion result is not available since it is stored in an internal buffer.

while

size_t mbstowcs (wchar t *wstring, const char *string, size t size)

does not (at least not in my glibc 2.17 docs), thus is free to cause undefined behavior.

Reported-by: Ganapati Bhat <life.mytake(a)gmail.com>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>

---

 src/lib.cpp                | 25 +++++++++++++++++++++----
 src/lib.h                  |  1 +
 src/process/do_process.cpp | 10 ++++++----
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/src/lib.cpp b/src/lib.cpp
index 723517a..1a2f6c8 100644
--- a/src/lib.cpp
+++ b/src/lib.cpp
@@ -257,22 +257,39 @@ string read_sysfs_string(const char *format, const char *param)
 	return content;
 }
 
+void align_string(char *buffer, size_t min_sz, size_t max_sz)
+{
+	size_t sz;
+
+	/** mbsrtowcs() allows NULL dst and zero sz,
+	 * comparing to mbstowcs(), which causes undefined
+	 * behaviour under given circumstances*/
+
+	/* start with mbsrtowcs() local mbstate_t * and
+	 * NULL dst pointer*/
+	sz = mbsrtowcs(NULL, (const char **)&buffer, max_sz, NULL);
+	if (sz == (size_t)-1) {
+		buffer[min_sz] = 0x00;
+		return;
+	}
+	while (sz < min_sz) {
+		strcat(buffer, " ");
+		sz++;
+	}
+}
 
 void format_watts(double W, char *buffer, unsigned int len)
 {
 	buffer[0] = 0;
 	char buf[32];
-
 	sprintf(buffer, _("%7sW"), fmt_prefix(W, buf));
 
 	if (W < 0.0001)
 		sprintf(buffer, _("    0 mW"));
 
-	while (mbstowcs(NULL,buffer,0) < len)
-		strcat(buffer, " ");
+	align_string(buffer, len, len);
 }
 
-
 #ifndef HAVE_NO_PCI
 static struct pci_access *pci_access;
 
diff --git a/src/lib.h b/src/lib.h
index de2de5a..867f480 100644
--- a/src/lib.h
+++ b/src/lib.h
@@ -75,4 +75,5 @@ extern void process_directory(const char *d_name, callback fn);
 extern int utf_ok;
 extern int get_user_input(char *buf, unsigned sz);
 
+extern void align_string(char *buffer, size_t min_sz, size_t max_sz);
 #endif
diff --git a/src/process/do_process.cpp b/src/process/do_process.cpp
index 322ad4c..4aa1a92 100644
--- a/src/process/do_process.cpp
+++ b/src/process/do_process.cpp
@@ -852,13 +852,13 @@ void process_update_display(void)
 		char usage[20];
 		char events[20];
 		char descr[128];
-		format_watts(all_power[i]->Witts(), power, 10);
 
+		format_watts(all_power[i]->Witts(), power, 10);
 		if (!show_power)
 			strcpy(power, "          ");
 		sprintf(name, "%s", all_power[i]->type());
-		while (mbstowcs(NULL,name,0) < 14) strcat(name, " ");
 
+		align_string(name, 14, 20);
 
 		if (all_power[i]->events() == 0 && all_power[i]->usage() == 0 && all_power[i]->Witts() == 0)
 			break;
@@ -870,14 +870,16 @@ void process_update_display(void)
 			else
 				sprintf(usage, "%5i%s", (int)all_power[i]->usage(), all_power[i]->usage_units());
 		}
-		while (mbstowcs(NULL,usage,0) < 14) strcat(usage, " ");
+
+		align_string(usage, 14, 20);
+
 		sprintf(events, "%5.1f", all_power[i]->events());
 		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());
 
-		while (strlen(events) < 12) strcat(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));
 	}
 }