Hi Joe, Well, this is a use-after-free problem. We could in theory add code to detect various cases of use-after-free, if we see places where it happens commonly, but there's no good way for us to detect all cases. I recommend instead running your code under valgrind, which is extremely good at catching these things.
-Kenton On Sat, Apr 20, 2019 at 1:23 PM Joe Ludwig <[email protected]> wrote: > That's exactly what it was. The main interface was being freed after the > client that it came from. > > An exception in an IOCP promise is a confusing way to find out that I > messed up cleaning up some objects. Is there some way to tell if an event > loop (or in my case EzRpcClient since I'm not using event loops directly > yet) still has outstanding work to do? > > Thanks for your help! > > > Joe > > > On Saturday, April 20, 2019 at 10:15:25 AM UTC-7, Kenton Varda wrote: >> >> Hi Joe, >> >> Hmm, I'm not sure. It's helpful to read the comments that you removed, >> which can be found here: >> >> >> https://github.com/capnproto/capnproto/blob/d68cffe544e8479f3cb01a88d8a547eff8782d71/c++/src/kj/async-win32.c++#L57-L91 >> >> IoPromiseAdapter is waiting for a specific event to complete. The >> completion is indicated by GetQueuedCompletionStatus() on the IOCP >> returning the OVERLAPPED structure associated with this IoPromiseAdapter. >> We cannot allow the IoPromiseAdapter to be destroyed until that time, >> because the operating system may still be operating on that structure. >> >> The purpose of the while() loop calling port.waitIocp() is to repeatedly >> call GetQueuedCompletionStatus() until we get our event, at which point the >> destructor can safely complete. >> >> When CancelIoEx() produces ERROR_INVALID_HANDLE, this means that the >> handle has already been closed (probably, recently, due to destructors >> running in the wrong order). I believe closing the handle should implicitly >> cancel all I/O, but we still have to wait for the IOCP to produce the event >> completions before we can destroy the OVERLAPPED structure. >> >> What do you mean by "this call chokes on an invalid handle and throws an >> exception"? What part of the call throws exactly? What's the stack trace? >> waitIocp() shouldn't perform any new operations on the now-closed handle; >> it should only wait for old operations to complete. >> >> Is it possible that the Win32IocpEventPort *itself* has also already been >> destroyed, and so waitIocp() fails because the IOCP handle is invalid? If >> that's the case, this indicates a bug in your code: you've allowed the >> event loop to be shut down while you still had events in-flight. You'll >> need to make sure you destroy all promises and Cap'n Proto objects before >> you shut down the event loop. >> >> If that's not it, could you produce a small self-contained test that >> demonstrates the problem, for us to debug? >> >> -Kenton >> >> On Sat, Apr 20, 2019 at 9:24 AM Joe Ludwig <[email protected]> wrote: >> >>> Hi Kenton, >>> >>> I think I may be running into some of the cleanup complication you're >>> implying. During that, I think I'm seeing a straight-up bug, though. The >>> snippet of code below is from async-win32.c++, just with the comment blocks >>> removed. >>> >>> Win32IocpEventPort::~IoPromiseAdapter() { >>> if (handle != INVALID_HANDLE_VALUE) { >>> if (!CancelIoEx(handle, this)) { >>> DWORD error = GetLastError(); >>> >>> >>> if (error != ERROR_NOT_FOUND && error != ERROR_INVALID_HANDLE) { >>> KJ_FAIL_WIN32("CancelIoEx()", error, handle); >>> } >>> // if error was ERROR_INVALID_HANDLE, the handle will be != >>> INVALID_HANDLE_VALUE >>> } >>> >>> >>> while (handle != INVALID_HANDLE_VALUE) { >>> port.waitIocp(INFINITE); // this call chokes on an invalid >>> handle and throws an exception >>> } >>> } >>> } >>> >>> >>> Here's one possible fix: >>> ~IoPromiseAdapter() { >>> if (handle != INVALID_HANDLE_VALUE) { >>> if (!CancelIoEx(handle, this)) { >>> DWORD error = GetLastError(); >>> >>> >>> if( error == ERROR_INVALID_HANDLE ) { >>> // we know the handle has gone invalid, so just forget our >>> handle >>> handle = INVALID_HANDLE_VALUE; >>> } >>> else if (error != ERROR_NOT_FOUND ) { >>> KJ_FAIL_WIN32("CancelIoEx()", error, handle); >>> } >>> } >>> >>> >>> while (handle != INVALID_HANDLE_VALUE) { >>> port.waitIocp(INFINITE); >>> } >>> } >>> } >>> >>> >>> >>> If it would help, I can express that in the form of a pull request. >>> >>> >>> Joe >>> >>> >>> On Friday, April 19, 2019 at 10:03:32 AM UTC-7, Kenton Varda wrote: >>>> >>>> Hi Joe, >>>> >>>> Take a look at the doc comments for thisCap(): >>>> >>>> inline Capability::Client thisCap(); >>>> // Get a capability pointing to this object, much like the `this` >>>> keyword. >>>> // >>>> // The effect of this method is undefined if: >>>> // - No capability client has been created pointing to this object. >>>> (This is always the case in >>>> // the server's constructor.) >>>> // - The capability client pointing at this object has been >>>> destroyed. (This is always the case >>>> // in the server's destructor.) >>>> // - Multiple capability clients have been created around the same >>>> server (possible if the server >>>> // is refcounted, which is not recommended since the client itself >>>> provides refcounting). >>>> >>>> Your code is hitting the first bullet point -- you're trying to call >>>> thisCap() before any capability client has been created. >>>> >>>> What you really want to do here is take advantage of the fact that >>>> capability clients are copyable using reference counting. You can create a >>>> client, and then keep a copy for yourself, like so: >>>> >>>> auto server = kj::heap<MyTypeImpl>(...); >>>> auto& serverRef = *server; >>>> MyType::Client capability = kj::mv(server); >>>> >>>> // Send a ref to the client. >>>> context.getResults().setApp(capability); >>>> >>>> // Also keep one for ourself, along with a reference to the >>>> implementation >>>> myServers.add(ClientRefPair { client, serverRef }); >>>> >>>> By keeping a `Client` that points to your server object, you ensure >>>> that the refcount remains non-zero so the object stays alive. As shown >>>> above, you can separately keep a reference or pointer to the underlying >>>> server object if you need to be able to access it's C++ interface directly. >>>> >>>> Note that your server's destructor is only called after all references >>>> -- yours, and remote references -- are dropped. If you need to force your >>>> server object to be destroyed at a specific time, then you'll need to do >>>> one of the following: >>>> >>>> - Use a layer of indirection where your server object wraps another >>>> object that can be nulled out later, such that all the RPC methods start >>>> throwing exceptions once nulled. >>>> - Wrap the Client object in a membrane (capnp/membrane.h) which you >>>> later revoke. >>>> >>>> -Kenton >>>> >>>> On Thu, Apr 18, 2019 at 10:00 PM Joe Ludwig <[email protected]> >>>> wrote: >>>> >>>>> I have a really simple RPC protocol: >>>>> interface AvServer >>>>> { >>>>> createApp @0 ( name: Text ) -> ( app: AvApp ); >>>>> } >>>>> >>>>> >>>>> interface AvApp >>>>> { >>>>> name @0 () -> ( name: Text ); >>>>> >>>>> >>>>> destroy @1 (); >>>>> } >>>>> >>>>> >>>>> I implement that on the server with two concrete classes: >>>>> >>>>> class AvServerImpl final : public AvServer::Server >>>>> class CAardvarkApp final: public AvApp::Server >>>>> >>>>> >>>>> >>>>> And then when the client calls createApp I want to create an AvApp, >>>>> return a reference to it, and keep ownership of the app itself in some >>>>> datastructure on the server: >>>>> >>>>> ::kj::Promise<void> AvServerImpl::createApp( CreateAppContext >>>>> context ) >>>>> { >>>>> // "works" >>>>> context.getResults().setApp( kj::heap<CAardvarkApp>( context. >>>>> getParams().getName() ) ); >>>>> >>>>> >>>>> // crashes because "thisHook" is null >>>>> auto pApp = kj::heap<CAardvarkApp>( context.getParams().getName() ); >>>>> context.getResults().setApp( pApp->GetClient() ); // GetClient() is >>>>> just a call to thisCap() >>>>> m_listApps.push_back( std::move( pApp ) ); >>>>> >>>>> >>>>> return kj::READY_NOW; >>>>> } >>>>> >>>>> >>>>> The first approach there works, but the AvServerImpl class doesn't >>>>> keep any kind of record of the new CAardvarkApp object, so it won't be >>>>> able >>>>> to do anything with the app outside of a call from this one particular >>>>> client. >>>>> >>>>> What I'm attempting to do in the second approach is create an object >>>>> on the heap and set it in the results, but keep ownership of it in the >>>>> list. I don't get that far, though, because thisCap crashes because >>>>> thisHook returns nullptr. >>>>> >>>>> I think I must be missing something really fundamental about how all >>>>> this is supposed to work. Am I required to create a unique foo::Server >>>>> object for each incoming call that refers to a single logical object? If I >>>>> have to pass ownership to the results in order to return values, it seems >>>>> like something along that line will be called for. That doesn't sound >>>>> right, though, since I'd have to manage an unbounded number of those >>>>> objects as requests come in from different clients. >>>>> >>>>> Help? :) >>>>> >>>>> >>>>> Joe >>>>> >>>>> -- >>>>> You received this message because you are subscribed to the Google >>>>> Groups "Cap'n Proto" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to [email protected]. >>>>> Visit this group at https://groups.google.com/group/capnproto. >>>>> >>>> -- >>> You received this message because you are subscribed to the Google >>> Groups "Cap'n Proto" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> Visit this group at https://groups.google.com/group/capnproto. >>> >> -- > You received this message because you are subscribed to the Google Groups > "Cap'n Proto" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > Visit this group at https://groups.google.com/group/capnproto. > -- You received this message because you are subscribed to the Google Groups "Cap'n Proto" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. Visit this group at https://groups.google.com/group/capnproto.
