[PATCH] libtraceeval: Fix comparing unsigned against zero

[PATCH] libtraceeval: Fix comparing unsigned against zero

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

nr_key_types and nr_val_types are both size_t which is an unsized number.
A compare against zero is always false:

	if (teval->nr_key_types < 0)

Typecast it to signed for these comparisons.

Also change the few places the "size_t i" is used where it does:

	for (; i >= 0; i--)

as that too needs to be signed to work.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 src/histograms.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/histograms.c b/src/histograms.c
index 96f0926f062c..42959c154a11 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -159,7 +159,7 @@ static size_t type_alloc(const struct traceeval_type *defs,
 {
 	struct traceeval_type *new_defs = NULL;
 	size_t size;
-	size_t i;
+	ssize_t i;
 
 	*copy = NULL;
 
@@ -196,7 +196,7 @@ fail:
 	else
 		print_err("traceeval_type list missing a name");
 
-	for (; i >=0; i--)
+	for (; i >= 0; i--)
 		free(new_defs[i].name);
 	free(new_defs);
 	return -1;
@@ -303,14 +303,14 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
 
 	/* alloc key types */
 	teval->nr_key_types = type_alloc(keys, &teval->key_types);
-	if (teval->nr_key_types <= 0) {
+	if ((ssize_t)teval->nr_key_types <= 0) {
 		err_msg = "Failed to allocate user defined keys";
 		goto fail_release;
 	}
 
 	/* alloc val types */
 	teval->nr_val_types = type_alloc(vals, &teval->val_types);
-	if (teval->nr_val_types < 0) {
+	if ((ssize_t)teval->nr_val_types < 0) {
 		err_msg = "Failed to allocate user defined values";
 		goto fail_release;
 	}
@@ -785,7 +785,7 @@ static int update_entry(struct traceeval *teval, struct entry *entry,
 	union traceeval_data *copy = entry->vals;
 	union traceeval_data old[teval->nr_val_types];
 	size_t size = teval->nr_val_types;
-	size_t i;
+	ssize_t i;
 
 	if (!size)
 		return 0;
-- 
2.40.1

Re: [PATCH] libtraceeval: Fix comparing unsigned against zero

[Ross Zwisler]

On Wed, Sep 27, 2023 at 04:16:29AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> nr_key_types and nr_val_types are both size_t which is an unsized number.
> A compare against zero is always false:
> 
> 	if (teval->nr_key_types < 0)
> 
> Typecast it to signed for these comparisons.
> 
> Also change the few places the "size_t i" is used where it does:
> 
> 	for (; i >= 0; i--)
> 
> as that too needs to be signed to work.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  src/histograms.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/histograms.c b/src/histograms.c
> index 96f0926f062c..42959c154a11 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -159,7 +159,7 @@ static size_t type_alloc(const struct traceeval_type *defs,
>  {
>  	struct traceeval_type *new_defs = NULL;
>  	size_t size;
> -	size_t i;
> +	ssize_t i;
>  
>  	*copy = NULL;
>  
> @@ -196,7 +196,7 @@ fail:
>  	else
>  		print_err("traceeval_type list missing a name");
>  
> -	for (; i >=0; i--)
> +	for (; i >= 0; i--)
>  		free(new_defs[i].name);
>  	free(new_defs);
>  	return -1;
> @@ -303,14 +303,14 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
>  
>  	/* alloc key types */
>  	teval->nr_key_types = type_alloc(keys, &teval->key_types);
> -	if (teval->nr_key_types <= 0) {
> +	if ((ssize_t)teval->nr_key_types <= 0) {

Rather than casting, wouldn't it be better to just change the type of
nr_key_types and nr_val_types to ssize_t?  I think this should be the return
type of type_alloc() as well, since this returns -1 on error.

>  		err_msg = "Failed to allocate user defined keys";
>  		goto fail_release;
>  	}
>  
>  	/* alloc val types */
>  	teval->nr_val_types = type_alloc(vals, &teval->val_types);
> -	if (teval->nr_val_types < 0) {
> +	if ((ssize_t)teval->nr_val_types < 0) {
>  		err_msg = "Failed to allocate user defined values";
>  		goto fail_release;
>  	}
> @@ -785,7 +785,7 @@ static int update_entry(struct traceeval *teval, struct entry *entry,
>  	union traceeval_data *copy = entry->vals;
>  	union traceeval_data old[teval->nr_val_types];
>  	size_t size = teval->nr_val_types;
> -	size_t i;
> +	ssize_t i;
>  
>  	if (!size)
>  		return 0;
> -- 
> 2.40.1
> 

Re: [PATCH] libtraceeval: Fix comparing unsigned against zero

[Steven Rostedt]

On Mon, 2 Oct 2023 12:31:02 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > @@ -303,14 +303,14 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
> >  
> >  	/* alloc key types */
> >  	teval->nr_key_types = type_alloc(keys, &teval->key_types);
> > -	if (teval->nr_key_types <= 0) {
> > +	if ((ssize_t)teval->nr_key_types <= 0) {  
> 
> Rather than casting, wouldn't it be better to just change the type of
> nr_key_types and nr_val_types to ssize_t?  I think this should be the return
> type of type_alloc() as well, since this returns -1 on error.

I thought about that, and there was a reason against it. But I forgot what
that was :-p

-- Steve

Re: [PATCH] libtraceeval: Fix comparing unsigned against zero

[Steven Rostedt]

On Mon, 2 Oct 2023 14:37:15 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 2 Oct 2023 12:31:02 -0600
> Ross Zwisler <zwisler@google.com> wrote:
> 
> > > @@ -303,14 +303,14 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
> > >  
> > >  	/* alloc key types */
> > >  	teval->nr_key_types = type_alloc(keys, &teval->key_types);
> > > -	if (teval->nr_key_types <= 0) {
> > > +	if ((ssize_t)teval->nr_key_types <= 0) {    
> > 
> > Rather than casting, wouldn't it be better to just change the type of
> > nr_key_types and nr_val_types to ssize_t?  I think this should be the return
> > type of type_alloc() as well, since this returns -1 on error.  
> 
> I thought about that, and there was a reason against it. But I forgot what
> that was :-p

I still can't remember why I didn't convert ssize_t, so maybe it was not a
real reason. I'll send a v2 changing the type to ssize_t and if there was a
reason I didn't do that before, maybe it will show up when I make the
change!

-- Steve