Bug report: soabort() will free *so twice:

[jack wang]

I found a bug: soabort() will free *so twice:

soabort() (in bsd/sys/kern/uipc_socket.cc)

   -> pru_abort() (in tcp_usr_abort in bsd/sys/netinet/tcp_usrreq.cc)

       ->tcp_drop() (in tcp_usr_abort in bsd/sys/netinet/tcp_subr.cc)

           ->tcp_close() (in tcp_usr_abort in bsd/sys/netinet/tcp_subr.cc)

               ->sofree(so) //free so here for the first time

   ->sofree(so); //free so here for the second time

I am not familiar with the code here. I don't know how to fix it. Can someone help me?

source code:

void

soabort(struct socket *so)

{

       uipc_d("soabort() so=%" PRIx64, (uint64_t)so);

        /*

        * In as much as is possible, assert that no references to this

        * socket are held.  This is not quite the same as asserting that the

        * current thread is responsible for arranging for no references, but

        * is as close as we can get for now.

        */

       KASSERT(so->so_count == 0, ("soabort: so_count"));

       KASSERT((so->so_state & SS_PROTOREF) == 0, ("soabort: SS_PROTOREF"));

       KASSERT(so->so_state & SS_NOFDREF, ("soabort: !SS_NOFDREF"));

       KASSERT((so->so_state & SQ_COMP) == 0, ("soabort: SQ_COMP"));

       KASSERT((so->so_state & SQ_INCOMP) == 0, ("soabort: SQ_INCOMP"));

       VNET_SO_ASSERT(so);

        if (so->so_proto->pr_usrreqs->pru_abort != NULL)

               (*so->so_proto->pr_usrreqs->pru_abort)(so);  /*free so*/

        ACCEPT_LOCK();

       SOCK_LOCK(so);

       sofree(so); /*free so*/

}

[Geraldo Netto]

Hello Jack/All,

Nice catch, as soon as I can I'll check that

I guess we can do a recursive grep for sofree to check where sofree() is been called

Let's keep in touch

Kind Regards,

Geraldo Netto

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Nadav Har'El]

I wonder if https://github.com/cloudius-systems/osv/issues/936 is related to this?

--
Nadav Har'El

n...@scylladb.com

[Waldek Kozaczuk]

I think that this code in tcp_usr_abort has to be changed - https://github.com/cloudius-systems/osv/blob/master/bsd/sys/netinet/tcp_usrreq.cc#L909-L924. If first "if" with two conditions is true than the second is as well (unless inp_flags get changed by tcp_drop()) and even this statement might blow up -  

so->so_state |= SS_PROTOREF; .

I think that tcp_drop() returns NULL if it calls sofree() downstream in tcp_close(). So I think we need to change the tcp_usr_abort to detect if tcp_drop returned NULL, change the signature and pass somehow the information back to soabort() to make it do not call soclose() again.

I checked and pru_abort is only used in 2 places.

I also wonder if Charles Meyers from Spirent fixed it in one of his patches.

Waldek

[Waldek Kozaczuk]

Adding Charles to the thread.

You received this message because you are subscribed to a topic in the Google Groups "OSv Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/osv-dev/uOv1057u5Q0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to osv-dev+u...@googlegroups.com.

[Geraldo Netto]

Hey Guys!

I guess, a simple if would solve the problem at the surface
eg:

if (so) sofree(so);

What do you think?

Shall I submit a patch on this?

@jack wang, do you happen to have any test case

So we can confirm this if (so) might be enough?

Any suggestion is welcomed :)

Kind Regards,

Geraldo Netto
Sapere Aude => Non dvcor, dvco
http://exdev.sf.net/

[Waldek Kozaczuk]

I am afraid the solution is not as simple as what you are suggesting. The so pointer is passed by value, not by reference, so there is no chance this variable can be modified by downstream pru_abort()/tcp_usr_abort() and then checked for null. That is why we would have to change signature of pru_abort() to let us know if sofree() was called.

I agree with you that it would be nice to have a test to verify that any potential fix works. I might be able to come up with a patch.

Waldek

On Sunday, November 4, 2018 at 3:18:33 PM UTC-5, Geraldo Netto wrote:

Hey Guys!

I guess, a simple if would solve the problem at the surface
eg:

if (so) sofree(so);

What do you think?

Shall I submit a patch on this?

...@jack wang, do you happen to have any test case

[Waldek Kozaczuk]

Jack,

I have just emailed partial fix patch for tcp socket. Can you please test it?

Waldek