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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
