linux-kernel - Re: [PATCH] building libtraceevent with clang

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170210141408.5eeb6e91@gandalf.local.home>
Date:   Fri, 10 Feb 2017 14:14:08 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Namhyung Kim <namhyung@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] building libtraceevent with clang

On Fri, 10 Feb 2017 14:03:17 -0300
Arnaldo Carvalho de Melo <acme@...nel.org> wrote:

> Hi Steven,
> 
> 	I tried building perf (and thus libtraceevent) with clang and
> got this one:
> 
> kbuffer-parse.c:312:7: warning: variable 'length' is used uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>         case OLD_RINGBUF_TYPE_TIME_EXTEND:
>              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> kbuffer-parse.c:339:29: note: uninitialized use occurs here
>         kbuf->next = kbuf->index + length;
>                                    ^~~~~~
> kbuffer-parse.c:297:21: note: initialize the variable 'length' to silence this warning
>         unsigned int length;
>                            ^
>                             = 0
> 1 warning generated.

Hmm, I'm surprised gcc never complained.

> 
> 
> Please take a look if the following is what should be done:
> 
> diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
> index 65984f1c2974..3f717294cb82 100644
> --- a/tools/lib/traceevent/kbuffer-parse.c
> +++ b/tools/lib/traceevent/kbuffer-parse.c
> @@ -309,20 +309,20 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
>  		kbuf->next = kbuf->size;
>  		return 0;
>  
> -	case OLD_RINGBUF_TYPE_TIME_EXTEND:
> -		extend = read_4(kbuf, ptr);
> -		extend <<= TS_SHIFT;
> -		extend += delta;
> -		delta = extend;
> -		ptr += 4;
> -		break;
> -
>  	case OLD_RINGBUF_TYPE_TIME_STAMP:
>  		/* should never happen! */
>  		kbuf->curr = kbuf->size;
>  		kbuf->next = kbuf->size;
>  		kbuf->index = kbuf->size;
>  		return -1;
> +
> +	case OLD_RINGBUF_TYPE_TIME_EXTEND:
> +		extend = read_4(kbuf, ptr);
> +		extend <<= TS_SHIFT;
> +		extend += delta;
> +		delta = extend;
> +		ptr += 4;
> +		/* Fall through */

No, actually, the length should be zeroed here.

	length = 0;

As the ptr is placed passed the metadata. At the end of this case
statement, the ptr will be after the metadata. The length is
to represent the length of the data that is part of the event. The
index is the ptr - start_data + length. As there's no data to this type
of event, it should be zero.

When I add length = 0, I get more events from my old 2.6.30 trace.dat
file :-/  I'll have to update my tests.

Care to send another patch?

-- Steve


>  	default:
>  		if (len)
>  			length = len * 4;