rpc decoder possible double-free problem

Zeek
1

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

2

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