[Bro] rpc decoder possible double-free problem

Ruoming Pang rpang at cs.princeton.edu
Fri Aug 17 09:34:47 PDT 2007


Hi Jimmy,

Thanks a bunch for pointing out the problem.

One solution is to add &refcount to "RPC_Call" and make FindCall()
return a Ref'ed pointer. (~RPC_Reply() will not delete call_, but
Unref it.)

But this solution is not perfect either, because it doesn't work for:
  RPC_AcceptedReply($context.conn.FindCall(msg.xid))

* * *

A potentially better solution is to add the "ref" at the let var definition:

  call = ref(conn.FindCall(...))

But binpac currently does not recognize this syntax. It requires us to
add "ref" as a binpac keyword. I will work on it once I'm back next
week.

* * *

Another possibility is to avoid cleaning up of &let variables. There
are a few caveats in this case:

1. All "withinput" variables should be released. This is easy to check.

2. Bytestring's should be released, too. This I plan to solve by
making every bytestring release itself when going out of scope.

3. Maybe others? I can't find any, but can't tell for sure.

I think I prefer the former solution. What do you think?

Ruoming

On 7/26/07, jmzhou.ml at gmail.com <jmzhou.ml at gmail.com> wrote:
> Hi,
>
> I notice that in ~RPC_Reply, there is a statement "delete call_;". However,
> this call_ is instantiated by calling t_context->connection()->FindCall(
> msg()->xid()). This means that it is not "new"ed. This call_ supposely is
> deleted in the ~RPC_Message. Thus, there is a potential double-free here.
>
> The root cause is that binpac generates the "delete call_" for &let field.
> This is incorrect behavior. However, I haven't figured out how to fix it
> in binpac.
>
> A quick fix to this problem is to modify the rpc-protocol.pac:RPC_Reply,
> replacing the RPC_AcceptedReply(call) and RPC_RejectedReply(call) with:
>    RPC_AcceptedReply($context.conn.FindCall(msg.xid))
>    RPC_RejectedReply($context.conn.FindCall(msg.xid))
>
> Cheers,
>
> Jimmy
> ____________________________________________________________
> The future is not set.  There is no fate but what we make
> for ourselves.             - Terminator II, Judgment Day
> ------------------------------------------------------------
> _______________________________________________
> Bro mailing list
> bro at bro-ids.org
> http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/bro
>



More information about the Bro mailing list