bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: fork: mach_port_mod_refs: EKERN_UREFS_OWERFLOW


From: Thomas Schwinge
Subject: Re: fork: mach_port_mod_refs: EKERN_UREFS_OWERFLOW
Date: Fri, 26 Nov 2010 01:22:05 +0100
User-agent: Mutt/1.5.20 (2009-06-14)

Hello!

On Mon, Nov 22, 2010 at 11:56:45AM +0100, Samuel Thibault wrote:
> Thomas Schwinge, le Mon 22 Nov 2010 09:38:24 +0100, a écrit :
> >        463            (err = __mach_port_mod_refs (newtask, ss->thread,
> >        464                                         MACH_PORT_RIGHT_SEND,
> >        465                                         thread_refs)))
> 
> and
> 
> thread_refs = 65534
> 
> it happens that gnumach has
> 
> #define       MACH_PORT_UREFS_MAX     ((mach_port_urefs_t) ((1 << 16) - 1))
> 
> So the original issues seems to be that thread_refs went in the sky.
> It's coming from some __mach_port_get_refs above.

OK.  I just had a look at this.  Let me reuse what I wrote on IRC as a
caveat emptor:

    <tschwinge> So.  I'm half asleep.
    <tschwinge> But I just had a look at glibc's fork implementaiton.
    <tschwinge> W.r.t. the bug that I recently reported.
    <tschwinge> youpi: ^
    <tschwinge> And indeed there's something fishy there.
    <tschwinge> Or I'm not awake enough to grok it.
    <tschwinge> Hmm.  Let me think / read again.
    <tschwinge> youpi: I'll send email.  You / Roland / Thomas / ...
      should be able to quickly tell whether it makes sense what I'm
      suspecting.

[glibc]/sysdeps/mach/hurd/fork.c:__fork, in the venerable port rights
duplication code.

          /* Insert all our port rights into the child task.  */
          thread_refs = sigthread_refs = 0;
          for (i = 0; i < nportnames; ++i)
            {
              if (porttypes[i] & MACH_PORT_TYPE_RECEIVE)
                {
    [...]
                }
              else if (porttypes[i] &
                       (MACH_PORT_TYPE_SEND|MACH_PORT_TYPE_DEAD_NAME))
                {
                  /* This is a send right or a dead name.
                     Give the child as many references for it as we have.  */
                  mach_port_urefs_t refs, *record_refs = NULL;
    [...]
                  else if (portnames[i] == ss->thread)
                    {
                      /* For the name we use for our own thread port, we will
                         insert the thread port for the child main user thread
                         after we create it.  */
                      insert = MACH_PORT_NULL;
                      record_refs = &thread_refs;
                      /* Allocate a dead name right for this name as a
                         placeholder, so the kernel will not chose this name
                         for any other new port (it might use it for one of the
                         rights created when a thread is created).  */
                      if (err = __mach_port_allocate_name
                          (newtask, MACH_PORT_RIGHT_DEAD_NAME, portnames[i]))
                        LOSE;
                    }
                  else if (portnames[i] == _hurd_msgport_thread)
                    /* For the name we use for our signal thread's thread port,
                       we will insert the thread port for the child's signal
                       thread after we create it.  */
                    {
                      insert = MACH_PORT_NULL;
                      record_refs = &sigthread_refs;
                      /* Allocate a dead name right as a placeholder.  */
                      if (err = __mach_port_allocate_name
                          (newtask, MACH_PORT_RIGHT_DEAD_NAME, portnames[i]))
                        LOSE;
                    }

In only these two cases we set record_refs.

    [...]
                  /* Find out how many user references we have for
                     the send right with this name.  */
                  if (err = __mach_port_get_refs (__mach_task_self (),
                                                  portnames[i],
                                                  MACH_PORT_RIGHT_SEND,
                                                  record_refs ?: &refs))
                    LOSE;

Depending on whether record_refs is valid, we stored the value in
*record_refs, or in the generic refs.

                  if (insert == MACH_PORT_NULL)
                    continue;
                  if (insert == portnames[i] &&
                      (porttypes[i] & MACH_PORT_TYPE_DEAD_NAME))
                    /* This is a dead name; allocate another dead name
                       with the same name in the child.  */
                  allocate_dead_name:
                    err = __mach_port_allocate_name (newtask,
                                                     MACH_PORT_RIGHT_DEAD_NAME,
                                                     portnames[i]);
                  else
                    /* Insert the chosen send right into the child.  */
                    err = __mach_port_insert_right (newtask,
                                                    portnames[i],
                                                    insert, insert_type);
                  switch (err)
                    {
    [...]
                    case KERN_SUCCESS:
                      /* Give the child as many user references as we have.  */
                      if (refs > 1 &&
                          (err = __mach_port_mod_refs (newtask,
                                                       portnames[i],
                                                       MACH_PORT_RIGHT_SEND,
                                                       refs - 1)))
                        LOSE;

Here, we've unconditionally used the value of refs, and didn't take into
account that record_refs ought to have been used instead, and refs could
be any value (uninitialized) -- or which detail am I missing here?
Should refs have simply been initialized to zero (as the zero value is
noneffective, and we'll set the ss->thread, etc. values later on)?  Can
this possibly have caused the following:

                    }
                }
            }
    [...]
          if (thread_refs > 1 &&
              (err = __mach_port_mod_refs (newtask, ss->thread,
                                           MACH_PORT_RIGHT_SEND,
                                           thread_refs)))
            LOSE;

According to GDB, thread_refs is 65534 here, which is already
(MACH_PORT_UREFS_MAX - 1) (mach_port_mod_refs takes a signed delta).

(a) I'm not sure if we can trust GDB in presence of GCC's optimized code.
(b) Due to my sleepiness I can't figure out at the moment where this
value may be coming from, and whether it may be valid or not.


Regards,
 Thomas

Attachment: signature.asc
Description: Digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]