From b1597f7933c989758567ee6e88e3c5dde0f8cfba Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Fri, 21 Feb 2020 03:21:54 -0500 Subject: [PATCH] Fix some broken logic re: ownership of map keys. See the comments for the current state of affairs. The code I'd written previously didn't have any coherent story here, and was *sometimes* working I think just because of luck wrt. what memory gets re-used by the allocator or doesn't. This needs more testing, but experimenting with my filesystem demo, grains, it gets further than anything before it. --- .../sandstorm-http-bridge-internal.capnp | 16 ++- src/sandstorm/sandstorm-http-bridge.c++ | 115 +++++++++++++----- 2 files changed, 99 insertions(+), 32 deletions(-) diff --git a/src/sandstorm/sandstorm-http-bridge-internal.capnp b/src/sandstorm/sandstorm-http-bridge-internal.capnp index 64885d5946..a740699e61 100644 --- a/src/sandstorm/sandstorm-http-bridge-internal.capnp +++ b/src/sandstorm/sandstorm-http-bridge-internal.capnp @@ -23,6 +23,7 @@ using Identity = import "identity.capnp"; using WebSession = import "web-session.capnp".WebSession; using ApiSession = import "api-session.capnp".ApiSession; using AppPersistent = import "grain.capnp".AppPersistent; +using Powerbox = import "powerbox.capnp"; struct BridgeObjectId { # The object ID format used by sandstorm-http-bridge. @@ -59,8 +60,15 @@ interface BridgeHttpSession extends(ApiSession, AppPersistent(BridgeObjectId)) { const bridgeRequestSessionHtml :Text = embed "sandstorm-http-bridge-request.html"; -enum SessionType { - normal @0; - request @1; - offer @2; +struct SessionInfo { + union { + normal @0 :Void; + request :group { + requestInfo @1 :List(Powerbox.PowerboxDescriptor); + } + offer :group { + offer @2 :Capability; + descriptor @3 :Powerbox.PowerboxDescriptor; + } + } } diff --git a/src/sandstorm/sandstorm-http-bridge.c++ b/src/sandstorm/sandstorm-http-bridge.c++ index 60aaf1f718..6af4bf477f 100644 --- a/src/sandstorm/sandstorm-http-bridge.c++ +++ b/src/sandstorm/sandstorm-http-bridge.c++ @@ -20,6 +20,7 @@ // that server and then redirect incoming requests to it over standard HTTP on // the loopback network interface. +#include #include #include #include @@ -1208,20 +1209,18 @@ public: } void insertSession(const kj::StringPtr& id, SessionContext::Client& session) { - sessions.insert({kj::str(id), session}); + sessions.insert({kj::StringPtr(id), session}); } void insertRequest( const kj::StringPtr& id, capnp::List::Reader descriptor) { - auto results = - capnp::MallocMessageBuilder() - .initRoot(); - + auto msg = capnp::MallocMessageBuilder(); + auto results = msg.initRoot(); results.setRequestInfo(descriptor); - requests.insert({kj::str(id), capnp::clone(results.asReader())}); + requests.insert({kj::StringPtr(id), capnp::clone(results.asReader())}); } void insertOffer( @@ -1229,14 +1228,13 @@ public: capnp::Capability::Client&& offer, PowerboxDescriptor::Reader descriptor) { - auto results = - capnp::MallocMessageBuilder() - .initRoot(); + auto msg = capnp::MallocMessageBuilder(); + auto results = msg.initRoot(); results.setOffer(offer); results.setDescriptor(descriptor); - offers.insert({kj::str(id), capnp::clone(results.asReader())}); + offers.insert({kj::StringPtr(id), capnp::clone(results.asReader())}); } private: @@ -1244,6 +1242,15 @@ private: spk::BridgeConfig::Reader config; kj::AutoCloseFd identitiesDir; kj::AutoCloseFd trashDir; + + // These maps store information relating to active sessions. `sessions` always has an entry + // for any active session that is handled by the app. `requests` and `offers` will only + // have an entry for request and offer sessions, respectively. + // + // In all cases the keys are owned by the session object, which is responsible for deleting + // the session from these maps (via eraseSession) on cleanup. For `sessions`, the value + // is also owned by the session. For `requests` and `offers`, the value is owned by the map + // itself. std::map sessions; std::map> requests; std::map> offers; @@ -1363,7 +1370,7 @@ public: kj::String&& rootPath, kj::String&& permissions, kj::Maybe remoteAddress, kj::Maybe>&& apiInfo, - SessionType sessionType) + SessionInfo::Reader sessionInfo) : serverAddr(serverAddr), sessionContext(kj::mv(sessionContext)), bridgeContext(bridgeContext), @@ -1380,12 +1387,32 @@ public: rootPath(kj::mv(rootPath)), remoteAddress(kj::mv(remoteAddress)), apiInfo(kj::mv(apiInfo)), - sessionType(sessionType) { + sessionType(sessionInfo.which()) { if (userInfo.hasIdentityId()) { userId = textIdentityId(userInfo.getIdentityId()); } if (this->sessionId != nullptr) { bridgeContext.insertSession(kj::StringPtr(this->sessionId), this->sessionContext); + switch(sessionInfo.which()) { + case SessionInfo::NORMAL: + break; + case SessionInfo::REQUEST: + bridgeContext.insertRequest( + kj::StringPtr(this->sessionId), + sessionInfo.getRequest().getRequestInfo()); + break; + case SessionInfo::OFFER: + { + auto offerInfo = sessionInfo.getOffer(); + bridgeContext.insertOffer( + kj::StringPtr(this->sessionId), + offerInfo.getOffer(), + offerInfo.getDescriptor()); + } + break; + default: + KJ_FAIL_ASSERT("Unknown session type."); + } } } @@ -1647,7 +1674,7 @@ private: spk::BridgeConfig::Reader config; kj::Maybe remoteAddress; kj::Maybe> apiInfo; - SessionType sessionType; + SessionInfo::Which sessionType; kj::String makeHeaders(kj::StringPtr method, kj::StringPtr path, WebSession::Context::Reader context, @@ -1710,11 +1737,25 @@ private: } } { - capnp::EnumSchema schema = capnp::Schema::from(); - uint typeValue = static_cast(sessionType); - auto enumerants = schema.getEnumerants(); - lines.add(kj::str("X-Sandstorm-Session-Type: ", - enumerants[typeValue].getProto().getName())); + // TODO(zenhack): there's probably an existing method to get the name + // of a variant; look it up and just do that instead of doing all + // this manually: + auto setHeader = [&](const char *value) { + lines.add(kj::str("X-Sandstorm-Session-Type: ", value)); + }; + switch(sessionType) { + case SessionInfo::Which::NORMAL: + setHeader("normal"); + break; + case SessionInfo::Which::REQUEST: + setHeader("request"); + break; + case SessionInfo::Which::OFFER: + setHeader("offer"); + break; + default: + KJ_FAIL_ASSERT("Unknown session type."); + } } lines.add(kj::str("X-Sandstorm-Permissions: ", permissions)); if (basePath.size() > 0) { @@ -1950,6 +1991,10 @@ WebSession::Client newPowerboxApiSession( userInfo.setIdentityId(httpApi.getIdentityId()); userInfo.setIdentity(kj::mv(identity)); + auto msg = capnp::MallocMessageBuilder(); + auto sessionInfo = msg.initRoot(); + sessionInfo.setNormal(); + return WebSession::Client( kj::heap(serverAddress, userInfo, nullptr, bridgeContext, nullptr, nullptr, @@ -1957,7 +2002,7 @@ WebSession::Client newPowerboxApiSession( kj::str(httpApi.getPath(), '/'), bridgeContext.formatPermissions(httpApi.getPermissions()), nullptr, kj::mv(httpApi), - SessionType::NORMAL)); + sessionInfo)); }); }); } @@ -2341,7 +2386,7 @@ public: WebSession::Params::Reader sessionParams, SessionContext::Client sessionCtx, kj::ArrayPtr tabId, - SessionType sessionType) { + SessionInfo::Reader sessionInfo) { auto userPermissions = userInfo.getPermissions(); return @@ -2354,7 +2399,7 @@ public: kj::heapString("/"), bridgeContext.formatPermissions(userPermissions), nullptr, nullptr, - sessionType); + sessionInfo); } @@ -2373,9 +2418,12 @@ public: } if (sessionType == capnp::typeId()) { - auto userPermissions = userInfo.getPermissions(); auto sessionParams = params.getSessionParams().getAs(); + auto msg = capnp::MallocMessageBuilder(); + auto sessionInfo = msg.initRoot(); + sessionInfo.setNormal(); + UiSession::Client session = newUiSession( userInfo, @@ -2383,7 +2431,7 @@ public: sessionParams, params.getContext(), params.getTabId(), - SessionType::NORMAL); + sessionInfo); context.getResults(capnp::MessageSize {2, 1}).setSession( connectPromise.addBranch().then([KJ_MVCAP(session)]() mutable { @@ -2397,6 +2445,9 @@ public: addr = addressToString(sessionParams.getRemoteAddress()); } + auto msg = capnp::MallocMessageBuilder(); + auto sessionInfo = msg.initRoot(); + sessionInfo.setNormal(); UiSession::Client session = kj::heap(serverAddress, userInfo, params.getContext(), bridgeContext, kj::str(sessionIdCounter++), @@ -2405,7 +2456,7 @@ public: kj::heapString(config.getApiPath()), bridgeContext.formatPermissions(userPermissions), kj::mv(addr), nullptr, - SessionType::NORMAL); + sessionInfo); context.getResults(capnp::MessageSize {2, 1}).setSession( connectPromise.addBranch().then([KJ_MVCAP(session)]() mutable { @@ -2458,8 +2509,12 @@ public: // the request. auto sessionId = kj::str(sessionIdCounter++); - bridgeContext.insertRequest(kj::str(sessionId), requestInfo); auto sessionParams = params.getSessionParams().getAs(); + + auto msg = capnp::MallocMessageBuilder(); + auto sessionInfo = msg.initRoot(); + sessionInfo.initRequest().setRequestInfo(requestInfo); + UiSession::Client session = newUiSession( userInfo, @@ -2467,7 +2522,7 @@ public: sessionParams, params.getContext(), params.getTabId(), - SessionType::REQUEST); + sessionInfo); context.getResults(capnp::MessageSize {2, 1}).setSession( connectPromise.addBranch().then([KJ_MVCAP(session)]() mutable { @@ -2483,7 +2538,11 @@ public: auto userInfo = params.getUserInfo(); auto sessionParams = params.getSessionParams().getAs(); - bridgeContext.insertOffer(kj::str(sessionId), params.getOffer(), params.getDescriptor()); + auto msg = capnp::MallocMessageBuilder(); + auto sessionInfo = msg.initRoot(); + auto offerInfo = sessionInfo.initOffer(); + offerInfo.setOffer(params.getOffer()); + offerInfo.setDescriptor(params.getDescriptor()); UiSession::Client session = newUiSession( @@ -2492,7 +2551,7 @@ public: sessionParams, params.getContext(), params.getTabId(), - SessionType::OFFER); + sessionInfo); context.getResults(capnp::MessageSize {2, 1}).setSession( connectPromise.addBranch().then([KJ_MVCAP(session)]() mutable {