possible bug in the sbappendrecord_locked()? (Was: Re: core dump with bluetooth device)

[Maksim Yevmenkin]

On Thu, Apr 16, 2009 at 7:22 PM, Maksim Yevmenkin
<maksim.yevmenkin at gmail.com> wrote:
> On Thu, Apr 16, 2009 at 5:39 PM, Maksim Yevmenkin
> <maksim.yevmenkin at gmail.com> wrote:
>> On Thu, Apr 16, 2009 at 12:59 PM, Alexander Best
>> <alexbestms at math.uni-muenster.de> wrote:
>>> hi there,
>>>
>>> i'm running r191076M. when i try to send files from my mobile phone to my
>>> computer via bt the core dumps. here's a backtrace:
>>>
>>> Unread portion of the kernel message buffer:
>>> sblastmbufchk: sb_mb 0xc8d54d00 sb_mbtail 0 last 0xc8d54d00
>>> packet tree:
>>>        0xc8d54d00
>>> panic: sblastmbufchk from /usr/src/sys/kern/uipc_sockbuf.c:797
>>> cpuid = 0
>>
>> are you, by change, have "options  SOCKBUF_DEBUG" in your kernel?
>
> ok, there is something strange going on in the
> sbappendrecord_locked(). consider the following initial conditions:
>
> 1) sockbuf sb is empty, i.e. sb_mb == sb_mbtail == sb_lastrecord == NULL
>
> 2) sbappendrecord_locked() is given mbuf m0 with has exactly one mbuf,
> i.e. m0->m_next == NULL
>
> so
>
> void
> sbappendrecord_locked(struct sockbuf *sb, struct mbuf *m0)
> {
>        struct mbuf *m;
>
>        SOCKBUF_LOCK_ASSERT(sb);
>
>        if (m0 == 0)
>                return;
>        m = sb->sb_mb;
>
>        if (m)
>                while (m->m_nextpkt)
>                        m = m->m_nextpkt;
>
> --> m is still NULL at this point
>
>        /*
>         * Put the first mbuf on the queue.  Note this permits zero length
>         * records.
>         */
>        sballoc(sb, m0);
>        SBLASTRECORDCHK(sb);
>
> --> passed successfully, because sb_mb == sb_lastrecord == NULL (i.e.
> sockbuf is empty)
>
>        SBLINKRECORD(sb, m0);
>
> --> at this point sb_mb == sb_lastrecord  == m0, _but_ sb_mtail == NULL
>
>        if (m)
>                m->m_nextpkt = m0;
>        else
>                sb->sb_mb = m0;
>
> --> not sure about those lines above, didn't SBLINKRECORD(sb, m0) take
> care of it already?
> --> in any case, still, sb_mb == sb_lastrecord == m0 _and_ still
> sb_mtail == NULL
>
>        m = m0->m_next;
>        m0->m_next = 0;
>
> --> m is still NULL here
>
>        if (m && (m0->m_flags & M_EOR)) {
>                m0->m_flags &= ~M_EOR;
>                m->m_flags |= M_EOR;
>        }
>
> --> sbcompress() is called with m == NULL, which is triggers the panic
> (read below)
>
>        sbcompress(sb, m, m0);
> }
>
> ===========
>
> void
> sbcompress(struct sockbuf *sb, struct mbuf *m, struct mbuf *n)
> {
>        int eor = 0;
>        struct mbuf *o;
>
>        SOCKBUF_LOCK_ASSERT(sb);
>
>        while (m) {
>
> --> lots of code that never gets executed because m == NULL
>
>        }
>        if (eor) {
>                KASSERT(n != NULL, ("sbcompress: eor && n == NULL"));
>                n->m_flags |= eor;
>        }
>
> --> this where panic happens, because sb_mbtail is still NULL, but
> sockbuf now contains exactly one record
>
>        SBLASTMBUFCHK(sb);
> }
>
> so, it looks like, sbcompress() should only be called when m != NULL.
> also, when m == NULL, m0 should be marked as EOR.

actually, no, EOR should be set (or not set already).

> comments anyone?

i think there is also something strange going on in
sbappendaddr_locked(), basically,

int
sbappendaddr_locked(struct sockbuf *sb, const struct sockaddr *asa,
    struct mbuf *m0, struct mbuf *control)
{
        struct mbuf *m, *n, *nlast;
        int space = asa->sa_len;

        SOCKBUF_LOCK_ASSERT(sb);

        if (m0 && (m0->m_flags & M_PKTHDR) == 0)
                panic("sbappendaddr_locked");
        if (m0)
                space += m0->m_pkthdr.len;
        space += m_length(control, &n);

        if (space > sbspace(sb))
                return (0);
#if MSIZE <= 256
        if (asa->sa_len > MLEN)
                return (0);
#endif
        MGET(m, M_DONTWAIT, MT_SONAME);
        if (m == 0)
                return (0);
        m->m_len = asa->sa_len;
        bcopy(asa, mtod(m, caddr_t), asa->sa_len);

--> at this point n is not initialized, or at least i do not see where
it was initialized. shouldn't be compiler giving a warning here?

        if (n)
                n->m_next = m0;         /* concatenate data to control */
        else
                control = m0;

am i missing something obvious here?

thanks
max