Re: [Powertop] [PATCH 1/2] perf_bundle: Check memory allocation failure

Re: [Powertop] [PATCH 1/2] perf_bundle: Check memory allocation failure

[Arjan van de Ven]

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

On 9/18/2012 7:21 AM, Namhyung Kim wrote:
> Check return value of malloc/strdup not to make NULL dereferences.

I don't mind these patches; they are clean code

however, one of the core userspace developers at work basically said "we don't check for malloc NULL in userspace;
if that ever happens the system is so screwed up anyway that you just cannot continue".



> 
> Signed-off-by: Namhyung Kim <namhyung(a)gmail.com>
> ---
>  src/perf/perf_bundle.cpp |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/src/perf/perf_bundle.cpp b/src/perf/perf_bundle.cpp
> index 38e1e91..4e70f30 100644
> --- a/src/perf/perf_bundle.cpp
> +++ b/src/perf/perf_bundle.cpp
> @@ -60,6 +60,9 @@ void perf_bundle_event::handle_event(struct perf_event_header *header, void *coo
>  	vector<void *> *vector;
>  
>  	buffer = (unsigned char *)malloc(header->size);
> +	if (!buffer)
> +		return;
> +
>  	memcpy(buffer, header, header->size);
>  
>  #ifdef USE_DECLTYPE
> @@ -130,6 +133,9 @@ static void parse_event_format(const char *event_name)
>  {
>  	char *tptr;
>  	char *name = strdup(event_name);
> +	if (!name)
> +		return;
> +
>  	char *sys = strtok_r(name, ":", &tptr);
>  	char *event = strtok_r(NULL, ":", &tptr);
>  	char *file;
> @@ -137,6 +143,9 @@ static void parse_event_format(const char *event_name)
>  
>  	file = (char *)malloc(strlen(sys) + strlen(event) +
>  		      strlen("/sys/kernel/debug/tracing/events////format") + 2);
> +	if (!file)
> +		return;
> +
>  	sprintf(file, "/sys/kernel/debug/tracing/events/%s/%s/format", sys, event);
>  
>  	buf = read_file(file);
> @@ -169,6 +178,10 @@ bool perf_bundle::add_event(const char *event_name)
>  		if ((int)ev->trace_type >= 0) {
>  			if (event_names.find(ev->trace_type) == event_names.end()) {
>  				event_names[ev->trace_type] = strdup(event_name);
> +				if (!event_names[ev->trace_type]) {
> +					delete ev;
> +					continue;
> +				}
>  				parse_event_format(event_name);
>  			}
>  			events.push_back(ev);
> 

Re: [Powertop] [PATCH 1/2] perf_bundle: Check memory allocation failure

[Namhyung Kim]

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

2012-09-18 (화), 07:25 -0700, Arjan van de Ven:
> On 9/18/2012 7:21 AM, Namhyung Kim wrote:
> > Check return value of malloc/strdup not to make NULL dereferences.
> 
> I don't mind these patches; they are clean code
> 
> however, one of the core userspace developers at work basically said "we don't check for malloc NULL in userspace;
> if that ever happens the system is so screwed up anyway that you just cannot continue".

Thanks for the quick reply. :)

Basically I agree with you.  But even in that case it'd be better
letting a user know about current situation somehow rather than just
segfault.  Maybe by replacing them to xmalloc or so?

Thanks,
Namhyung

[Powertop] [PATCH 1/2] perf_bundle: Check memory allocation failure

[Namhyung Kim]

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

Check return value of malloc/strdup not to make NULL dereferences.

Signed-off-by: Namhyung Kim <namhyung(a)gmail.com>
---
 src/perf/perf_bundle.cpp |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/perf/perf_bundle.cpp b/src/perf/perf_bundle.cpp
index 38e1e91..4e70f30 100644
--- a/src/perf/perf_bundle.cpp
+++ b/src/perf/perf_bundle.cpp
@@ -60,6 +60,9 @@ void perf_bundle_event::handle_event(struct perf_event_header *header, void *coo
 	vector<void *> *vector;
 
 	buffer = (unsigned char *)malloc(header->size);
+	if (!buffer)
+		return;
+
 	memcpy(buffer, header, header->size);
 
 #ifdef USE_DECLTYPE
@@ -130,6 +133,9 @@ static void parse_event_format(const char *event_name)
 {
 	char *tptr;
 	char *name = strdup(event_name);
+	if (!name)
+		return;
+
 	char *sys = strtok_r(name, ":", &tptr);
 	char *event = strtok_r(NULL, ":", &tptr);
 	char *file;
@@ -137,6 +143,9 @@ static void parse_event_format(const char *event_name)
 
 	file = (char *)malloc(strlen(sys) + strlen(event) +
 		      strlen("/sys/kernel/debug/tracing/events////format") + 2);
+	if (!file)
+		return;
+
 	sprintf(file, "/sys/kernel/debug/tracing/events/%s/%s/format", sys, event);
 
 	buf = read_file(file);
@@ -169,6 +178,10 @@ bool perf_bundle::add_event(const char *event_name)
 		if ((int)ev->trace_type >= 0) {
 			if (event_names.find(ev->trace_type) == event_names.end()) {
 				event_names[ev->trace_type] = strdup(event_name);
+				if (!event_names[ev->trace_type]) {
+					delete ev;
+					continue;
+				}
 				parse_event_format(event_name);
 			}
 			events.push_back(ev);
-- 
1.7.9.2