Re: [PATCH] Port leak when using clisp

[Justus Winter]

Hi,

Quoting Flávio Cruz (2015-08-27 00:26:13)
> Em qua, 26 de ago de 2015 às 12:18, Justus Winter <
> 4winter@informatik.uni-hamburg.de> escreveu:
> 
> 
>     > After looking around and using KDB, I've figured out that the following
>     loop in
>     > kern/exceptions.c/exception_raise_continue_slow was the culprit:
> 
>     Could you elaborate a little on your method?
> 
> 
> I changed the code to chain all the allocated ports together in a linked list.
> Then, I added new fields to ipc_port to track send-only rights and to
> understand where those rights were being created and where the port was being
> manipulated. Finally, I added a new KDB command to show all the inactive 
> ports.
> It was then easy to track exactly how the port was handled inside
> exception_raise_continue_slow.

Neat.  Thanks for sharing.  Fwiw, I've used Coccinelle for code
instrumentations.  Also for fixing up code in a mechanical way.  It is
an awesome tool :)

>     > while (mr == MACH_RCV_INTERRUPTED) {
>     > /*
>     > * Somebody is trying to force this thread
>     > * to a clean point.  We must cooperate
>     > * and then resume the receive.
>     > */
>     >
>     > while (thread_should_halt(self)) {
>     > /* don't terminate while holding a reference */
>     > if (self->ast & AST_TERMINATE)
>     > ipc_port_release(reply_port);
>     > thread_halt_self();
>     > }
>     >
>     > ip_lock(reply_port);
>     > ....
>     > }
>     >
>     > The reply port of the target thread (from CLISP?) is not being released
>     since
>     > it enters the while(thread_should_halt(self)) loop and the thread
>     terminates
>     > inside thread_halt_self but the previous condition does not hold, which
>     fails
>     > to release the port. I also think that once the code enters
>     thread_halt_self,
>     > it never comes back since the stack is discarded (for both if cases
>     inside the
>     > function).
> 
>     While I agree that the function thread_halt_self does not return, it
>     only terminates the thread if the AST_TERMINATE flag is set.
>     Otherwise, it returns to userspace.
> 
> 
> Yes, it returns to userspace using thread_exception_return, but notice that a
> reference must be released anyway (check first statement after the while 
> loop).

Right.  We have two references, one for the send-once right, and one
for the port being saved in ith_port (see exception.c:386).

> For the else case in thread_halt_self, the thread may throw away the current
> stack and run thread_exception_return in thread_block, which is also executed
> in the case of success in exception_raise_continue_slow.

And indeed exception_raise_continue_fast releases two references too.

>     I find the use of `while' dodgy in the first place b/c if
>     thread_halt_self doesn't return, an if would do the trick as well and
>     don't imply the chance of that code looping...
> 
> I think there might be some situations where it will loop around depending on
> the scheduling order (see line 798 on sched_prim.c).

Could be.  I keep messing up the semantics of thread_invoke in my head >,<

> I've added a continuation argument to thread_halt_self so that the port is
> released if the thread resumes using the continuation (followed by
>  thread_exception_return). Otherwise, it may loop and it will eventually also
> release the reference and call thread_exception_return.

Yes.  This looks like the clean solution.

> I've also made some cosmetic changes to the code.

Would be best to split the change up nevertheless.

Thanks for looking into this :)
Justus