Re: [PATCH -v2] futex: fix reference leak

Ingo Molnar
2009-02-11T12:29:36Z

* Peter Zijlstra <peterz@infradead.org> wrote:

> + if (!unqueue_me(&q)) {
> + ret = 0;
> + goto out_put_key;
> + }
> + if (rem) {
> + ret = -ETIMEDOUT;
> + goto out_put_key;
> + }

hm, we generally prefer to write such things as:

ret = 0;
if (!unqueue_me(&q))
goto out_put_key;

ret = -ETIMEDOUT;
if (rem)
goto out_put_key;

Also, while at it, the flow of control looks weird in other places too:

if (!abs_time)
return -ERESTARTSYS;
else {
struct restart_block *restart;
restart = &current_thread_info()->restart_block;
restart->fn = futex_wait_restart;
restart->futex.uaddr = (u32 *)uaddr;

Shouldnt it be something like:

if (!abs_time)
return -ERESTARTSYS;

restart = &current_thread_info()->restart_block;
restart->fn = futex_wait_restart;
restart->futex.uaddr = (u32 *)uaddr;

(with the variable definition moving up to local variables.)

and this:

> + if (!abs_time) {
> + ret = -ERESTARTSYS;
> + goto out_put_key;
> + } else {
> struct restart_block *restart;
> restart = &current_thread_info()->restart_block;
> restart->fn = futex_wait_restart;
> @@ -1309,11 +1314,13 @@ retry:
> restart->futex.flags |= FLAGS_SHARED;
> if (clockrt)
> restart->futex.flags |= FLAGS_CLOCKRT;
> - return -ERESTART_RESTARTBLOCK;
> + ret = -ERESTART_RESTARTBLOCK;
> + goto out_put_key;
> }
>
> out_unlock_put_key:
> queue_unlock(&q, hb);
> +out_put_key:
> put_futex_key(fshared, &q.key);

and this looks weird too, we jump-goto over the queue_unlock() in essence.

A proper flow would be to rename the error labels as err_unlock_*[etc], move
them out of line, let them jump back to the normal labels - and let the
tail section of the code above fall through.

Ingo