Re: [PATCH 13/13] android: binder: add function for processing work nodes in binder_thread_read

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, May 28, 2015 at 04:08:31PM -0700, Riley Andrews wrote:
> -done:
> +static int binder_thread_read(struct binder_proc *proc,
> +			      struct binder_thread *thread,
> +			      binder_uintptr_t binder_buffer, size_t size,
> +			      binder_size_t *consumed, int non_block)
> +{
> +	void __user *buffer = (void __user *)(uintptr_t)binder_buffer;
> +	void __user *ptr = buffer + *consumed;
> +	void __user *end = buffer + size;
> +	bool wait_for_proc_work;
> +
> +	int ret = 0;
> +
> +	if (*consumed == 0) {
> +		if (put_user(BR_NOOP, (uint32_t __user *)ptr))
> +			return -EFAULT;
> +		ptr += sizeof(uint32_t);
> +	}
> +
> +	do {
> +		if (thread->return_error != BR_OK) {
> +			ret =  binder_handle_thread_error(thread, &ptr, end);
> +			if (ret < 0)
> +				return ret;
> +			break;
> +		}
> +		if (!thread->transaction_stack && list_empty(&thread->todo))
> +			wait_for_proc_work = true;
> +		else
> +			wait_for_proc_work = false;
> +
> +		ret = binder_wait_for_work(thread, non_block,
> +					   wait_for_proc_work);
> +		if (ret)
> +			return ret;
> +
> +		ret = binder_thread_read_do_work(thread, wait_for_proc_work,
> +						 buffer, end, &ptr);
> +		if (ret)
> +			return ret;
> +	} while ((ptr - buffer == 4) &&
> +		 !(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN) &&
> +		 ((end - ptr) >= sizeof(struct binder_transaction_data) + 4));

"end" and "buffer" don't change so we could move check:

		((end - ptr) >= sizeof(struct binder_transaction_data) + 4)

to the start of the function.  I may have missed something because I'm
not terribly familiar with this code.

I don't really like the way this condition is written because if "ptr"
were greater than "end" it would be true.  This seems like something
that might happen.  Pass in bwr.read_size = 1. When we do the first
ptr += sizeof(uint32_t); then "end" is less than "ptr".

This condition was there in the original code as well so it's not
something the patch introduced but it worries me every time I look at
it, even if it turns out that it's not a problem.

Please write it like:

	(ptr + sizeof(struct binder_transaction_data) + 4 <= end)

or whatever so that we don't have to think about negative numbers.

regards,
dan carpenter