[PATCH v2 1/3] kernel-shark: Fix simple typo in the "File" menu.

[PATCH v2 1/3] kernel-shark: Fix simple typo in the "File" menu.

[Yordan Karadzhov (VMware)]

Reported-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsMainWindow.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index a583f5f..3402764 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -45,7 +45,7 @@ KsMainWindow::KsMainWindow(QWidget *parent)
   _openAction("Open", this),
   _restoreSessionAction("Restore Last Session", this),
   _importSessionAction("Import Session", this),
-  _exportSessionAction("Export Sassion", this),
+  _exportSessionAction("Export Session", this),
   _quitAction("Quit", this),
   _importFilterAction("Import Filter", this),
   _exportFilterAction("Export Filter", this),
-- 
2.20.1

[PATCH v2 2/3] kernel-shark: Fix potential memory leak in libkshark-collection

[Yordan Karadzhov (VMware)]

When searching for the entry, do not loop over the original list of
requests. Use a copy instead. If we loop over the original list and
no entry is found in the first element of the list, later the memory
used for this first element will leak.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/libkshark-collection.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel-shark/src/libkshark-collection.c b/kernel-shark/src/libkshark-collection.c
index 02a014e..95fdbab 100644
--- a/kernel-shark/src/libkshark-collection.c
+++ b/kernel-shark/src/libkshark-collection.c
@@ -622,6 +622,7 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
 				  ssize_t *index)
 {
 	const struct kshark_entry *entry = NULL;
+	struct kshark_entry_request *list;
 	int req_count;
 
 	/*
@@ -638,12 +639,10 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
 	 * Loop over the list of redefined requests and search until you find
 	 * the first matching entry.
 	 */
-	while (*req) {
-		entry = kshark_get_entry_front(*req, data, index);
+	for (list = *req; list; list = list->next) {
+		entry = kshark_get_entry_front(list, data, index);
 		if (entry)
 			break;
-
-		*req = (*req)->next;
 	}
 
 	return entry;
@@ -680,6 +679,7 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
 				 ssize_t *index)
 {
 	const struct kshark_entry *entry = NULL;
+	struct kshark_entry_request *list;
 	int req_count;
 
 	/*
@@ -695,12 +695,10 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
 	 * Loop over the list of redefined requests and search until you find
 	 * the first matching entry.
 	 */
-	while (*req) {
-		entry = kshark_get_entry_back(*req, data, index);
+	for (list = *req; list; list = list->next) {
+		entry = kshark_get_entry_back(list, data, index);
 		if (entry)
 			break;
-
-		*req = (*req)->next;
 	}
 
 	return entry;
-- 
2.20.1

Re: [PATCH v2 2/3] kernel-shark: Fix potential memory leak in libkshark-collection

[Steven Rostedt]

On Wed, 23 Oct 2019 15:21:44 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> When searching for the entry, do not loop over the original list of
> requests. Use a copy instead. If we loop over the original list and
> no entry is found in the first element of the list, later the memory
> used for this first element will leak.
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark/src/libkshark-collection.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel-shark/src/libkshark-collection.c b/kernel-shark/src/libkshark-collection.c
> index 02a014e..95fdbab 100644
> --- a/kernel-shark/src/libkshark-collection.c
> +++ b/kernel-shark/src/libkshark-collection.c
> @@ -622,6 +622,7 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
>  				  ssize_t *index)
>  {
>  	const struct kshark_entry *entry = NULL;
> +	struct kshark_entry_request *list;

Hi Yordan,

I was looking at this patch in more detail, and I'm thinking that we
don't need to pass in the address of the req pointer, but just the req
pointer itself. The only place that I see the req pointer being
modified is the failure case in map_collection_request_init() where it
does:

	kshark_free_entry_request(*req);
	*req = NULL;

But all callers do that free anyway.

Maybe I'm missing something, but why are we passing in the pointer to
the pointer of req, and not just the req pointer itself? I don't see a
need to modify the pointer.

Before this patch, *req is modified, but after this patch, it is not.
If you pass in just "struct kshark_entry_request *req" then you don't
even need to have the "list" variable, you could just use "req" because
that would be a copy of the pointer.

-- Steve



>  	int req_count;
>  
>  	/*
> @@ -638,12 +639,10 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
>  	 * Loop over the list of redefined requests and search until you find
>  	 * the first matching entry.
>  	 */
> -	while (*req) {
> -		entry = kshark_get_entry_front(*req, data, index);
> +	for (list = *req; list; list = list->next) {
> +		entry = kshark_get_entry_front(list, data, index);
>  		if (entry)
>  			break;
> -
> -		*req = (*req)->next;
>  	}
>  
>  	return entry;
> @@ -680,6 +679,7 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
>  				 ssize_t *index)
>  {
>  	const struct kshark_entry *entry = NULL;
> +	struct kshark_entry_request *list;
>  	int req_count;
>  
>  	/*
> @@ -695,12 +695,10 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
>  	 * Loop over the list of redefined requests and search until you find
>  	 * the first matching entry.
>  	 */
> -	while (*req) {
> -		entry = kshark_get_entry_back(*req, data, index);
> +	for (list = *req; list; list = list->next) {
> +		entry = kshark_get_entry_back(list, data, index);
>  		if (entry)
>  			break;
> -
> -		*req = (*req)->next;
>  	}
>  
>  	return entry;

Re: [PATCH v2 2/3] kernel-shark: Fix potential memory leak in libkshark-collection

[Yordan Karadzhov (VMware)]

On 27.11.19 г. 20:54 ч., Steven Rostedt wrote:
> On Wed, 23 Oct 2019 15:21:44 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> When searching for the entry, do not loop over the original list of
>> requests. Use a copy instead. If we loop over the original list and
>> no entry is found in the first element of the list, later the memory
>> used for this first element will leak.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   kernel-shark/src/libkshark-collection.c | 14 ++++++--------
>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel-shark/src/libkshark-collection.c b/kernel-shark/src/libkshark-collection.c
>> index 02a014e..95fdbab 100644
>> --- a/kernel-shark/src/libkshark-collection.c
>> +++ b/kernel-shark/src/libkshark-collection.c
>> @@ -622,6 +622,7 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
>>   				  ssize_t *index)
>>   {
>>   	const struct kshark_entry *entry = NULL;
>> +	struct kshark_entry_request *list;
> 
> Hi Yordan,
> 
> I was looking at this patch in more detail, and I'm thinking that we
> don't need to pass in the address of the req pointer, but just the req
> pointer itself. The only place that I see the req pointer being
> modified is the failure case in map_collection_request_init() where it
> does:
> 
> 	kshark_free_entry_request(*req);
> 	*req = NULL;
> 
> But all callers do that free anyway.
>

Yes, because the caller is expected to do 
kshark_free_entry_request(*req) at the end, here we have to set the 
original pointer to NULL. Otherwise we will get double free error.
I think this is what I have been trying to fix, when I introduced the 
memory leak.

And yes, I agree with you that carrying the address of the pointer 
through all these functions is a bit ugly.

Thanks!
Yordan

> Maybe I'm missing something, but why are we passing in the pointer to
> the pointer of req, and not just the req pointer itself? I don't see a
> need to modify the pointer.
> 
> Before this patch, *req is modified, but after this patch, it is not.
> If you pass in just "struct kshark_entry_request *req" then you don't
> even need to have the "list" variable, you could just use "req" because
> that would be a copy of the pointer.
> 
> -- Steve
> 
> 
> 
>>   	int req_count;
>>   
>>   	/*
>> @@ -638,12 +639,10 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
>>   	 * Loop over the list of redefined requests and search until you find
>>   	 * the first matching entry.
>>   	 */
>> -	while (*req) {
>> -		entry = kshark_get_entry_front(*req, data, index);
>> +	for (list = *req; list; list = list->next) {
>> +		entry = kshark_get_entry_front(list, data, index);
>>   		if (entry)
>>   			break;
>> -
>> -		*req = (*req)->next;
>>   	}
>>   
>>   	return entry;
>> @@ -680,6 +679,7 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
>>   				 ssize_t *index)
>>   {
>>   	const struct kshark_entry *entry = NULL;
>> +	struct kshark_entry_request *list;
>>   	int req_count;
>>   
>>   	/*
>> @@ -695,12 +695,10 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
>>   	 * Loop over the list of redefined requests and search until you find
>>   	 * the first matching entry.
>>   	 */
>> -	while (*req) {
>> -		entry = kshark_get_entry_back(*req, data, index);
>> +	for (list = *req; list; list = list->next) {
>> +		entry = kshark_get_entry_back(list, data, index);
>>   		if (entry)
>>   			break;
>> -
>> -		*req = (*req)->next;
>>   	}
>>   
>>   	return entry;
> 

[PATCH v2 3/3] kernel-shark: When running as Root save all config settings in /root/

[Yordan Karadzhov (VMware)]

If KernelShark is running with Root privileges, do not save the settings
in the standard location. Otherwise the configuration files will be owned
by Root and later the normal user will have no access to those files.

The patch seems to do the right thing in all cases that I tested, however
there is definitely something that I do not understand. QDir::homePath()
always returns the path to the home of the normal user, even if I build
and run kernelshark as root (sudo -s).

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsMainWindow.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 3402764..bd6c338 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -69,7 +69,7 @@ KsMainWindow::KsMainWindow(QWidget *parent)
   _contentsAction("Contents", this),
   _bugReportAction("Report a bug", this),
   _deselectShortcut(this),
-  _settings("kernelshark.org", "Kernel Shark") // organization , application
+  _settings(_getCacheDir() + "/setting.ini", QSettings::IniFormat)
 {
 	setWindowTitle("Kernel Shark");
 	_createActions();
@@ -431,6 +431,9 @@ QString KsMainWindow::_getCacheDir()
 		dir = QStandardPaths::writableLocation(appCachePath);
 		dir += "/kernelshark";
 
+		if (geteuid() == 0)
+			dir.replace(QDir::homePath(), "/root");
+
 		if (!QDir(dir).exists())
 			lamMakePath(false);
 	}
-- 
2.20.1

Re: [PATCH v2 3/3] kernel-shark: When running as Root save all config settings in /root/

[Steven Rostedt]

On Wed, 23 Oct 2019 15:21:45 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> If KernelShark is running with Root privileges, do not save the settings
> in the standard location. Otherwise the configuration files will be owned
> by Root and later the normal user will have no access to those files.
> 
> The patch seems to do the right thing in all cases that I tested, however
> there is definitely something that I do not understand. QDir::homePath()
> always returns the path to the home of the normal user, even if I build
> and run kernelshark as root (sudo -s).
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark/src/KsMainWindow.cpp | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index 3402764..bd6c338 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -69,7 +69,7 @@ KsMainWindow::KsMainWindow(QWidget *parent)
>    _contentsAction("Contents", this),
>    _bugReportAction("Report a bug", this),
>    _deselectShortcut(this),
> -  _settings("kernelshark.org", "Kernel Shark") // organization , application
> +  _settings(_getCacheDir() + "/setting.ini", QSettings::IniFormat)
>  {
>  	setWindowTitle("Kernel Shark");
>  	_createActions();
> @@ -431,6 +431,9 @@ QString KsMainWindow::_getCacheDir()
>  		dir = QStandardPaths::writableLocation(appCachePath);
>  		dir += "/kernelshark";
>  
> +		if (geteuid() == 0)
> +			dir.replace(QDir::homePath(), "/root");
> +
>  		if (!QDir(dir).exists())
>  			lamMakePath(false);
>  	}

I'll pull this patch in, but this assumes that root is always at /root.
I've had machines where that was not the case. I wonder if we should
add something like this on top of this patch. Not this change directly,
(because this is me just writing C with at C++ compiler ;-), but
something that is more the Qt way...

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index bd6c338f..56cf9b9b 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -31,6 +31,43 @@
 #include "KsCaptureDialog.hpp"
 #include "KsAdvFilteringDialog.hpp"
 
+static QString find_root_home(void)
+{
+	FILE *fp = fopen("/etc/passwd", "r");
+	char *buf;
+	char *sav;
+	char *id;
+	size_t n;
+
+	if (!fp)
+		return QString("/root");
+
+	n = 0;
+	while (getline(&buf, &n, fp) != -1) {
+		/* user */
+		strtok_r(buf, ":", &sav);
+		/* type */
+		strtok_r(NULL, ":", &sav);
+		/* pid */
+		id = strtok_r(NULL, ":", &sav);
+		if (atoi(id) != 0) {
+			n = 0;
+			free(buf);
+			continue;
+		}
+		/* gid */
+		strtok_r(NULL, ":", &sav);
+		/* group */
+		strtok_r(NULL, ":", &sav);
+		/* home */
+		QString ret = QString(strtok_r(NULL, ":", &sav));
+		free(buf);
+		return ret;
+	}
+	free(buf);
+	return QString("/root");
+}
+
 /** Create KernelShark Main window. */
 KsMainWindow::KsMainWindow(QWidget *parent)
 : QMainWindow(parent),
@@ -432,7 +469,8 @@ QString KsMainWindow::_getCacheDir()
 		dir += "/kernelshark";
 
 		if (geteuid() == 0)
-			dir.replace(QDir::homePath(), "/root");
+			dir.replace(QDir::homePath(),
+				    find_root_home().toStdString().c_str());
 
 		if (!QDir(dir).exists())
 			lamMakePath(false);

This reads the /etc/passwd file and searches for the pid of 0, and
returns the home path for that user. On any error it just
quietly defaults back to "/root".

-- Steve

Re: [PATCH v2 3/3] kernel-shark: When running as Root save all config settings in /root/

[Yordan Karadzhov (VMware)]

On 27.11.19 г. 22:13 ч., Steven Rostedt wrote:
> On Wed, 23 Oct 2019 15:21:45 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> If KernelShark is running with Root privileges, do not save the settings
>> in the standard location. Otherwise the configuration files will be owned
>> by Root and later the normal user will have no access to those files.
>>
>> The patch seems to do the right thing in all cases that I tested, however
>> there is definitely something that I do not understand. QDir::homePath()
>> always returns the path to the home of the normal user, even if I build
>> and run kernelshark as root (sudo -s).
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   kernel-shark/src/KsMainWindow.cpp | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
>> index 3402764..bd6c338 100644
>> --- a/kernel-shark/src/KsMainWindow.cpp
>> +++ b/kernel-shark/src/KsMainWindow.cpp
>> @@ -69,7 +69,7 @@ KsMainWindow::KsMainWindow(QWidget *parent)
>>     _contentsAction("Contents", this),
>>     _bugReportAction("Report a bug", this),
>>     _deselectShortcut(this),
>> -  _settings("kernelshark.org", "Kernel Shark") // organization , application
>> +  _settings(_getCacheDir() + "/setting.ini", QSettings::IniFormat)
>>   {
>>   	setWindowTitle("Kernel Shark");
>>   	_createActions();
>> @@ -431,6 +431,9 @@ QString KsMainWindow::_getCacheDir()
>>   		dir = QStandardPaths::writableLocation(appCachePath);
>>   		dir += "/kernelshark";
>>   
>> +		if (geteuid() == 0)
>> +			dir.replace(QDir::homePath(), "/root");
>> +
>>   		if (!QDir(dir).exists())
>>   			lamMakePath(false);
>>   	}
> 
> I'll pull this patch in, but this assumes that root is always at /root.
> I've had machines where that was not the case. I wonder if we should
> add something like this on top of this patch. Not this change directly,
> (because this is me just writing C with at C++ compiler ;-), but
> something that is more the Qt way...
> 
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index bd6c338f..56cf9b9b 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -31,6 +31,43 @@
>   #include "KsCaptureDialog.hpp"
>   #include "KsAdvFilteringDialog.hpp"
>   
> +static QString find_root_home(void)
> +{
> +	FILE *fp = fopen("/etc/passwd", "r");
> +	char *buf;
> +	char *sav;
> +	char *id;
> +	size_t n;
> +
> +	if (!fp)
> +		return QString("/root");
> +
> +	n = 0;
> +	while (getline(&buf, &n, fp) != -1) {
> +		/* user */
> +		strtok_r(buf, ":", &sav);
> +		/* type */
> +		strtok_r(NULL, ":", &sav);
> +		/* pid */
> +		id = strtok_r(NULL, ":", &sav);
> +		if (atoi(id) != 0) {
> +			n = 0;
> +			free(buf);
> +			continue;
> +		}
> +		/* gid */
> +		strtok_r(NULL, ":", &sav);
> +		/* group */
> +		strtok_r(NULL, ":", &sav);
> +		/* home */
> +		QString ret = QString(strtok_r(NULL, ":", &sav));
> +		free(buf);
> +		return ret;
> +	}
> +	free(buf);
> +	return QString("/root");
> +}
> +
>   /** Create KernelShark Main window. */
>   KsMainWindow::KsMainWindow(QWidget *parent)
>   : QMainWindow(parent),
> @@ -432,7 +469,8 @@ QString KsMainWindow::_getCacheDir()
>   		dir += "/kernelshark";
>   
>   		if (geteuid() == 0)
> -			dir.replace(QDir::homePath(), "/root");
> +			dir.replace(QDir::homePath(),
> +				    find_root_home().toStdString().c_str());
>   
>   		if (!QDir(dir).exists())
>   			lamMakePath(false);
> 
> This reads the /etc/passwd file and searches for the pid of 0, and
> returns the home path for that user. On any error it just
> quietly defaults back to "/root".
> 

Hi Steven,

Very good point. Thanks a lot!

I am sending a Qt-ish looking patch ;)

cheers,
Yordan


> -- Steve
>