BpHwBinder: call tranasact callback
Parcel (because of scatter-gather) has pointers to buffers in
userspace memory. Because of this, wherever you say "Parcel" you can
almost think of it as a reference type to other things that are
being parceled into it. Because it's a reference type, it should
always have a lifetime that is guaranteed to be longer than the data
it is referencing.
For instance:
Parcel foo;
{
hidl_vec<uint8_t> vec = {0, 1, 2};
// whatever the actual method is
foo.writeBuffer(... vec.data() ...);
}
// **BAD!** foo contains invalid references
Because of this, ideally, the lifetime of a Parcel should never be able
to exceed that of data this is written on it. Further, the
BpHwBinder::onTransact signature should not have Parcel reply as an
output parameter (but we cannot change this).
So, for sake of simplicity, the current definition of onTransact is this:
... BpHwBinder::onTransact(Parcel in, Parcel* out, TransactCallback cb);
An interface implementation might look like this (ignoring error cases
and collapsing calls to callbacks in libhwbinder here):
... BnFoo::onTransact(Parcel in, Parcel* out, TransactCallback cb) {
return f(in.read..., [&] (...) {
...
out->write...(output);
// this is where things could go wrong
// if output is in this scope, Parcel has references to
// local uninitialized data
cb(*out);
});
}
In normal transactions for both C++ and Java, this ends up being safe
(the binder driver makes a copy of the data whenever the server
returns, and the lifetime of that data is guaranteed to be longer
than the duration of the callback).
For in-process C++ transactions, this parceling never happens because
these interfaces haven't been used.
For an in-process Java<->C++ transaction though, for instance, the
code effectively looks like this (ignoring JNI boundaries/libhwbinder
layers/etc):
... BnFoo::onTransact(Parcel in, Parcel* out, TransactCallback cb) {
return f(in.read..., [&] (const auto& foo) {
// data structure which creates copies of
// java objects so that they can be used
// with scatter-gather
HwBlob blob;
// Java data gets copied into a format that will work
// with the HIDL wire protocol
blob.write(foo);
// Parcel actually has references to data inside the blob
out->write...(<references inside blob>);
cb(*out);
// BOOM, rather than reading data inside the callback that
// was passed in, the Bp* code has to read it from 'out'
// as an out parameter. However, out references data that
// is only valid in this scope (the HwBlob), so it should
// only be read within the cb.
});
}
I would expect this same problem for in-process C++ parceling (which
I'll follow up with tests for).
In order to prevent this problem entirely, the signature should have
the out Parcel removed, collapsing cb scopes again, it might look like
this:
... onTransact(Parcel in, TransactCallback cb) {
auto foo = in.readData...;
f(foo, [&] (...) {
Parcel reply; can never live longer than data in cb scopes
reply.write...(...);
cb(*reply);
});
}
However, we can't change the function signature. As an additoinal
follow-up, we could consider always nulling the out parameter, but
it will be several years before all the code which uses it will be
updated.
By fixing this here, we gain two things:
- C++ <-> Java in-process transactions (which we only use for
getService now in order to further unify Java/C++ internals.
This is explicitly disabled for normal use for the time being).
- possibility of using these interfaces with transport protocols other
than binder which don't provide these nice guarantees.
Bug: 129150021
Test: boot/use java passthrough in-process for getService
Change-Id: Ic8082afb9fbb603b83933fab4430651dbaedf637
1 file changed
