[PATCH 0/5] trace-cmd: Fixes for four small bugs plus minor cleanup

[PATCH 0/5] trace-cmd: Fixes for four small bugs plus minor cleanup

[Michael Sartain]

Thanks much.
 -Mike

---

Michael Sartain (5):
  trace-cmd: Fix incorrect malloc size arg: *item instead of item
  trace-cmd: Fix NULL pointer being passed to memcpy
  trace-cmd: Add ULL suffix to MISSING_EVENTS since ints shouldn't be
    left shifted by 31
  trace-cmd: Use unsigned values in Hsieh's trace_hash fast hash
    function
  trace-cmd: Remove unused view_width variable

 kbuffer-parse.c    | 4 ++--
 trace-dialog.c     | 2 +-
 trace-graph.c      | 2 --
 trace-hash-local.h | 4 ++--
 trace-output.c     | 6 +++++-
 5 files changed, 10 insertions(+), 8 deletions(-)

-- 
2.13.2

[PATCH 1/5] trace-cmd: Fix incorrect malloc size arg: *item instead of item

[Michael Sartain]

Signed-off-by: Michael Sartain <mikesart@fastmail.com>
---
 trace-dialog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/trace-dialog.c b/trace-dialog.c
index b5776cc..87e597a 100644
--- a/trace-dialog.c
+++ b/trace-dialog.c
@@ -97,7 +97,7 @@ static void push_cursor(GdkCursor *cursor)
 {
 	struct cursor_stack *item;
 
-	item = malloc_or_die(sizeof(item));
+	item = malloc_or_die(sizeof(*item));
 	item->next = cursor_stack;
 	cursor_stack = item;
 	item->cursor = cursor;
-- 
2.13.2

[PATCH 2/5] trace-cmd: Fix NULL pointer being passed to memcpy

[Michael Sartain]

Signed-off-by: Michael Sartain <mikesart@fastmail.com>
---
 trace-output.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/trace-output.c b/trace-output.c
index bfe6331..84b21b0 100644
--- a/trace-output.c
+++ b/trace-output.c
@@ -929,7 +929,11 @@ tracecmd_add_option(struct tracecmd_output *handle,
 		free(option);
 		return NULL;
 	}
-	memcpy(option->data, data, size);
+
+	/* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass NULL data */
+	if (data)
+		memcpy(option->data, data, size);
+
 	list_add_tail(&option->list, &handle->options);
 
 	return option;
-- 
2.13.2

Re: [PATCH 2/5] trace-cmd: Fix NULL pointer being passed to memcpy

[Steven Rostedt]

On Sat, 12 Aug 2017 11:30:44 -0600
Michael Sartain <mikesart@fastmail.com> wrote:

> Signed-off-by: Michael Sartain <mikesart@fastmail.com>
> ---
>  trace-output.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/trace-output.c b/trace-output.c
> index bfe6331..84b21b0 100644
> --- a/trace-output.c
> +++ b/trace-output.c
> @@ -929,7 +929,11 @@ tracecmd_add_option(struct tracecmd_output *handle,
>  		free(option);
>  		return NULL;
>  	}
> -	memcpy(option->data, data, size);
> +
> +	/* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass NULL data */
> +	if (data)
> +		memcpy(option->data, data, size);

Is this a problem, as when this happens, size should be zero. Does it
crash with data=NULL and size=0, or have you seen size not be zero?

-- Steve

> +
>  	list_add_tail(&option->list, &handle->options);
>  
>  	return option;

Re: [PATCH 2/5] trace-cmd: Fix NULL pointer being passed to memcpy

[Michael Sartain]

On Mon, Oct 09, 2017 at 06:24:32PM -0400, Steven Rostedt wrote:
> On Sat, 12 Aug 2017 11:30:44 -0600
> Michael Sartain <mikesart@fastmail.com> wrote:
> 
> > Signed-off-by: Michael Sartain <mikesart@fastmail.com>
> > ---
> >  trace-output.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/trace-output.c b/trace-output.c
> > index bfe6331..84b21b0 100644
> > --- a/trace-output.c
> > +++ b/trace-output.c
> > @@ -929,7 +929,11 @@ tracecmd_add_option(struct tracecmd_output *handle,
> >  		free(option);
> >  		return NULL;
> >  	}
> > -	memcpy(option->data, data, size);
> > +
> > +	/* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass NULL data */
> > +	if (data)
> > +		memcpy(option->data, data, size);
> 
> Is this a problem, as when this happens, size should be zero. Does it
> crash with data=NULL and size=0, or have you seen size not be zero?

I got an ASAN warning, but you are correct - the size was 0 and it did
not crash.

Re: [PATCH 2/5] trace-cmd: Fix NULL pointer being passed to memcpy

[Steven Rostedt]

On Mon, 9 Oct 2017 16:27:10 -0600
Michael Sartain <mikesart@fastmail.com> wrote:

> > > -	memcpy(option->data, data, size);
> > > +
> > > +	/* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass NULL data */
> > > +	if (data)
> > > +		memcpy(option->data, data, size);  
> > 
> > Is this a problem, as when this happens, size should be zero. Does it
> > crash with data=NULL and size=0, or have you seen size not be zero?  
> 
> I got an ASAN warning, but you are correct - the size was 0 and it did
> not crash.

OK, but it's almost like dividing zero from zero. Can you send another
patch, but this time check if (size) instead of if (data).

-- Steve

[PATCH 3/5] trace-cmd: Add ULL suffix to MISSING_EVENTS since ints shouldn't be left shifted by 31

[Michael Sartain]

Signed-off-by: Michael Sartain <mikesart@fastmail.com>
---
 kbuffer-parse.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kbuffer-parse.c b/kbuffer-parse.c
index 4e6e95e..dde642c 100644
--- a/kbuffer-parse.c
+++ b/kbuffer-parse.c
@@ -24,8 +24,8 @@
 
 #include "kbuffer.h"
 
-#define MISSING_EVENTS (1 << 31)
-#define MISSING_STORED (1 << 30)
+#define MISSING_EVENTS (1ULL << 31)
+#define MISSING_STORED (1ULL << 30)
 
 #define COMMIT_MASK ((1 << 27) - 1)
 
-- 
2.13.2

Re: [PATCH 3/5] trace-cmd: Add ULL suffix to MISSING_EVENTS since ints shouldn't be left shifted by 31

[Steven Rostedt]

On Sat, 12 Aug 2017 11:30:45 -0600
Michael Sartain <mikesart@fastmail.com> wrote:


> Signed-off-by: Michael Sartain <mikesart@fastmail.com>
> ---
>  kbuffer-parse.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kbuffer-parse.c b/kbuffer-parse.c
> index 4e6e95e..dde642c 100644
> --- a/kbuffer-parse.c
> +++ b/kbuffer-parse.c
> @@ -24,8 +24,8 @@
>  
>  #include "kbuffer.h"
>  
> -#define MISSING_EVENTS (1 << 31)

Actually, why not? This could also be just UL, because it's fine to
shift 31, that would give us: 0x80000000  And that bit is all we care
for.

-- Steve

> -#define MISSING_STORED (1 << 30)
> +#define MISSING_EVENTS (1ULL << 31)
> +#define MISSING_STORED (1ULL << 30)
>  
>  #define COMMIT_MASK ((1 << 27) - 1)
>  

[PATCH 4/5] trace-cmd: Use unsigned values in Hsieh's trace_hash fast hash function

[Michael Sartain]

Signed int values were being used where the original code used uint32_t types:

  http://www.azillionmonkeys.com/qed/hash.html

Right shifting negative int values has implementation-defined and left shifting
has undefined behavior.

On my platform (x86_64) right shifting was doing sign extension and filling
high bits with 1s, which is different than the original algorithm.

Signed-off-by: Michael Sartain <mikesart@fastmail.com>
---
 trace-hash-local.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/trace-hash-local.h b/trace-hash-local.h
index b2a1002..b3f9b06 100644
--- a/trace-hash-local.h
+++ b/trace-hash-local.h
@@ -22,7 +22,7 @@
 
 static inline unsigned int trace_hash(int val)
 {
-	int hash, tmp;
+	unsigned int hash, tmp;
 
 	hash = 12546869;	/* random prime */
 
@@ -34,7 +34,7 @@ static inline unsigned int trace_hash(int val)
 	 */
 
 	hash +=	(val & 0xffff);
-	tmp = (val >> 16) ^ hash;
+	tmp = ((unsigned int)val >> 16) ^ hash;
 	hash = (hash << 16) ^ tmp;
 	hash += hash >> 11;
 
-- 
2.13.2

Re: [PATCH 4/5] trace-cmd: Use unsigned values in Hsieh's trace_hash fast hash function

[Steven Rostedt]

On Sat, 12 Aug 2017 11:30:46 -0600
Michael Sartain <mikesart@fastmail.com> wrote:

> Signed int values were being used where the original code used uint32_t types:
> 
>   http://www.azillionmonkeys.com/qed/hash.html
> 
> Right shifting negative int values has implementation-defined and left shifting
> has undefined behavior.
> 
> On my platform (x86_64) right shifting was doing sign extension and filling
> high bits with 1s, which is different than the original algorithm.
> 

Heh, nice catch. Although the hash was never used for anything too
important. Mostly just colors of the graph.

> Signed-off-by: Michael Sartain <mikesart@fastmail.com>
> ---
>  trace-hash-local.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/trace-hash-local.h b/trace-hash-local.h
> index b2a1002..b3f9b06 100644
> --- a/trace-hash-local.h
> +++ b/trace-hash-local.h
> @@ -22,7 +22,7 @@
>  
>  static inline unsigned int trace_hash(int val)
>  {
> -	int hash, tmp;
> +	unsigned int hash, tmp;
>  
>  	hash = 12546869;	/* random prime */
>  
> @@ -34,7 +34,7 @@ static inline unsigned int trace_hash(int val)
>  	 */
>  
>  	hash +=	(val & 0xffff);
> -	tmp = (val >> 16) ^ hash;
> +	tmp = ((unsigned int)val >> 16) ^ hash;

Why not just have val be unsigned int, and not do the typecast?

-- Steve

>  	hash = (hash << 16) ^ tmp;
>  	hash += hash >> 11;
>  

[PATCH 5/5] trace-cmd: Remove unused view_width variable

[Michael Sartain]

Signed-off-by: Michael Sartain <mikesart@fastmail.com>
---
 trace-graph.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/trace-graph.c b/trace-graph.c
index 1db342f..2c49549 100644
--- a/trace-graph.c
+++ b/trace-graph.c
@@ -1263,7 +1263,6 @@ static void draw_info_box(struct graph_info *ginfo, const gchar *buffer,
 	gint width, height;
 	GdkPixmap *pix;
 	static GdkGC *pix_bg;
-	gint view_width;
 	gint view_start;
 
 	if (!pix_bg) {
@@ -1284,7 +1283,6 @@ static void draw_info_box(struct graph_info *ginfo, const gchar *buffer,
 	height += PLOT_BOARDER * 2;
 
 	view_start = gtk_adjustment_get_value(ginfo->hadj);
-	view_width = gtk_adjustment_get_page_size(ginfo->hadj);
 	if (x > view_start + width)
 		x -= width;
 
-- 
2.13.2

Re: [PATCH 5/5] trace-cmd: Remove unused view_width variable

[Steven Rostedt]

On Sat, 12 Aug 2017 11:30:47 -0600
Michael Sartain <mikesart@fastmail.com> wrote:


This is an annoying warning, but I've kept from doing this because it
reminds me that I have an idea to actually use that variable
someday. But I've just been procrastinating on doing that. ;-)

-- Steve


> Signed-off-by: Michael Sartain <mikesart@fastmail.com>
> ---
>  trace-graph.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/trace-graph.c b/trace-graph.c
> index 1db342f..2c49549 100644
> --- a/trace-graph.c
> +++ b/trace-graph.c
> @@ -1263,7 +1263,6 @@ static void draw_info_box(struct graph_info *ginfo, const gchar *buffer,
>  	gint width, height;
>  	GdkPixmap *pix;
>  	static GdkGC *pix_bg;
> -	gint view_width;
>  	gint view_start;
>  
>  	if (!pix_bg) {
> @@ -1284,7 +1283,6 @@ static void draw_info_box(struct graph_info *ginfo, const gchar *buffer,
>  	height += PLOT_BOARDER * 2;
>  
>  	view_start = gtk_adjustment_get_value(ginfo->hadj);
> -	view_width = gtk_adjustment_get_page_size(ginfo->hadj);
>  	if (x > view_start + width)
>  		x -= width;
>  

Re: [PATCH 0/5] trace-cmd: Fixes for four small bugs plus minor cleanup

[Michael Sartain]

Ping? Were there any concerns with these or things needed to be
done before they could be merged?

Thanks.

On Sat, Aug 12, 2017, at 11:30 AM, Michael Sartain wrote:
> Thanks much.
>  -Mike
> 
> ---
> 
> Michael Sartain (5):
>   trace-cmd: Fix incorrect malloc size arg: *item instead of item
>   trace-cmd: Fix NULL pointer being passed to memcpy
>   trace-cmd: Add ULL suffix to MISSING_EVENTS since ints shouldn't be
>     left shifted by 31
>   trace-cmd: Use unsigned values in Hsieh's trace_hash fast hash
>     function
>   trace-cmd: Remove unused view_width variable
> 
>  kbuffer-parse.c    | 4 ++--
>  trace-dialog.c     | 2 +-
>  trace-graph.c      | 2 --
>  trace-hash-local.h | 4 ++--
>  trace-output.c     | 6 +++++-
>  5 files changed, 10 insertions(+), 8 deletions(-)
> 
> -- 
> 2.13.2
> 

Re: [PATCH 0/5] trace-cmd: Fixes for four small bugs plus minor cleanup

[Steven Rostedt]

On Mon, 9 Oct 2017 16:13:42 -0600
Michael Sartain <mikesart@fastmail.com> wrote:

> Ping? Were there any concerns with these or things needed to be
> done before they could be merged?
> 

Thanks for the ping, this fell back in the todo list of my emails.

I'll look at it this week.

-- Steve