lkml.org 
[lkml]   [2015]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 13/13] android: binder: add function for processing work nodes in binder_thread_read
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



\
 
 \ /
  Last update: 2015-05-29 14:41    [W:0.186 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site