linux-kernel - Re: [PATCH 2/2] perf header: Less function calls in read_event

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 for Android: free password hash cracker in your pocket

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

Message-ID: <558C5E82.9000805@users.sourceforge.net>
Date:	Thu, 25 Jun 2015 22:03:14 +0200
From:	SF Markus Elfring <elfring@...rs.sourceforge.net>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
CC:	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	LKML <linux-kernel@...r.kernel.org>,
	kernel-janitors@...r.kernel.org,
	Julia Lawall <julia.lawall@...6.fr>
Subject: Re: [PATCH 2/2] perf header: Less function calls in read_event_desc()
 after error detection

>> The functions "free" and "free_event_desc" were called in a few cases by the
>> function "read_event_desc" during error handling even if the passed variable
>> contained a null pointer.
> 
> And that is not a problem, free(NULL) is perfectly valid, as is valid to
> call free_event_desc(NULL).

I hope that inefficient exception handling can be fixed.


> But if you want to avoid those extra NULL checks done at those functions,
> then add a new out: label that just do 'return events;' that is NULL, etc.

I adjusted the jump labels in the affected function already.


>> This implementation detail could be improved by the adjustment of jump targets.
>> Drop unnecessary initialisations for the variables "buf" and "events" then.
>>
>> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
>> ---
>>  tools/perf/util/header.c | 29 +++++++++++++++--------------
>>  1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index 03ace57..8071163 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -978,9 +978,9 @@ static void free_event_desc(struct perf_evsel *events)
>>  static struct perf_evsel *
>>  read_event_desc(struct perf_header *ph, int fd)
>>  {
>> -	struct perf_evsel *evsel, *events = NULL;
>> +	struct perf_evsel *evsel, *events;
>>  	u64 *id;
>> -	void *buf = NULL;
>> +	void *buf;
>>  	u32 nre, sz, nr, i, j;
>>  	ssize_t ret;
>>  	size_t msz;
>> @@ -988,14 +988,14 @@ read_event_desc(struct perf_header *ph, int fd)
>>  	/* number of events */
>>  	ret = readn(fd, &nre, sizeof(nre));
>>  	if (ret != (ssize_t)sizeof(nre))
>> -		goto error;
>> +		return NULL;
> 
> Please leave the single point of exit, i.e. this should 'goto out:' and
> the out: label will return events, that is set to NULL

Does my update suggestion fit to the wording "If there is no cleanup needed
then just return directly." from the Linux coding style documentation?


>>  	if (ph->needs_swap)
>>  		nre = bswap_32(nre);
>>  
>>  	ret = readn(fd, &sz, sizeof(sz));
>>  	if (ret != (ssize_t)sizeof(sz))
>> -		goto error;
>> +		return NULL;
>>  
>>  	if (ph->needs_swap)
>>  		sz = bswap_32(sz);
>> @@ -1003,12 +1003,12 @@ read_event_desc(struct perf_header *ph, int fd)
>>  	/* buffer to hold on file attr struct */
>>  	buf = malloc(sz);
>>  	if (!buf)
>> -		goto error;
>> +		return NULL;
>>  
>>  	/* the last event terminates with evsel->attr.size == 0: */
>>  	events = calloc(nre + 1, sizeof(*events));
>>  	if (!events)
>> -		goto error;
>> +		goto free_buffer;
>>  
>>  	msz = sizeof(evsel->attr);
>>  	if (sz < msz)
>> @@ -1023,7 +1023,7 @@ read_event_desc(struct perf_header *ph, int fd)
>>  		 */
>>  		ret = readn(fd, buf, sz);
>>  		if (ret != (ssize_t)sz)
>> -			goto error;
>> +			goto free_events;
>>  
>>  		if (ph->needs_swap)
>>  			perf_event__attr_swap(buf);
>> @@ -1032,7 +1032,7 @@ read_event_desc(struct perf_header *ph, int fd)
>>  
>>  		ret = readn(fd, &nr, sizeof(nr));
>>  		if (ret != (ssize_t)sizeof(nr))
>> -			goto error;
>> +			goto free_events;
>>  
>>  		if (ph->needs_swap) {
>>  			nr = bswap_32(nr);
>> @@ -1046,26 +1046,27 @@ read_event_desc(struct perf_header *ph, int fd)
>>  
>>  		id = calloc(nr, sizeof(*id));
>>  		if (!id)
>> -			goto error;
>> +			goto free_events;
>>  		evsel->ids = nr;
>>  		evsel->id = id;
>>  
>>  		for (j = 0 ; j < nr; j++) {
>>  			ret = readn(fd, id, sizeof(*id));
>>  			if (ret != (ssize_t)sizeof(*id))
>> -				goto error;
>> +				goto free_events;
>>  			if (ph->needs_swap)
>>  				*id = bswap_64(*id);
>>  			id++;
>>  		}
>>  	}
>> -out:
>> +
>>  	free(buf);
>>  	return events;
>> -error:
>> +free_events:
>>  	free_event_desc(events);
>> -	events = NULL;
>> -	goto out;
>> +free_buffer:
>> +	free(buf);
>> +	return NULL;
>>  }
>>  
>>  static int __desc_attr__fprintf(FILE *fp, const char *name, const char *val,
>> -- 
>> 2.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/