linux-kernel - Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction

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]

Date:	Thu, 05 Dec 2013 18:35:13 +0000
From:	Serban Constantinescu <Serban.Constantinescu@....com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"arve@...roid.com" <arve@...roid.com>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"john.stultz@...aro.org" <john.stultz@...aro.org>,
	"ccross@...roid.com" <ccross@...roid.com>,
	Dave Butcher <Dave.Butcher@....com>,
	"irogers@...gle.com" <irogers@...gle.com>,
	"romlem@...roid.com" <romlem@...roid.com>
Subject: Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic
 into subfunction

On 05/12/13 08:18, Dan Carpenter wrote:
> On Wed, Dec 04, 2013 at 06:09:33PM +0000, Serban Constantinescu wrote:
>> +static void bc_increfs_done(struct binder_proc *proc,
>> +		struct binder_thread *thread, uint32_t cmd,
>> +		void __user *node_ptr, void __user *cookie)
>> +{
>> +	struct binder_node *node;
>> +
>> +	node = binder_get_node(proc, node_ptr);
>> +	if (node == NULL) {
>> +		binder_user_error("%d:%d %s u%p no match\n",
>> +			proc->pid, thread->pid,
>> +			cmd == BC_INCREFS_DONE ?
>> +			"BC_INCREFS_DONE" :
>> +			"BC_ACQUIRE_DONE",
>> +			node_ptr);
>> +		return;
>> +	}
>> +	if (cookie != node->cookie) {
>> +		binder_user_error("%d:%d %s u%p node %d cookie mismatch %p != %p\n",
>> +			proc->pid, thread->pid,
>> +			cmd == BC_INCREFS_DONE ?
>> +			"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
>> +			node_ptr, node->debug_id,
>> +			cookie, node->cookie);
>> +		return;
>> +	}
>> +	if (cmd == BC_ACQUIRE_DONE) {
>> +		if (node->pending_strong_ref == 0) {
>> +			binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
>> +				proc->pid, thread->pid,
>> +				node->debug_id);
>> +			return;
>> +		}
>> +		node->pending_strong_ref = 0;
>> +	} else {
>> +		if (node->pending_weak_ref == 0) {
>> +			binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
>> +				proc->pid, thread->pid,
>> +				node->debug_id);
>> +			return;
>> +		}
>> +		node->pending_weak_ref = 0;
>> +	}
>> +	binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
>> +	binder_debug(BINDER_DEBUG_USER_REFS,
>> +		     "%d:%d %s node %d ls %d lw %d\n",
>> +		     proc->pid, thread->pid,
>> +		     cmd == BC_INCREFS_DONE ?
>> +		     "BC_INCREFS_DONE" :
>> +		     "BC_ACQUIRE_DONE",
>> +		     node->debug_id, node->local_strong_refs,
>> +		     node->local_weak_refs);
>> +	return;
>> +}
>
> This function is 52 lines long but at least 32 of those lines are
> debug code.
>
> Just extra of lines of code means you have increased the number of bugs
> 150%.  But what I know is that from a static analysis perspective, debug
> code has more defects per line then regular code it's worse than that.
> Plus all the extra noise obscures the actual logic and makes the real
> code annoying to look at so it's worse still.
>
> If you want to move this stuff out of staging, then remove all the extra
> crap so it doesn't look like barf.

This patch moves code around so that some of it can be shared with the
compat layer. I agree that due to the abundance of debug code it is, at
times, hard to have a clear idea of what is actually happening and adds
extra obscurities to the logic.

Thanks,
Serban C

--
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/