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]

Message-ID: <20131205081822.GU28413@mwanda>
Date:	Thu, 5 Dec 2013 11:18:22 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Serban Constantinescu <serban.constantinescu@....com>
Cc:	gregkh@...uxfoundation.org, arve@...roid.com,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	john.stultz@...aro.org, ccross@...roid.com, Dave.Butcher@....com,
	irogers@...gle.com, romlem@...roid.com
Subject: Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic
 into subfunction

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.

regards,
dan carpenter
--
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/